[PATCH] Fixes: null pointer dereference in pfnmap_lockdep_assert

[PATCH] Fixes: null pointer dereference in pfnmap_lockdep_assert

[Manas via B4 Relay]

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>
---
 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>

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

[Peter Xu]

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;

        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

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

[Manas]

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
>

-- 
Manas

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

[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
> 

-- 
Peter Xu

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

[Matthew Wilcox]

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>
> 
> 
> 

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

[Peter Xu]

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,

-- 
Peter Xu