Message ID | 20241004-fix-null-deref-v3-1-f9459b1cc95f@iiitd.ac.in (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v3] Fixes: null pointer dereference in pfnmap_lockdep_assert | expand |
On 04.10.2024 19:15, 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 assigns mapping only if vm_file is not NULL. I also edited the commit message (and cover letter) slightly to tell about the newer fix, instead of the v1 fix of returning. I hope this is okay.
On Fri, Oct 04, 2024 at 07:15:48PM +0530, Manas via B4 Relay wrote: > +++ b/mm/memory.c > @@ -6346,10 +6346,10 @@ 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 > - struct address_space *mapping = vma->vm_file->f_mapping; > + struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL; Overly long and complex line. Much simpler to write: struct address_space *mapping = NULL; if (vma->vm_file) mapping = vma->vm_file->f_mapping; > if (mapping) > - lockdep_assert(lockdep_is_held(&vma->vm_file->f_mapping->i_mmap_rwsem) || > + 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)); This one should have been lockdep_assert_held(&vma->vm_mm->mmap_lock). I'm not sure that the previous one is correct. The lockdep_assert_held() macro is pretty careful about checking LOCK_STATE_NOT_HELD to avoid the LOCK_STATE_UNKNOWN possibility. But I'll leave that for Peter to fix.
Hi Matthew, On 04.10.2024 16:17, Matthew Wilcox wrote: >On Fri, Oct 04, 2024 at 07:15:48PM +0530, Manas via B4 Relay wrote: >> +++ b/mm/memory.c >> @@ -6346,10 +6346,10 @@ 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 >> - struct address_space *mapping = vma->vm_file->f_mapping; >> + struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL; > >Overly long and complex line. Much simpler to write: > > struct address_space *mapping = NULL; > > if (vma->vm_file) > mapping = vma->vm_file->f_mapping; > Thank you for reviewing! I am sending v4. >> if (mapping) >> - lockdep_assert(lockdep_is_held(&vma->vm_file->f_mapping->i_mmap_rwsem) || >> + 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)); > >This one should have been lockdep_assert_held(&vma->vm_mm->mmap_lock). > >I'm not sure that the previous one is correct. The >lockdep_assert_held() macro is pretty careful about checking >LOCK_STATE_NOT_HELD to avoid the LOCK_STATE_UNKNOWN possibility. >But I'll leave that for Peter to fix.
diff --git a/mm/memory.c b/mm/memory.c index 2366578015ad..828967a13596 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -6346,10 +6346,10 @@ 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 - struct address_space *mapping = vma->vm_file->f_mapping; + struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL; if (mapping) - lockdep_assert(lockdep_is_held(&vma->vm_file->f_mapping->i_mmap_rwsem) || + 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));