[PATCH] arch/s390/pci: fix fixup_user_fault() calls with NULL unlocked parameter

[PATCH] arch/s390/pci: fix fixup_user_fault() calls with NULL unlocked parameter

[Liu Dalin]

The s390 PCI MMIO functions call fixup_user_fault() with the 'unlocked'
parameter set to NULL. This means when handle_mm_fault() returns
VM_FAULT_COMPLETED or VM_FAULT_RETRY, the function re-acquires the
mmap_lock but cannot notify the caller about this state change.

This is problematic because the caller subsequently calls
mmap_read_unlock() at the end of the function. If the lock was
re-acquired inside fixup_user_fault(), the unlock happens correctly.
But passing NULL makes the code fragile and hard to reason about.

Fix this by providing a proper 'unlocked' variable to fixup_user_fault()
so the lock state is properly tracked.

This is a follow-up to the defensive NULL check added in fixup_user_fault()
by the previous patch in this series.

Fixes: 41a0926e82f4 ("s390/pci: Fix s390_mmio_read/write syscall page fault handling")
Signed-off-by: Liu Dalin <liudalin@kylinsec.com.cn>
---
 arch/s390/pci/pci_mmio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
index f3f79ba78410..5ef2d6436d4b 100644
--- a/arch/s390/pci/pci_mmio.c
+++ b/arch/s390/pci/pci_mmio.c
@@ -182,7 +182,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
 	args.vma = vma;
 	ret = follow_pfnmap_start(&args);
 	if (ret) {
-		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL);
+		bool unlocked = false;
+
+		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, &unlocked);
 		ret = follow_pfnmap_start(&args);
 		if (ret)
 			goto out_unlock_mmap;
@@ -335,7 +337,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr,
 	args.address = mmio_addr;
 	ret = follow_pfnmap_start(&args);
 	if (ret) {
-		fixup_user_fault(current->mm, mmio_addr, 0, NULL);
+		bool unlocked = false;
+
+		fixup_user_fault(current->mm, mmio_addr, 0, &unlocked);
 		ret = follow_pfnmap_start(&args);
 		if (ret)
 			goto out_unlock_mmap;
-- 
2.43.0

Re: [PATCH] arch/s390/pci: fix fixup_user_fault() calls with NULL unlocked parameter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Changing the 'unlocked' parameter of fixup_user_fault() from NULL to &unlocked allows the mmap_lock to be temporarily dropped, causing a Use-After-Free of the VMA pointer.
--

commit 440b0dda47238ff5981413f278a2a1abe962cdd0
Author: Liu Dalin <liudalin@kylinsec.com.cn>
arch/s390/pci: fix fixup_user_fault() calls with NULL unlocked parameter

This patch provides a valid unlocked variable to fixup_user_fault() in
the s390 PCI MMIO read and write handlers to properly track the lock state,
resolving an issue where the lock state was obfuscated by passing NULL.

> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
> index f3f79ba78410a..5ef2d6436d4b1 100644
> --- a/arch/s390/pci/pci_mmio.c
> +++ b/arch/s390/pci/pci_mmio.c
> @@ -182,7 +182,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
>  	args.vma = vma;
>  	ret = follow_pfnmap_start(&args);
>  	if (ret) {
> -		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL);
> +		bool unlocked = false;
> +
> +		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, &unlocked);
>  		ret = follow_pfnmap_start(&args);

[Severity: Critical]
If fixup_user_fault() drops the mmap_read_lock and sets unlocked to true,
a concurrent munmap() could free the VMA. Since args.vma was populated
earlier, calling follow_pfnmap_start(&args) immediately after would 
dereference the freed VMA pointer:

mm/memory.c:follow_pfnmap_start() {
	struct vm_area_struct *vma = args->vma;
	...
	struct mm_struct *mm = vma->vm_mm;

Can this regression be avoided by checking the unlocked state and handling
the possibility that the VMA is no longer valid?

>  		if (ret)
>  			goto out_unlock_mmap;

[ ... ]

> @@ -335,7 +337,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr,
>  	args.address = mmio_addr;
>  	ret = follow_pfnmap_start(&args);
>  	if (ret) {
> -		fixup_user_fault(current->mm, mmio_addr, 0, NULL);
> +		bool unlocked = false;
> +
> +		fixup_user_fault(current->mm, mmio_addr, 0, &unlocked);
>  		ret = follow_pfnmap_start(&args);

[Severity: Critical]
Similarly in s390_pci_mmio_read(), if the lock is temporarily dropped by
fixup_user_fault(), can this second call to follow_pfnmap_start() cause a
use-after-free regression by accessing the stale args.vma pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0726CF177011E0E2+20260826064228.3255764-1-liudalin@kylinsec.com.cn?part=1