All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.