| Message ID | 20211201142918.921493-40-Liam.Howlett@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introducing the Maple Tree | expand |
On 12/1/21 15:30, Liam Howlett wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > I'm not sure if the VMA list can change under us, but dump_vma_snapshot() > is very careful to take the mmap_lock in write mode. We only need to > take it in read mode here as we do not care if the size of the stack > VMA changes underneath us. > > If it can be changed underneath us, this is a potential use-after-free > for a multithreaded process which is dumping core. > > Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files") > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> To be honest this feels out of place in this series. Send separately and CC Jann who AFAICS added dump_vma_snapshot()? > --- > fs/binfmt_elf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index d41cca755ff9..5915518c8a1d 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1652,6 +1652,7 @@ static int fill_files_note(struct memelfnote *note) > name_base = name_curpos = ((char *)data) + names_ofs; > remaining = size - names_ofs; > count = 0; > + mmap_read_lock(mm); > for_each_vma(vmi, vma) { > struct file *file; > const char *filename; > @@ -1662,6 +1663,7 @@ static int fill_files_note(struct memelfnote *note) > filename = file_path(file, name_curpos, remaining); > if (IS_ERR(filename)) { > if (PTR_ERR(filename) == -ENAMETOOLONG) { > + mmap_read_unlock(mm); > kvfree(data); > size = size * 5 / 4; > goto alloc; > @@ -1681,6 +1683,7 @@ static int fill_files_note(struct memelfnote *note) > *start_end_ofs++ = vma->vm_pgoff; > count++; > } > + mmap_read_unlock(mm); > > /* Now we know exact count of files, can store it */ > data[0] = count;
* Vlastimil Babka <vbabka@suse.cz> [220119 05:53]: > On 12/1/21 15:30, Liam Howlett wrote: > > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > > > I'm not sure if the VMA list can change under us, but dump_vma_snapshot() > > is very careful to take the mmap_lock in write mode. We only need to > > take it in read mode here as we do not care if the size of the stack > > VMA changes underneath us. > > > > If it can be changed underneath us, this is a potential use-after-free > > for a multithreaded process which is dumping core. > > > > Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files") > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > > To be honest this feels out of place in this series. Send separately and CC > Jann who AFAICS added dump_vma_snapshot()? Thanks. I have moved the version Matthew sent out separately to the start of my v5 set. > > > --- > > fs/binfmt_elf.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > > index d41cca755ff9..5915518c8a1d 100644 > > --- a/fs/binfmt_elf.c > > +++ b/fs/binfmt_elf.c > > @@ -1652,6 +1652,7 @@ static int fill_files_note(struct memelfnote *note) > > name_base = name_curpos = ((char *)data) + names_ofs; > > remaining = size - names_ofs; > > count = 0; > > + mmap_read_lock(mm); > > for_each_vma(vmi, vma) { > > struct file *file; > > const char *filename; > > @@ -1662,6 +1663,7 @@ static int fill_files_note(struct memelfnote *note) > > filename = file_path(file, name_curpos, remaining); > > if (IS_ERR(filename)) { > > if (PTR_ERR(filename) == -ENAMETOOLONG) { > > + mmap_read_unlock(mm); > > kvfree(data); > > size = size * 5 / 4; > > goto alloc; > > @@ -1681,6 +1683,7 @@ static int fill_files_note(struct memelfnote *note) > > *start_end_ofs++ = vma->vm_pgoff; > > count++; > > } > > + mmap_read_unlock(mm); > > > > /* Now we know exact count of files, can store it */ > > data[0] = count; >
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index d41cca755ff9..5915518c8a1d 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1652,6 +1652,7 @@ static int fill_files_note(struct memelfnote *note) name_base = name_curpos = ((char *)data) + names_ofs; remaining = size - names_ofs; count = 0; + mmap_read_lock(mm); for_each_vma(vmi, vma) { struct file *file; const char *filename; @@ -1662,6 +1663,7 @@ static int fill_files_note(struct memelfnote *note) filename = file_path(file, name_curpos, remaining); if (IS_ERR(filename)) { if (PTR_ERR(filename) == -ENAMETOOLONG) { + mmap_read_unlock(mm); kvfree(data); size = size * 5 / 4; goto alloc; @@ -1681,6 +1683,7 @@ static int fill_files_note(struct memelfnote *note) *start_end_ofs++ = vma->vm_pgoff; count++; } + mmap_read_unlock(mm); /* Now we know exact count of files, can store it */ data[0] = count;