Re: [PATCH] iommu/iommufd: Require write access for writable MAP_FILE mappings

Re: [PATCH] iommu/iommufd: Require write access for writable MAP_FILE mappings

[Jason Gunthorpe]

On Sun, Jun 07, 2026 at 08:53:18AM +0000, Yiming Qian wrote:
> IOMMU_IOAS_MAP_FILE pins folios from a shmem/tmpfs or hugetlb file and
> uses them as the backing storage for an IOAS mapping.  When userspace sets
> IOMMU_IOAS_MAP_WRITEABLE, the resulting IOMMU PTEs allow DMA writes to the
> file-backed folios.

This looks like an issue with the API design in memfd_pin_folios(),
all users would have a similar bug I think.

I don't know much about memfd but this seems like a legitimate issue.

Add those involved with gup.c and the patch adding memfd_pin_folios()

>  {
>  	struct iopt_pages *pages;
> +	int rc;
> +
> +	if (writable) {
> +		if (!(file->f_mode & FMODE_WRITE))
> +			return ERR_PTR(-EPERM);
> +
> +		rc = mapping_map_writable(file->f_mapping);
> +		if (rc)
> +			return ERR_PTR(rc);
> +	}

We probably need some kind of companion API for memfd_pin_folios(), a
start/pin/destroy kind of thing to manage this?

It should not be open coded like this.

Jason

Re: [PATCH] iommu/iommufd: Require write access for writable MAP_FILE mappings

[David Hildenbrand (Arm)]

On 6/7/26 14:09, Jason Gunthorpe wrote:
> On Sun, Jun 07, 2026 at 08:53:18AM +0000, Yiming Qian wrote:
>> IOMMU_IOAS_MAP_FILE pins folios from a shmem/tmpfs or hugetlb file and
>> uses them as the backing storage for an IOAS mapping.  When userspace sets
>> IOMMU_IOAS_MAP_WRITEABLE, the resulting IOMMU PTEs allow DMA writes to the
>> file-backed folios.
> 
> This looks like an issue with the API design in memfd_pin_folios(),
> all users would have a similar bug I think.

Agreed.

Not sure if it should be part of memfd_pin_folios() itself.

> 
> I don't know much about memfd but this seems like a legitimate issue.
> 
> Add those involved with gup.c and the patch adding memfd_pin_folios()
> 
>>  {
>>  	struct iopt_pages *pages;
>> +	int rc;
>> +
>> +	if (writable) {
>> +		if (!(file->f_mode & FMODE_WRITE))
>> +			return ERR_PTR(-EPERM);
>> +
>> +		rc = mapping_map_writable(file->f_mapping);
>> +		if (rc)
>> +			return ERR_PTR(rc);
>> +	}
> 
> We probably need some kind of companion API for memfd_pin_folios(), a
> start/pin/destroy kind of thing to manage this?
> 
> It should not be open coded like this.

The permission check is one thing that's clearly missing.

Not sure about the mapping_map_writable() handling ... it's weird to rely on
that when we are not actually mmaping.

Assume we GUP a page and then munmap, mapping_unmap_writable() would be called
while we still have a writable GUP reference. Hm.

-- 
Cheers,

David

Re: [PATCH] iommu/iommufd: Require write access for writable MAP_FILE mappings

[Jason Gunthorpe]

On Mon, Jun 08, 2026 at 03:38:00PM +0200, David Hildenbrand (Arm) wrote:
> On 6/7/26 14:09, Jason Gunthorpe wrote:
> > On Sun, Jun 07, 2026 at 08:53:18AM +0000, Yiming Qian wrote:
> >> IOMMU_IOAS_MAP_FILE pins folios from a shmem/tmpfs or hugetlb file and
> >> uses them as the backing storage for an IOAS mapping.  When userspace sets
> >> IOMMU_IOAS_MAP_WRITEABLE, the resulting IOMMU PTEs allow DMA writes to the
> >> file-backed folios.
> > 
> > This looks like an issue with the API design in memfd_pin_folios(),
> > all users would have a similar bug I think.
> 
> Agreed.
> 
> Not sure if it should be part of memfd_pin_folios() itself.

I think it should, drivers should not be open coding this.

If there is such a thing as a read-only memfd then memfd_pin_folios()
should accept the usual FOLL_WRITE and deal with it internally.

> > start/pin/destroy kind of thing to manage this?
> > 
> > It should not be open coded like this.
> 
> The permission check is one thing that's clearly missing.
> 
> Not sure about the mapping_map_writable() handling ... it's weird to rely on
> that when we are not actually mmaping.

I don't know anything about sealing, but it shouldn't something check
if it is sealed read-only ?

> Assume we GUP a page and then munmap, mapping_unmap_writable() would be called
> while we still have a writable GUP reference. Hm.

I suspect the user doing the sealing has to ensure there are no pins
to the memfd before it seals it. If it already let the unsealed memfd
out of its control then it probably cannot be reliably sealed read
only?

Jason
 

Re: [PATCH] iommu/iommufd: Require write access for writable MAP_FILE mappings

[David Hildenbrand (Arm)]

On 6/8/26 15:46, Jason Gunthorpe wrote:
> On Mon, Jun 08, 2026 at 03:38:00PM +0200, David Hildenbrand (Arm) wrote:
>> On 6/7/26 14:09, Jason Gunthorpe wrote:
>>>
>>> This looks like an issue with the API design in memfd_pin_folios(),
>>> all users would have a similar bug I think.
>>
>> Agreed.
>>
>> Not sure if it should be part of memfd_pin_folios() itself.
> 
> I think it should, drivers should not be open coding this.
> 
> If there is such a thing as a read-only memfd then memfd_pin_folios()
> should accept the usual FOLL_WRITE and deal with it internally.

I'd rather not want to use FOLL flags (FOLLOW PAGE leftovers, to be renamed to
GUP_ on some lucky day) on this interface.

But sure, we could add a new set of flags (single flag for now).

> 
>>> start/pin/destroy kind of thing to manage this?
>>>
>>> It should not be open coded like this.
>>
>> The permission check is one thing that's clearly missing.
>>
>> Not sure about the mapping_map_writable() handling ... it's weird to rely on
>> that when we are not actually mmaping.
> 
> I don't know anything about sealing, but it shouldn't something check
> if it is sealed read-only ?

Right. We could do mapping_map_writable() over the duration of the function I
guess if we care about concurrent races.

Alternative have a new helper that makes sure that mapping->i_mmap_writable is
not negative (write denied).

> 
>> Assume we GUP a page and then munmap, mapping_unmap_writable() would be called
>> while we still have a writable GUP reference. Hm.
> 
> I suspect the user doing the sealing has to ensure there are no pins
> to the memfd before it seals it. If it already let the unsealed memfd
> out of its control then it probably cannot be reliably sealed read
> only?

Yes, that's my assumption. writable pins derived pre-sealing stay valid.


-- 
Cheers,

David