[PATCH] mm/gup: fix NULL pointer dereference in fixup_user_fault()

[PATCH] mm/gup: fix NULL pointer dereference in fixup_user_fault()

[Nguyen Duy Nhat Anh]

In fixup_user_fault(), the 'unlocked' parameter is checked for NULL
early on, allowing callers to pass NULL if they do not track whether the
mmap lock was dropped.

However, if handle_mm_fault() returns VM_FAULT_COMPLETED, line 1597
dereferences 'unlocked' directly (*unlocked = true) without checking
if it is NULL. Callers like s390's pci_mmio.c pass NULL for 'unlocked',
leading to a kernel NULL pointer dereference when VM_FAULT_COMPLETED
occurs.

Fix this by checking if 'unlocked' is non-NULL before assigning to it.
Signed-off-by: Nguyen Duy Nhat Anh <neganhat@gmail.com>
---
 mm/gup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index eb898ea1ee22..64f709a45adc 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1594,7 +1594,8 @@ int fixup_user_fault(struct mm_struct *mm,
 		 * could tell the callers so they do not need to unlock.
 		 */
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		return 0;
 	}
 
@@ -1608,7 +1609,8 @@ int fixup_user_fault(struct mm_struct *mm,
 
 	if (ret & VM_FAULT_RETRY) {
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		fault_flags |= FAULT_FLAG_TRIED;
 		goto retry;
 	}
-- 
2.55.0

Re: [PATCH] mm/gup: fix NULL pointer dereference in fixup_user_fault()

[Andrew Morton]

On Sun,  4 Oct 2026 02:48:46 +0700 Nguyen Duy Nhat Anh <neganhat@gmail.com> wrote:

> In fixup_user_fault(), the 'unlocked' parameter is checked for NULL
> early on, allowing callers to pass NULL if they do not track whether the
> mmap lock was dropped.
> 
> However, if handle_mm_fault() returns VM_FAULT_COMPLETED, line 1597
> dereferences 'unlocked' directly (*unlocked = true) without checking
> if it is NULL. Callers like s390's pci_mmio.c pass NULL for 'unlocked',
> leading to a kernel NULL pointer dereference when VM_FAULT_COMPLETED
> occurs.
> 
> Fix this by checking if 'unlocked' is non-NULL before assigning to it.

This code is too subtle so you aren't the first to attempt to "fix" it.

The key hint is in the kerneldoc:

 * @unlocked:	did we unlock the mmap_lock while retrying, maybe NULL if caller
 *		does not allow retry. If NULL, the caller must guarantee
 *		that fault_flags does not contain FAULT_FLAG_ALLOW_RETRY.

Trace through the
FAULT_FLAG_ALLOW_RETRY/VM_FAULT_COMPLETED/VM_FAULT_RETRY logic
to confirm that the null deref is a cant-happen.

It would be great if you could pick through all this and propose
addition of a comment which will make this code less confusing for
others.

Thanks.

Re: [PATCH] mm/gup: fix NULL pointer dereference in fixup_user_fault()

[Lance Yang]

On Sun, Oct 04, 2026 at 12:56:01PM -0700, Andrew Morton wrote:
>On Sun,  4 Oct 2026 02:48:46 +0700 Nguyen Duy Nhat Anh <neganhat@gmail.com> wrote:
>
>> In fixup_user_fault(), the 'unlocked' parameter is checked for NULL
>> early on, allowing callers to pass NULL if they do not track whether the
>> mmap lock was dropped.
>> 
>> However, if handle_mm_fault() returns VM_FAULT_COMPLETED, line 1597
>> dereferences 'unlocked' directly (*unlocked = true) without checking
>> if it is NULL. Callers like s390's pci_mmio.c pass NULL for 'unlocked',
>> leading to a kernel NULL pointer dereference when VM_FAULT_COMPLETED
>> occurs.
>> 
>> Fix this by checking if 'unlocked' is non-NULL before assigning to it.
>
>This code is too subtle so you aren't the first to attempt to "fix" it.
>
>The key hint is in the kerneldoc:
>
> * @unlocked:	did we unlock the mmap_lock while retrying, maybe NULL if caller
> *		does not allow retry. If NULL, the caller must guarantee
> *		that fault_flags does not contain FAULT_FLAG_ALLOW_RETRY.
>
>Trace through the
>FAULT_FLAG_ALLOW_RETRY/VM_FAULT_COMPLETED/VM_FAULT_RETRY logic
>to confirm that the null deref is a cant-happen.

IIUC, that's indeed a can't-happen :)

Re: [PATCH] mm/gup: fix NULL pointer dereference in fixup_user_fault()

[David Hildenbrand (Arm)]

