[v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

[PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

Posted by Manas via B4 Relay 1 month, 3 weeks ago

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>

Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

Posted by Matthew Wilcox 1 month, 3 weeks ago

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.

Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

Posted by Peter Xu 1 month, 3 weeks ago

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

Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

Posted by Peter Xu 1 month, 2 weeks ago

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

Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

Posted by Manas 1 month, 3 weeks ago

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

Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

Posted by Manas 1 month, 3 weeks ago

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