Re: [PATCH rc] iommu/amd: Fix geometry.aperture_end for V2 tables

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Jason,


On 4/24/2025 7:36 PM, Jason Gunthorpe wrote:
> On Thu, Apr 24, 2025 at 01:26:21PM +0530, Vasant Hegde wrote:
>> Hi Jason,
>>
>> On 4/17/2025 9:51 PM, Jason Gunthorpe wrote:
>>> The AMD IOMMU documentation seems pretty clear that the V2 table follows
>>> the normal CPU expectation of sign extension. This is shown in
>>>
>>>   Figure 25: AMD64 Long Mode 4-Kbyte Page Address Translation
>>>
>>> Where bits Sign-Extend [63:57] == [56]. This is typical for x86 which
>>> would have three regions in the page table: lower, non-canonical, upper.
>>>
>>> The manual describes that the V1 table does not sign extend in section
>>> 2.2.4 Sharing AMD64 Processor and IOMMU Page Tables GPA-to-SPA
>>>
>>> The iommu domain geometry does not directly support sign extended page
>>> tables. The driver should report only one of the lower/upper spaces. Solve
>>> this by removing the top VA bit from the geometry to use only the lower
>>> space.
>>>
>>> Adjust dma_max_address() to do this. It now returns:
>>>
>>> 5 Level:
>>>   Before 0x1ffffffffffffff
>>>   After  0x0ffffffffffffff
>>> 4 Level:
>>>   Before 0xffffffffffff
>>>   After  0x7fffffffffff
>>>
>>> Fixes: 11c439a19466 ("iommu/amd/pgtbl_v2: Fix domain max address")
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>  drivers/iommu/amd/iommu.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> AMD folks: I'm just reading the documentation, it would be good to confirm
>>> this understanding. I'm a bit surprised nobody hit this, but given the domain
>>> aperture was wildly wrong up till 2023 maybe it never gets exercised
>>> carefully.
>>
>> We have tested this with 4 and 5 level w/ some bench marks as well as w/
>> forcedac=1. It works fine. My understanding is IOMMU uses bit 56/47 as well for
>> address translation.
> 
> Yes, it should use bit 56 for address translation, that is part of the
> page table architecture.

I have checked with HW architects.

In DMA API mode (PASID=0), IOMMU HW does not use canonical addresses. It's safe
to use bit 56 (5 level page table) -OR- bit 47 (4 level page table) for address
translation (we don't need sign extension). However when PASID is *enabled*
(PASID != 0), then IOMMU expects canonical address bit[63-57] should match
bit[56]. Otherwise it will abort the request.

I have requested spec writer to add details in the spec.

-Vasant


> 
> The question is what happen if a device uses IOVA 0x0100000000000000
> with the iommu. This is a non-canonical address, so I think
> architecturally on x86 it should be rejected. I would not be surprised
> if some HW treats it the same as 0xFF00000000000000 - though that
> would be dangerous.
> 
> There is a significant correctness issue here with ATS, the IOMMU
> *must not* allow address aliases to exist, so if it responds to ATS
> queries at both 0x0100000000000000 and 0xFF00000000000000 with the
> same PTE then it is security broken. The device ATC is only flushed
> based on the canonical IOVA, so any aliases can remain in the ATC and
> trigger UAF issues. This can possibly be triggered by userspace when
> using VFIO :\
> 
> So the question is not about if bit 56/47 is used, but if the IOMMU hw
> is validating the sign extension. Assuming it is validating then we
> must not tell the iommu core code to use
>  0x0100000000000000 -> 0x01FFFFFFFFFFFFFF
> as IOVA since it is not legal IOVA. This is why the bit width is
> reduced by one when computing the aperture.
> 
> Given the security sensistivity with ATS the sign validation behavior
> should be understood because I do plan to come with a patch to enable
> the high address space for iommufd and if some AMD implementations
> need to block that we should know :)
> 
> Thanks,
> Jason