On 10/5/26 04:19, Lance Yang wrote:
> 
> On Sun, Oct 04, 2026 at 12:56:01PM -0700, Andrew Morton wrote:
>> On Sun,  4 Oct 2026 02:48:46 +0700 Nguyen Duy Nhat Anh <neganhat@gmail.com> wrote:
>>
>>> In fixup_user_fault(), the 'unlocked' parameter is checked for NULL
>>> early on, allowing callers to pass NULL if they do not track whether the
>>> mmap lock was dropped.
>>>
>>> However, if handle_mm_fault() returns VM_FAULT_COMPLETED, line 1597
>>> dereferences 'unlocked' directly (*unlocked = true) without checking
>>> if it is NULL. Callers like s390's pci_mmio.c pass NULL for 'unlocked',
>>> leading to a kernel NULL pointer dereference when VM_FAULT_COMPLETED
>>> occurs.
>>>
>>> Fix this by checking if 'unlocked' is non-NULL before assigning to it.
>>
>> This code is too subtle so you aren't the first to attempt to "fix" it.
>>
>> The key hint is in the kerneldoc:
>>
>> * @unlocked:	did we unlock the mmap_lock while retrying, maybe NULL if caller
>> *		does not allow retry. If NULL, the caller must guarantee
>> *		that fault_flags does not contain FAULT_FLAG_ALLOW_RETRY.
>>
>> Trace through the
>> FAULT_FLAG_ALLOW_RETRY/VM_FAULT_COMPLETED/VM_FAULT_RETRY logic
>> to confirm that the null deref is a cant-happen.
> 
> IIUC, that's indeed a can't-happen :)

https://lore.kernel.org/linux-mm/c44e5924-091f-43ed-8cff-34e87c3d9763@kernel.org/

People should stop trusting tool output and use their brain ;)

If you did mmap_read_lock() and wouldn't have a way to indicate that to the user
something would be seriously messed up.

-- 
Cheers,

David

[PATCH v2] mm/gup: document unlocked invariant in fixup_user_fault

[Nguyen Duy Nhat Anh]

Static analysis tools flag potential NULL pointer
dereferences of 'unlocked' in fixup_user_fault() when handling
VM_FAULT_COMPLETED or VM_FAULT_RETRY.

These warnings are false positives. 'unlocked' is only dereferenced when
handle_mm_fault() returns VM_FAULT_COMPLETED or VM_FAULT_RETRY. Both of
these return codes require FAULT_FLAG_ALLOW_RETRY to be set in
fault_flags, which fixup_user_fault() only sets if 'unlocked' is non-NULL
upon entry. Therefore, if 'unlocked' is NULL, the control flow branches
that dereference 'unlocked' are unreachable.

However, this part of the code is subtle and can trip up
contributors or automated tools. Document this invariant with a comment
above 'if (unlocked)' where fault_flags is constructed, explaining why
omitting FAULT_FLAG_ALLOW_RETRY guarantees 'unlocked' will not be
dereferenced later in the fault recovery loop.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Nguyen Duy Nhat Anh <neganhat@gmail.com>
---
v1: https://lore.kernel.org/linux-mm/20261003194846.205918-1-neganhat@gmail.com/

v2 changes:
- Instead of adding runtime NULL checks at dereference sites,
  document the FAULT_FLAG_ALLOW_RETRY invariant above if (unlocked).
