* [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert @ 2024-10-04 13:45 Manas via B4 Relay 2024-10-04 13:47 ` Manas 2024-10-04 15:17 ` Matthew Wilcox 0 siblings, 2 replies; 6+ messages in thread From: Manas via B4 Relay @ 2024-10-04 13:45 UTC (permalink / raw) To: Andrew Morton Cc: Peter Xu, Shuah Khan, Anup Sharma, linux-mm, linux-kernel, syzbot+093d096417e7038a689b, Manas 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. 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 assigns mapping only if vm_file is not NULL. [1] https://syzkaller.appspot.com/bug?extid=093d096417e7038a689b Signed-off-by: Manas <manas18244@iiitd.ac.in> --- Changes in v3: - v3: use assigned var instead of accessing member again - Link to v2: https://lore.kernel.org/r/20241004-fix-null-deref-v2-1-23ad90999cd1@iiitd.ac.in Changes in v2: - v2: use ternary operator according to feedback - Link to v1: https://lore.kernel.org/r/20241003-fix-null-deref-v1-1-0a45df9d016a@iiitd.ac.in --- mm/memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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)); --- base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc change-id: 20241003-fix-null-deref-6bfa0337efc3 Best regards, -- Manas <manas18244@iiitd.ac.in> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert 2024-10-04 13:45 [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert Manas via B4 Relay @ 2024-10-04 13:47 ` Manas 2024-10-04 15:17 ` Matthew Wilcox 1 sibling, 0 replies; 6+ messages in thread From: Manas @ 2024-10-04 13:47 UTC (permalink / raw) To: Andrew Morton, Peter Xu, Shuah Khan, Anup Sharma, linux-mm, linux-kernel, syzbot+093d096417e7038a689b 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. -- Manas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert 2024-10-04 13:45 [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert Manas via B4 Relay 2024-10-04 13:47 ` Manas @ 2024-10-04 15:17 ` Matthew Wilcox 2024-10-04 17:40 ` Manas 2024-10-07 13:23 ` Peter Xu 1 sibling, 2 replies; 6+ messages in thread From: Matthew Wilcox @ 2024-10-04 15:17 UTC (permalink / raw) To: manas18244 Cc: Andrew Morton, Peter Xu, Shuah Khan, Anup Sharma, linux-mm, linux-kernel, syzbot+093d096417e7038a689b 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert 2024-10-04 15:17 ` Matthew Wilcox @ 2024-10-04 17:40 ` Manas 2024-10-07 13:23 ` Peter Xu 1 sibling, 0 replies; 6+ messages in thread From: Manas @ 2024-10-04 17:40 UTC (permalink / raw) To: Matthew Wilcox Cc: Andrew Morton, Peter Xu, Shuah Khan, Anup Sharma, linux-mm, linux-kernel, syzbot+093d096417e7038a689b 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. -- Manas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert 2024-10-04 15:17 ` Matthew Wilcox 2024-10-04 17:40 ` Manas @ 2024-10-07 13:23 ` Peter Xu 2024-10-08 19:27 ` Peter Xu 1 sibling, 1 reply; 6+ messages in thread From: Peter Xu @ 2024-10-07 13:23 UTC (permalink / raw) To: Matthew Wilcox Cc: manas18244, Andrew Morton, Shuah Khan, Anup Sharma, linux-mm, linux-kernel, syzbot+093d096417e7038a689b On Fri, Oct 04, 2024 at 04:17:42PM +0100, 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; > > > 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. Indeed.. Then looks like we could have quite a few other places in Linux that can have used this wrong.. when the assert wants to check against either of the two locks (one mutex or rcu read lock, for example) is held. I'll send a patch after this one lands. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert 2024-10-07 13:23 ` Peter Xu @ 2024-10-08 19:27 ` Peter Xu 0 siblings, 0 replies; 6+ messages in thread From: Peter Xu @ 2024-10-08 19:27 UTC (permalink / raw) To: Matthew Wilcox Cc: manas18244, Andrew Morton, Shuah Khan, Anup Sharma, linux-mm, linux-kernel, syzbot+093d096417e7038a689b On Mon, Oct 07, 2024 at 09:23:47AM -0400, Peter Xu wrote: > On Fri, Oct 04, 2024 at 04:17:42PM +0100, 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; > > > > > 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. > > Indeed.. > > Then looks like we could have quite a few other places in Linux that can > have used this wrong.. when the assert wants to check against either of the > two locks (one mutex or rcu read lock, for example) is held. > > I'll send a patch after this one lands. Just to follow this up and leave a record: I had a closer look today and then quickly I found above should be all fine (similar to all kernel usages like this, for example, rcu_dereference_check()). The trick is LOCK_STATE_NOT_HELD is defined as 0: #define LOCK_STATE_UNKNOWN -1 #define LOCK_STATE_NOT_HELD 0 #define LOCK_STATE_HELD 1 So this: #define lockdep_assert_held(l) \ lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD) Is the same to: #define lockdep_assert_held(l) \ lockdep_assert(lockdep_is_held(l)) The lockdep_assert() was introduced exactly for such >1 lock assertion use cases, in this commit: commit d19c81378829e5d774c951219c5a973965b9202c Author: Peter Zijlstra <peterz@infradead.org> Date: Mon Aug 2 18:59:56 2021 +0800 locking/lockdep: Provide lockdep_assert{,_once}() helpers Extract lockdep_assert{,_once}() helpers to more easily write composite assertions like, for example: lockdep_assert(lockdep_is_held(&drm_device.master_mutex) || lockdep_is_held(&drm_file.master_lookup_lock)); Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-05 15:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-10-04 13:45 [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert Manas via B4 Relay 2024-10-04 13:47 ` Manas 2024-10-04 15:17 ` Matthew Wilcox 2024-10-04 17:40 ` Manas 2024-10-07 13:23 ` Peter Xu 2024-10-08 19:27 ` Peter Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox