| Message ID | 20241003-fix-null-deref-v1-1-0a45df9d016a@iiitd.ac.in (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes: null pointer dereference in pfnmap_lockdep_assert | expand |
On Thu, Oct 03, 2024 at 09:31:06PM +0530, Manas via B4 Relay wrote: > From: Manas <manas18244@iiitd.ac.in> > > syzbot has pointed to a possible null pointer dereference in > pfnmap_lockdep_assert. vm_file member of vm_area_struct is being > dereferenced without any checks. > > This fix returns if vm_file member in vm_area_struct is NULL. This seems like the wrong fix. It's mmap'ing a file, so vm_file should not be NULL. Or have I forgotten something very important about how the MM works? > Reported-by: syzbot+093d096417e7038a689b@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b > --- > This bug[1] triggers a general protection fault in follow_pfnmap_start > function. An assertion pfnmap_lockdep_assert inside this function > dereferences vm_file member of vm_area_struct. And panic gets triggered > when vm_file is NULL. > > This patch returns from the assertion pfnmap_lockdep_assert if vm_file > is found to be NULL. > > [1] https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b > > Signed-off-by: Manas <manas18244@iiitd.ac.in> > --- > mm/memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/memory.c b/mm/memory.c > index 2366578015ad..b152a95e543f 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -6346,6 +6346,9 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args, > static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) > { > #ifdef CONFIG_LOCKDEP > + if (!vma->vm_file) > + return; > + > struct address_space *mapping = vma->vm_file->f_mapping; > > if (mapping) > > --- > base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc > change-id: 20241003-fix-null-deref-6bfa0337efc3 > > Best regards, > -- > Manas <manas18244@iiitd.ac.in> > > >
Matthew, On Thu, Oct 03, 2024 at 09:31:17PM +0100, Matthew Wilcox wrote: > On Thu, Oct 03, 2024 at 09:31:06PM +0530, Manas via B4 Relay wrote: > > From: Manas <manas18244@iiitd.ac.in> > > > > syzbot has pointed to a possible null pointer dereference in > > pfnmap_lockdep_assert. vm_file member of vm_area_struct is being > > dereferenced without any checks. > > > > This fix returns if vm_file member in vm_area_struct is NULL. > > This seems like the wrong fix. It's mmap'ing a file, so vm_file should > not be NULL. Or have I forgotten something very important about how the > MM works? If I read the stack right, the crash was before mmap() of the new vma happens, instead it's during unmap() of one existing vma which existed and overlapped with the new vma's mapping range: follow_phys arch/x86/mm/pat/memtype.c:956 [inline] get_pat_info+0x182/0x3f0 arch/x86/mm/pat/memtype.c:988 untrack_pfn+0x327/0x640 arch/x86/mm/pat/memtype.c:1101 unmap_single_vma+0x1f6/0x2b0 mm/memory.c:1834 unmap_vmas+0x3cc/0x5f0 mm/memory.c:1900 unmap_region+0x214/0x380 mm/vma.c:354 <--------------- here mmap_region+0x22f9/0x2990 mm/mmap.c:1573 do_mmap+0x8f0/0x1000 mm/mmap.c:496 vm_mmap_pgoff+0x1dd/0x3d0 mm/util.c:588 ksys_mmap_pgoff+0x4eb/0x720 mm/mmap.c:542 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f It looks like the vma that was overwritten by the new file vma mapping could be a VM_PFNMAP vma (I'm guessing vvar or something similar..), that's where untrack_pfn() got kicked off. In this case, the vma being overwritten and to be unmapped can have ->vm_file==NULL (while ->vm_ops non-NULL; /me looking at __install_special_mapping()). Thanks,
Hi Peter, thanks for reviewing. On 03.10.2024 12:41, Peter Xu wrote: >On Thu, Oct 03, 2024 at 09:31:06PM +0530, Manas via B4 Relay wrote: >> From: Manas <manas18244@iiitd.ac.in> >> >> syzbot has pointed to a possible null pointer dereference in >> pfnmap_lockdep_assert. vm_file member of vm_area_struct is being >> dereferenced without any checks. >> >> This fix returns if vm_file member in vm_area_struct is NULL. >> >> Reported-by: syzbot+093d096417e7038a689b@syzkaller.appspotmail.com >> Closes: https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b >> --- >> This bug[1] triggers a general protection fault in follow_pfnmap_start >> function. An assertion pfnmap_lockdep_assert inside this function >> dereferences vm_file member of vm_area_struct. And panic gets triggered >> when vm_file is NULL. >> >> This patch returns from the assertion pfnmap_lockdep_assert if vm_file >> is found to be NULL. >> >> [1] https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b >> >> Signed-off-by: Manas <manas18244@iiitd.ac.in> > >Thanks for the patch! > >> --- >> mm/memory.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/mm/memory.c b/mm/memory.c >> index 2366578015ad..b152a95e543f 100644 >> --- a/mm/memory.c >> +++ b/mm/memory.c >> @@ -6346,6 +6346,9 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args, >> static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) >> { >> #ifdef CONFIG_LOCKDEP >> + if (!vma->vm_file) >> + return; >> + > >Hmm I guess I wasn't careful enough here as I was mostly only thinking >about file mappings, but I just notice we have other pfnmaps like the vvar >mappings.. the mapping var can also already be reused later when available. > >Logically even if !vm_file we can still check against mmap write lock. So >would it be better to do this instead: > > struct address_space *mapping = vma->vm_file && vma->vm_file->f_mapping; > This will lead to `-Wint-conversion` error in the assignment. We can either do a cast like the following: struct address_space *mapping = (struct address_space *)(vma->vm_file && vma->vm_file->f_mapping); But I am not sure if it is the canonical way of doing it. It will also lead to warning about pointer from integer casting. Or will a conditional like this work here? struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL; > if (mapping) > lockdep_assert(lockdep_is_held(&mapping->i_mmap_rwsem) || > lockdep_is_held(&vma->vm_mm->mmap_lock)); > else > lockdep_assert(lockdep_is_held(&vma->vm_mm->mmap_lock)); > >? > >> struct address_space *mapping = vma->vm_file->f_mapping; >> >> if (mapping) >> >> --- >> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc >> change-id: 20241003-fix-null-deref-6bfa0337efc3 >> >> Best regards, >> -- >> Manas <manas18244@iiitd.ac.in> >> >> >> > >-- >Peter Xu >
On Fri, Oct 04, 2024 at 03:43:12PM +0530, Manas wrote: > Hi Peter, thanks for reviewing. > > On 03.10.2024 12:41, Peter Xu wrote: > > On Thu, Oct 03, 2024 at 09:31:06PM +0530, Manas via B4 Relay wrote: > > > From: Manas <manas18244@iiitd.ac.in> > > > > > > syzbot has pointed to a possible null pointer dereference in > > > pfnmap_lockdep_assert. vm_file member of vm_area_struct is being > > > dereferenced without any checks. > > > > > > This fix returns if vm_file member in vm_area_struct is NULL. > > > > > > Reported-by: syzbot+093d096417e7038a689b@syzkaller.appspotmail.com > > > Closes: https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b > > > --- > > > This bug[1] triggers a general protection fault in follow_pfnmap_start > > > function. An assertion pfnmap_lockdep_assert inside this function > > > dereferences vm_file member of vm_area_struct. And panic gets triggered > > > when vm_file is NULL. > > > > > > This patch returns from the assertion pfnmap_lockdep_assert if vm_file > > > is found to be NULL. > > > > > > [1] https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b > > > > > > Signed-off-by: Manas <manas18244@iiitd.ac.in> > > > > Thanks for the patch! > > > > > --- > > > mm/memory.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/mm/memory.c b/mm/memory.c > > > index 2366578015ad..b152a95e543f 100644 > > > --- a/mm/memory.c > > > +++ b/mm/memory.c > > > @@ -6346,6 +6346,9 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args, > > > static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) > > > { > > > #ifdef CONFIG_LOCKDEP > > > + if (!vma->vm_file) > > > + return; > > > + > > > > Hmm I guess I wasn't careful enough here as I was mostly only thinking > > about file mappings, but I just notice we have other pfnmaps like the vvar > > mappings.. the mapping var can also already be reused later when available. > > > > Logically even if !vm_file we can still check against mmap write lock. So > > would it be better to do this instead: > > > > struct address_space *mapping = vma->vm_file && vma->vm_file->f_mapping; > > > This will lead to `-Wint-conversion` error in the assignment. We can either do a > cast like the following: > > struct address_space *mapping = (struct address_space *)(vma->vm_file && vma->vm_file->f_mapping); > > But I am not sure if it is the canonical way of doing it. It will also lead to > warning about pointer from integer casting. > > Or will a conditional like this work here? > > struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL; Sorry, that was a pretty stupid mistake of mine, just to show what I meant without any compilation tests. Yes this one. Thanks, > > if (mapping) > > lockdep_assert(lockdep_is_held(&mapping->i_mmap_rwsem) || > > lockdep_is_held(&vma->vm_mm->mmap_lock)); > > else > > lockdep_assert(lockdep_is_held(&vma->vm_mm->mmap_lock)); > > > > ? > > > > > > struct address_space *mapping = vma->vm_file->f_mapping; > > > > > > if (mapping) > > > > > > --- > > > base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc > > > change-id: 20241003-fix-null-deref-6bfa0337efc3 > > > > > > Best regards, > > > -- > > > Manas <manas18244@iiitd.ac.in> > > > > > > > > > > > > > -- > > Peter Xu > > > > -- > Manas >
diff --git a/mm/memory.c b/mm/memory.c index 2366578015ad..b152a95e543f 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -6346,6 +6346,9 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args, static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) { #ifdef CONFIG_LOCKDEP + if (!vma->vm_file) + return; + struct address_space *mapping = vma->vm_file->f_mapping; if (mapping)