---
 mm/gup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index eb898ea1ee22..58fa75441da1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1570,6 +1570,12 @@ int fixup_user_fault(struct mm_struct *mm,
 
 	address = untagged_addr_remote(mm, address);
 
+	/*
+	 * If the caller passes 'unlocked' as NULL, FAULT_FLAG_ALLOW_RETRY is omitted.
+	 * This guarantees handle_mm_fault() will never drop the lock or
+	 * return VM_FAULT_COMPLETED / VM_FAULT_RETRY, making subsequent
+	 * dereferences of 'unlocked' unreachable when NULL.
+	 */
 	if (unlocked)
 		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
 
-- 
2.55.0

Re: [PATCH v2] mm/gup: document unlocked invariant in fixup_user_fault

[David Hildenbrand (Arm)]

On 10/5/26 11:37, Nguyen Duy Nhat Anh wrote:
> Static analysis tools flag potential NULL pointer
> dereferences of 'unlocked' in fixup_user_fault() when handling
> VM_FAULT_COMPLETED or VM_FAULT_RETRY.
> 
> These warnings are false positives. 'unlocked' is only dereferenced when
> handle_mm_fault() returns VM_FAULT_COMPLETED or VM_FAULT_RETRY. Both of
> these return codes require FAULT_FLAG_ALLOW_RETRY to be set in
> fault_flags, which fixup_user_fault() only sets if 'unlocked' is non-NULL
> upon entry. Therefore, if 'unlocked' is NULL, the control flow branches
> that dereference 'unlocked' are unreachable.
> 
> However, this part of the code is subtle and can trip up
> contributors or automated tools. Document this invariant with a comment
> above 'if (unlocked)' where fault_flags is constructed, explaining why
> omitting FAULT_FLAG_ALLOW_RETRY guarantees 'unlocked' will not be
> dereferenced later in the fault recovery loop.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Nguyen Duy Nhat Anh <neganhat@gmail.com>
> ---
> v1: https://lore.kernel.org/linux-mm/20261003194846.205918-1-neganhat@gmail.com/
> 
> v2 changes:
> - Instead of adding runtime NULL checks at dereference sites,
>   document the FAULT_FLAG_ALLOW_RETRY invariant above if (unlocked).
> ---
>  mm/gup.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index eb898ea1ee22..58fa75441da1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1570,6 +1570,12 @@ int fixup_user_fault(struct mm_struct *mm,
>  
>  	address = untagged_addr_remote(mm, address);
>  
> +	/*
> +	 * If the caller passes 'unlocked' as NULL, FAULT_FLAG_ALLOW_RETRY is omitted.
> +	 * This guarantees handle_mm_fault() will never drop the lock or
> +	 * return VM_FAULT_COMPLETED / VM_FAULT_RETRY, making subsequent
> +	 * dereferences of 'unlocked' unreachable when NULL.
> +	 */
>  	if (unlocked)
>  		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>  

For which audience is the comment targeted? For people that know what they are
doing? No.

For tools that report such problems? No.

So I don't think this (4 lines of comment) is the right way to improve this.

-- 
Cheers,

David

Re: [PATCH v2] mm/gup: document unlocked invariant in fixup_user_fault

[Nhật Anh Nguyễn Duy]

Hi David,

Thanks for taking the time to review.

My motivation came from running Smatch over fixup_user_fault(), which flagged
the dereference as a potential NULL pointer issue. Because the relationship
between FAULT_FLAG_ALLOW_RETRY and the unlocked pointer is subtle when
first reading through the path, I initially misread it as an unguarded
dereference in v1.

When Andrew pointed out the invariant, I thought documenting it might help
future readers or newer contributors who hit the same static analysis warning.

If adding a comment isn't the right approach here, would you prefer leaving
the implicit contract as-is, or is an explicit runtime check (such as
VM_WARN_ON_ONCE) something more fitting?

Thanks,
Anh

On Mon, Oct 5, 2026 at 5:29 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
>
> On 10/5/26 11:37, Nguyen Duy Nhat Anh wrote:
> > Static analysis tools flag potential NULL pointer
> > dereferences of 'unlocked' in fixup_user_fault() when handling
> > VM_FAULT_COMPLETED or VM_FAULT_RETRY.
> >
> > These warnings are false positives. 'unlocked' is only dereferenced when
> > handle_mm_fault() returns VM_FAULT_COMPLETED or VM_FAULT_RETRY. Both of
> > these return codes require FAULT_FLAG_ALLOW_RETRY to be set in
> > fault_flags, which fixup_user_fault() only sets if 'unlocked' is non-NULL
> > upon entry. Therefore, if 'unlocked' is NULL, the control flow branches
> > that dereference 'unlocked' are unreachable.
> >
> > However, this part of the code is subtle and can trip up
> > contributors or automated tools. Document this invariant with a comment
> > above 'if (unlocked)' where fault_flags is constructed, explaining why
> > omitting FAULT_FLAG_ALLOW_RETRY guarantees 'unlocked' will not be
> > dereferenced later in the fault recovery loop.
> >
> > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Nguyen Duy Nhat Anh <neganhat@gmail.com>
> > ---
> > v1: https://lore.kernel.org/linux-mm/20261003194846.205918-1-neganhat@gmail.com/
> >
> > v2 changes:
> > - Instead of adding runtime NULL checks at dereference sites,
> >   document the FAULT_FLAG_ALLOW_RETRY invariant above if (unlocked).
> > ---
> >  mm/gup.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index eb898ea1ee22..58fa75441da1 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1570,6 +1570,12 @@ int fixup_user_fault(struct mm_struct *mm,
> >
> >       address = untagged_addr_remote(mm, address);
> >
> > +     /*
> > +      * If the caller passes 'unlocked' as NULL, FAULT_FLAG_ALLOW_RETRY is omitted.
> > +      * This guarantees handle_mm_fault() will never drop the lock or
> > +      * return VM_FAULT_COMPLETED / VM_FAULT_RETRY, making subsequent
> > +      * dereferences of 'unlocked' unreachable when NULL.
> > +      */
> >       if (unlocked)
> >               fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> >
>
> For which audience is the comment targeted? For people that know what they are
> doing? No.
>
> For tools that report such problems? No.
>
> So I don't think this (4 lines of comment) is the right way to improve this.
>
> --
> Cheers,
>
> David