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/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