* [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages
@ 2026-08-06 4:50 Peiyang He
2026-08-07 5:36 ` Tushar Nimkar
2026-08-10 19:59 ` Jason Gunthorpe
0 siblings, 2 replies; 6+ messages in thread
From: Peiyang He @ 2026-08-06 4:50 UTC (permalink / raw)
To: kevin.tian, jgg, joro, will, iommu, linux-kernel
Cc: robin.murphy, steven.sistare, stable, Peiyang He
iommufd_ioas_change_process() iterates every IOAS area while only
holding every IOAS iova_rwsem, so it assumes every area has a non-NULL
pages pointer. That assumption can be false when it runs concurrently
with iopt_map_file_pages().
iopt_map_pages() executes in two phases. It first creates the area and
inserts it into the interval tree under iova_rwsem, with area->pages
still NULL. It then drops iova_rwsem and later fills area->pages
under domains_rwsem. This leaves a window between area creation and
area->pages fill where a concurrent iommufd_ioas_change_process()
can observe the area and dereference a NULL area->pages pointer,
leading to a NULL pointer dereference:
BUG: kernel NULL pointer dereference, address: 00000000000000c0
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 4b655067 P4D 4b655067 PUD 0
Oops: Oops: 0000 [#1] SMP NOPTI
CPU: 0 UID: 0 PID: 11841 Comm: syz.1.628 Not tainted 7.1.0 #3 PREEMPT(full)
Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
RIP: 0010:iommufd_ioas_change_process+0x419/0xd50 drivers/iommu/iommufd/ioas.c:538
Code: 48 89 c3 48 85 c0 0f 84 cc 00 00 00 e8 10 f5 cb fd 48 8d 7b 68 e8 a7 b5 eb fd 48 8b 6b 68 48 8d bd c0 00 00 00 e8 17 b2 eb fd <8b> ad c0 00 00 00 bf 01 00 00 00 89 ee e8 85 ef cb fd 83 fd 01 74
RSP: 0018:ffffc90015c17d28 EFLAGS: 00010246
RAX: ffff8880186d5328 RBX: ffff88801d25e240 RCX: 0000000080000000
RDX: 00000000000002d7 RSI: ffffffff83ba9e10 RDI: 00000000000000c0
RBP: 0000000000000000 R08: ffffffff8e781eb8 R09: 0000000000000000
R10: 00000000000000c0 R11: ffffffff83ba9e29 R12: ffff88802e216008
R13: ffff88802e216000 R14: 0000000000000001 R15: 0000000000000000
FS: 00007f4aea3f66c0(0000) GS:ffff8880b1fa1000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000000000c0 CR3: 000000004b75c000 CR4: 0000000000350ef0
Call Trace:
<TASK>
iommufd_fops_ioctl+0x287/0x400 drivers/iommu/iommufd/main.c:533
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl fs/ioctl.c:583 [inline]
__x64_sys_ioctl+0x120/0x170 fs/ioctl.c:583
x64_sys_call+0x1092/0x1fb0 arch/x86/include/generated/asm/syscalls_64.h:17
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x10a/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f4aec1a82bd
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f4aea3f6018 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f4aec436090 RCX: 00007f4aec1a82bd
RDX: 0000200000000180 RSI: 0000000000003b92 RDI: 0000000000000003
RBP: 00007f4aec250295 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007f4aec436128 R14: 00007f4aec436090 R15: 00007ffd04ef23e0
</TASK>
Modules linked in:
CR2: 00000000000000c0
---[ end trace 0000000000000000 ]---
RIP: 0010:iommufd_ioas_change_process+0x419/0xd50 drivers/iommu/iommufd/ioas.c:538
Code: 48 89 c3 48 85 c0 0f 84 cc 00 00 00 e8 10 f5 cb fd 48 8d 7b 68 e8 a7 b5 eb fd 48 8b 6b 68 48 8d bd c0 00 00 00 e8 17 b2 eb fd <8b> ad c0 00 00 00 bf 01 00 00 00 89 ee e8 85 ef cb fd 83 fd 01 74
RSP: 0018:ffffc90015c17d28 EFLAGS: 00010246
RAX: ffff8880186d5328 RBX: ffff88801d25e240 RCX: 0000000080000000
RDX: 00000000000002d7 RSI: ffffffff83ba9e10 RDI: 00000000000000c0
RBP: 0000000000000000 R08: ffffffff8e781eb8 R09: 0000000000000000
R10: 00000000000000c0 R11: ffffffff83ba9e29 R12: ffff88802e216008
R13: ffff88802e216000 R14: 0000000000000001 R15: 0000000000000000
FS: 00007f4aea3f66c0(0000) GS:ffff8880b1fa1000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000000000c0 CR3: 000000004b75c000 CR4: 0000000000350ef0
----------------
Code disassembly (best guess):
0: 48 89 c3 mov %rax,%rbx
3: 48 85 c0 test %rax,%rax
6: 0f 84 cc 00 00 00 je 0xd8
c: e8 10 f5 cb fd call 0xfdcbf521
11: 48 8d 7b 68 lea 0x68(%rbx),%rdi
15: e8 a7 b5 eb fd call 0xfdebb5c1
1a: 48 8b 6b 68 mov 0x68(%rbx),%rbp
1e: 48 8d bd c0 00 00 00 lea 0xc0(%rbp),%rdi
25: e8 17 b2 eb fd call 0xfdebb241
* 2a: 8b ad c0 00 00 00 mov 0xc0(%rbp),%ebp <-- trapping instruction
30: bf 01 00 00 00 mov $0x1,%edi
35: 89 ee mov %ebp,%esi
37: e8 85 ef cb fd call 0xfdcbefc1
3c: 83 fd 01 cmp $0x1,%ebp
3f: 74 .byte 0x74
Fix by mirroring the handling logic in iopt_unmap_iova_range(),
i.e. return -EBUSY if area->pages is NULL.
Fixes: 829ed626499c ("iommufd: Add IOMMU_IOAS_CHANGE_PROCESS")
Cc: stable@vger.kernel.org
Signed-off-by: Peiyang He <peiyang_he@smail.nju.edu.cn>
---
drivers/iommu/iommufd/ioas.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c
index fed06c2b728e..71bffece84b5 100644
--- a/drivers/iommu/iommufd/ioas.c
+++ b/drivers/iommu/iommufd/ioas.c
@@ -535,6 +535,10 @@ int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
return rc;
for_each_ioas_area(&ioas_list, index, ioas, area) {
+ if (!area->pages) {
+ rc = -EBUSY;
+ goto out;
+ }
if (area->pages->type != IOPT_ADDRESS_FILE) {
rc = -EINVAL;
goto out;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages
2026-08-06 4:50 [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages Peiyang He
@ 2026-08-07 5:36 ` Tushar Nimkar
2026-08-07 8:13 ` Peiyang He
2026-08-10 19:59 ` Jason Gunthorpe
1 sibling, 1 reply; 6+ messages in thread
From: Tushar Nimkar @ 2026-08-07 5:36 UTC (permalink / raw)
To: Peiyang He, kevin.tian, jgg, joro, will, iommu, linux-kernel
Cc: robin.murphy, steven.sistare, stable
Hey curious to know,
On 8/6/2026 10:20 AM, Peiyang He wrote:
> [You don't often get email from peiyang_he@smail.nju.edu.cn. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> iommufd_ioas_change_process() iterates every IOAS area while only
> holding every IOAS iova_rwsem, so it assumes every area has a non-NULL
> pages pointer. That assumption can be false when it runs concurrently
> with iopt_map_file_pages().
>
> iopt_map_pages() executes in two phases. It first creates the area and
> inserts it into the interval tree under iova_rwsem, with area->pages
> still NULL. It then drops iova_rwsem and later fills area->pages
> under domains_rwsem. This leaves a window between area creation and
> area->pages fill where a concurrent iommufd_ioas_change_process()
> can observe the area and dereference a NULL area->pages pointer,
> leading to a NULL pointer dereference:
AFAIU, the flow must be
iopt_map_file_pages()
->iopt_map_common()
->iopt_map_pages() <- here it does create and write pages ..
iommufd_ioas_change_process() <-the page pointer is being accessed..
1. Why IOCTL got triggered before setting the respective functionality ?
2. Since here unlocked_ioctl() used which means driver should have
manage locking as user space will not hold global Big Kernel Lock (BKL).
We have pages->mutex - do you think we need it some where instead
of "-EBUSY" return ? - if yes can you try and repro ?
snip:
*
* The locking order is domains_rwsem -> iova_rwsem -> pages::mutex
*/
struct io_pagetable {
---
down_read(&iopt->domains_rwsem);
rc = iopt_fill_domains_pages(pages_list);
if (rc)
goto out_unlock_domains;
down_write(&iopt->iova_rwsem);
list_for_each_entry(elm, pages_list, next) {
/*
* area->pages must be set inside the domains_rwsem to
ensure
* any newly added domains will get filled. Moves the
reference
* in from the list.
*/
elm->area->pages = elm->pages; <- maybe page mutex to
protect this ?
elm->pages = NULL;
elm->area = NULL;
}
up_write(&iopt->iova_rwsem);
out_unlock_domains:
up_read(&iopt->domains_rwsem);
return rc;
Thanks,
Tushar Nimkar
>
> BUG: kernel NULL pointer dereference, address: 00000000000000c0
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 4b655067 P4D 4b655067 PUD 0
> Oops: Oops: 0000 [#1] SMP NOPTI
> CPU: 0 UID: 0 PID: 11841 Comm: syz.1.628 Not tainted 7.1.0 #3 PREEMPT(full)
> Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> RIP: 0010:iommufd_ioas_change_process+0x419/0xd50 drivers/iommu/iommufd/ioas.c:538
> Code: 48 89 c3 48 85 c0 0f 84 cc 00 00 00 e8 10 f5 cb fd 48 8d 7b 68 e8 a7 b5 eb fd 48 8b 6b 68 48 8d bd c0 00 00 00 e8 17 b2 eb fd <8b> ad c0 00 00 00 bf 01 00 00 00 89 ee e8 85 ef cb fd 83 fd 01 74
> RSP: 0018:ffffc90015c17d28 EFLAGS: 00010246
> RAX: ffff8880186d5328 RBX: ffff88801d25e240 RCX: 0000000080000000
> RDX: 00000000000002d7 RSI: ffffffff83ba9e10 RDI: 00000000000000c0
> RBP: 0000000000000000 R08: ffffffff8e781eb8 R09: 0000000000000000
> R10: 00000000000000c0 R11: ffffffff83ba9e29 R12: ffff88802e216008
> R13: ffff88802e216000 R14: 0000000000000001 R15: 0000000000000000
> FS: 00007f4aea3f66c0(0000) GS:ffff8880b1fa1000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000000000c0 CR3: 000000004b75c000 CR4: 0000000000350ef0
> Call Trace:
> <TASK>
> iommufd_fops_ioctl+0x287/0x400 drivers/iommu/iommufd/main.c:533
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:597 [inline]
> __se_sys_ioctl fs/ioctl.c:583 [inline]
> __x64_sys_ioctl+0x120/0x170 fs/ioctl.c:583
> x64_sys_call+0x1092/0x1fb0 arch/x86/include/generated/asm/syscalls_64.h:17
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x10a/0x680 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f4aec1a82bd
> Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f4aea3f6018 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007f4aec436090 RCX: 00007f4aec1a82bd
> RDX: 0000200000000180 RSI: 0000000000003b92 RDI: 0000000000000003
> RBP: 00007f4aec250295 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f4aec436128 R14: 00007f4aec436090 R15: 00007ffd04ef23e0
> </TASK>
> Modules linked in:
> CR2: 00000000000000c0
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:iommufd_ioas_change_process+0x419/0xd50 drivers/iommu/iommufd/ioas.c:538
> Code: 48 89 c3 48 85 c0 0f 84 cc 00 00 00 e8 10 f5 cb fd 48 8d 7b 68 e8 a7 b5 eb fd 48 8b 6b 68 48 8d bd c0 00 00 00 e8 17 b2 eb fd <8b> ad c0 00 00 00 bf 01 00 00 00 89 ee e8 85 ef cb fd 83 fd 01 74
> RSP: 0018:ffffc90015c17d28 EFLAGS: 00010246
> RAX: ffff8880186d5328 RBX: ffff88801d25e240 RCX: 0000000080000000
> RDX: 00000000000002d7 RSI: ffffffff83ba9e10 RDI: 00000000000000c0
> RBP: 0000000000000000 R08: ffffffff8e781eb8 R09: 0000000000000000
> R10: 00000000000000c0 R11: ffffffff83ba9e29 R12: ffff88802e216008
> R13: ffff88802e216000 R14: 0000000000000001 R15: 0000000000000000
> FS: 00007f4aea3f66c0(0000) GS:ffff8880b1fa1000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000000000c0 CR3: 000000004b75c000 CR4: 0000000000350ef0
> ----------------
> Code disassembly (best guess):
> 0: 48 89 c3 mov %rax,%rbx
> 3: 48 85 c0 test %rax,%rax
> 6: 0f 84 cc 00 00 00 je 0xd8
> c: e8 10 f5 cb fd call 0xfdcbf521
> 11: 48 8d 7b 68 lea 0x68(%rbx),%rdi
> 15: e8 a7 b5 eb fd call 0xfdebb5c1
> 1a: 48 8b 6b 68 mov 0x68(%rbx),%rbp
> 1e: 48 8d bd c0 00 00 00 lea 0xc0(%rbp),%rdi
> 25: e8 17 b2 eb fd call 0xfdebb241
> * 2a: 8b ad c0 00 00 00 mov 0xc0(%rbp),%ebp <-- trapping instruction
> 30: bf 01 00 00 00 mov $0x1,%edi
> 35: 89 ee mov %ebp,%esi
> 37: e8 85 ef cb fd call 0xfdcbefc1
> 3c: 83 fd 01 cmp $0x1,%ebp
> 3f: 74 .byte 0x74
>
> Fix by mirroring the handling logic in iopt_unmap_iova_range(),
> i.e. return -EBUSY if area->pages is NULL.
>
> Fixes: 829ed626499c ("iommufd: Add IOMMU_IOAS_CHANGE_PROCESS")
> Cc: stable@vger.kernel.org
> Signed-off-by: Peiyang He <peiyang_he@smail.nju.edu.cn>
> ---
> drivers/iommu/iommufd/ioas.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c
> index fed06c2b728e..71bffece84b5 100644
> --- a/drivers/iommu/iommufd/ioas.c
> +++ b/drivers/iommu/iommufd/ioas.c
> @@ -535,6 +535,10 @@ int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
> return rc;
>
> for_each_ioas_area(&ioas_list, index, ioas, area) {
> + if (!area->pages) {
> + rc = -EBUSY;
> + goto out;
> + }
> if (area->pages->type != IOPT_ADDRESS_FILE) {
> rc = -EINVAL;
> goto out;
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages
2026-08-07 5:36 ` Tushar Nimkar
@ 2026-08-07 8:13 ` Peiyang He
0 siblings, 0 replies; 6+ messages in thread
From: Peiyang He @ 2026-08-07 8:13 UTC (permalink / raw)
To: Tushar Nimkar, kevin.tian, jgg, joro, will, iommu, linux-kernel
Cc: robin.murphy, steven.sistare, stable
Thanks for your reply!
On 2026/8/7 13:36, Tushar Nimkar wrote:
> Hey curious to know,
>
> On 8/6/2026 10:20 AM, Peiyang He wrote:
>> [You don't often get email from peiyang_he@smail.nju.edu.cn. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> iommufd_ioas_change_process() iterates every IOAS area while only
>> holding every IOAS iova_rwsem, so it assumes every area has a non-NULL
>> pages pointer. That assumption can be false when it runs concurrently
>> with iopt_map_file_pages().
>>
>> iopt_map_pages() executes in two phases. It first creates the area and
>> inserts it into the interval tree under iova_rwsem, with area->pages
>> still NULL. It then drops iova_rwsem and later fills area->pages
>> under domains_rwsem. This leaves a window between area creation and
>> area->pages fill where a concurrent iommufd_ioas_change_process()
>> can observe the area and dereference a NULL area->pages pointer,
>> leading to a NULL pointer dereference:
> AFAIU, the flow must be
>
> iopt_map_file_pages()
> ->iopt_map_common()
> ->iopt_map_pages() <- here it does create and write pages ..
>
>
> iommufd_ioas_change_process() <-the page pointer is being accessed..
>
> 1. Why IOCTL got triggered before setting the respective functionality ?
>
This bug was triggered during Syzkaller fuzzing, so iopt_map_file_pages() and iommufd_ioas_change_process()
can run concurrently. A normal user may not issue any IOCTL before setting the respective functionality.
> 2. Since here unlocked_ioctl() used which means driver should have manage locking as user space will not hold global Big Kernel Lock (BKL).
> We have pages->mutex - do you think we need it some where instead of "-EBUSY" return ? - if yes can you try and repro ?
>
> snip:
> *
> * The locking order is domains_rwsem -> iova_rwsem -> pages::mutex
> */
> struct io_pagetable {
>
> ---
>
> down_read(&iopt->domains_rwsem);
> rc = iopt_fill_domains_pages(pages_list);
> if (rc)
> goto out_unlock_domains;
>
> down_write(&iopt->iova_rwsem);
> list_for_each_entry(elm, pages_list, next) {
> /*
> * area->pages must be set inside the domains_rwsem to ensure
> * any newly added domains will get filled. Moves the reference
> * in from the list.
> */
> elm->area->pages = elm->pages; <- maybe page mutex to protect this ?
> elm->pages = NULL;
> elm->area = NULL;
> }
> up_write(&iopt->iova_rwsem);
> out_unlock_domains:
> up_read(&iopt->domains_rwsem);
> return rc;
>
Using pages->mutex would not work here. If area->pages is NULL, then there is no iopt_pages from which
iommufd_ioas_change_process() could obtain the mutex.
And access to area->pages is already serialized by iova_rwsem; the problem is that iopt_map_pages()
intentionally leaves a window where the area has been inserted but area->pages is still NULL.>
> Thanks,
>
> Tushar Nimkar
>
Best,
Peiyang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages
2026-08-06 4:50 [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages Peiyang He
2026-08-07 5:36 ` Tushar Nimkar
@ 2026-08-10 19:59 ` Jason Gunthorpe
2026-08-12 4:35 ` Peiyang He
1 sibling, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2026-08-10 19:59 UTC (permalink / raw)
To: Peiyang He
Cc: kevin.tian, jgg, joro, will, iommu, linux-kernel, robin.murphy,
steven.sistare, stable
> @@ -535,6 +535,10 @@ int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
> return rc;
>
> for_each_ioas_area(&ioas_list, index, ioas, area) {
> + if (!area->pages) {
> + rc = -EBUSY;
> + goto out;
> + }
> if (area->pages->type != IOPT_ADDRESS_FILE) {
> rc = -EINVAL;
> goto out;
If we do this then a concurrent map will get corrupted accounting. I think we
have to prevent map from progressing too. I belive the issue is we don't take
enough locks.
Does this fix it?
--- a/drivers/iommu/iommufd/ioas.c
+++ b/drivers/iommu/iommufd/ioas.c
@@ -383,6 +383,7 @@ static void iommufd_release_all_iova_rwsem(struct iommufd_ctx *ictx,
xa_for_each(ioas_list, index, ioas) {
up_write(&ioas->iopt.iova_rwsem);
+ up_write(&ioas->iopt.devices);
refcount_dec(&ioas->obj.users);
}
up_write(&ictx->ioas_creation_lock);
@@ -422,6 +423,8 @@ static int iommufd_take_all_iova_rwsem(struct iommufd_ctx *ictx,
xa_unlock(&ictx->objects);
ioas = container_of(obj, struct iommufd_ioas, obj);
+ down_write_nest_lock(&ioas->iopt.devices_rwsem,
+ &ictx->ioas_creation_lock);
down_write_nest_lock(&ioas->iopt.iova_rwsem,
&ictx->ioas_creation_lock);
--
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages
2026-08-10 19:59 ` Jason Gunthorpe
@ 2026-08-12 4:35 ` Peiyang He
2026-08-12 12:12 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Peiyang He @ 2026-08-12 4:35 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: kevin.tian, jgg, joro, will, iommu, linux-kernel, robin.murphy,
steven.sistare, stable
Thanks for your reply.
On 2026/8/11 03:59, Jason Gunthorpe wrote:
>> @@ -535,6 +535,10 @@ int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
>> return rc;
>>
>> for_each_ioas_area(&ioas_list, index, ioas, area) {
>> + if (!area->pages) {
>> + rc = -EBUSY;
>> + goto out;
>> + }
>> if (area->pages->type != IOPT_ADDRESS_FILE) {
>> rc = -EINVAL;
>> goto out;
>
> If we do this then a concurrent map will get corrupted accounting. I think we
From my understanding, if iommufd_ioas_change_process() returns with EBUSY when
detecting a NULL area->pages, later accounting logic in iommufd_ioas_change_process()
won't be executed. I may be missing something, feel free to point it out.
> have to prevent map from progressing too. I belive the issue is we don't take
> enough locks.
>
> Does this fix it?
>
> --- a/drivers/iommu/iommufd/ioas.c
> +++ b/drivers/iommu/iommufd/ioas.c
> @@ -383,6 +383,7 @@ static void iommufd_release_all_iova_rwsem(struct iommufd_ctx *ictx,
>
> xa_for_each(ioas_list, index, ioas) {
> up_write(&ioas->iopt.iova_rwsem);
> + up_write(&ioas->iopt.devices);
Should this be up_write(&ioas->iopt.domains_rwsem); ? It seems there isn't a devices field
in struct io_pagetable.> refcount_dec(&ioas->obj.users);
> }
> up_write(&ictx->ioas_creation_lock);
> @@ -422,6 +423,8 @@ static int iommufd_take_all_iova_rwsem(struct iommufd_ctx *ictx,
> xa_unlock(&ictx->objects);
>
> ioas = container_of(obj, struct iommufd_ioas, obj);
> + down_write_nest_lock(&ioas->iopt.devices_rwsem,
Should be down_write_nest_lock(&ioas->iopt.domains_rwsem, as well?> + &ictx->ioas_creation_lock);
> down_write_nest_lock(&ioas->iopt.iova_rwsem,
> &ictx->ioas_creation_lock);
>
Taking domains_rwsem in iommufd_take_all_iova_rwsem() alone may not prevent the NULL pointer deref.
For example:
change_process: take domains_rwsem for write
map: take iova_rwsem for write, insert an area with area->pages == NULL, and release iova_rwsem
map: try to take domains_rwsem for read and block
change_process: take iova_rwsem for write and iterate the area
At this point area->pages is still NULL. iopt_map_pages() takes domains_rwsem only after iopt_alloc_area_pages()
has inserted the NULL area->pages.
Best regards,
Peiyang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages
2026-08-12 4:35 ` Peiyang He
@ 2026-08-12 12:12 ` Jason Gunthorpe
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2026-08-12 12:12 UTC (permalink / raw)
To: Peiyang He
Cc: kevin.tian, joro, will, iommu, linux-kernel, robin.murphy,
steven.sistare, stable
On Wed, Aug 12, 2026 at 12:35:04PM +0800, Peiyang He wrote:
> On 2026/8/11 03:59, Jason Gunthorpe wrote:
> >> @@ -535,6 +535,10 @@ int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
> >> return rc;
> >>
> >> for_each_ioas_area(&ioas_list, index, ioas, area) {
> >> + if (!area->pages) {
> >> + rc = -EBUSY;
> >> + goto out;
> >> + }
> >> if (area->pages->type != IOPT_ADDRESS_FILE) {
> >> rc = -EINVAL;
> >> goto out;
> >
> > If we do this then a concurrent map will get corrupted accounting. I think we
>
> From my understanding, if iommufd_ioas_change_process() returns with EBUSY when
> detecting a NULL area->pages, later accounting logic in iommufd_ioas_change_process()
> won't be executed. I may be missing something, feel free to point it
> out.
The map side will be racing and it has already partially setup some of
its accounting, but I guess you are saying since the change process
aborts that is OK?
> Taking domains_rwsem in iommufd_take_all_iova_rwsem() alone may not prevent the NULL pointer deref.
> For example:
> change_process: take domains_rwsem for write
> map: take iova_rwsem for write, insert an area with area->pages == NULL, and release iova_rwsem
> map: try to take domains_rwsem for read and block
> change_process: take iova_rwsem for write and iterate the area
Yes, I guess it does unlock as it runs too bad
So this patch is probably the best option
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-08-12 12:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-06 4:50 [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages Peiyang He
2026-08-07 5:36 ` Tushar Nimkar
2026-08-07 8:13 ` Peiyang He
2026-08-10 19:59 ` Jason Gunthorpe
2026-08-12 4:35 ` Peiyang He
2026-08-12 12:12 ` Jason Gunthorpe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.