Re: [PATCH] iommu/iommufd: Fix NULL pointer deref in iommufd_ioas_change_process when racing with iopt_map_file_pages

[Peiyang He]

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@xxxxxxxxxxxxxxxx. 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