Re: [PATCH v2] iommu/vtd: fix address translation for superpages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2023 17:22, Roger Pau Monne wrote:
> When translating an address that falls inside of a superpage in the
> IOMMU page tables the fetching of the PTE value wasn't masking of the
> contiguous related data, which caused the returned data to be
> corrupt as it would contain bits that the caller would interpret as
> part of the address.
> 
> Fix this by masking off the contiguous bits.
> 
> Fixes: c71e55501a61 ('VT-d: have callers specify the target level for page 
> table walks')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just to clarify: The title says superpages and you also only deal with
superpages. Yet in the earlier discussion I pointed out that the 4k-page
case looks to also be flawed (I don't think anymore we iterate one too
many times, but I'm pretty sure the r/w flags are missing in what we
return to intel_iommu_lookup_page()). Did you convince yourself
otherwise in the meantime? Or is that going to be a separate change
(whether by you or someone else, like me)?

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -368,7 +368,7 @@ static uint64_t addr_to_dma_page_maddr(struct domain 
> *domain, daddr_t addr,
>                   * with the address adjusted to account for the residual of
>                   * the walk.
>                   */
> -                pte_maddr = pte->val +
> +                pte_maddr = (pte->val & ~DMA_PTE_CONTIG_MASK) +

While this addresses the problem at hand, wouldn't masking by PADDR_MASK
be more forward compatible (for whenever another of the high bits gets
used)?

Jan

>                      (addr & ((1UL << level_to_offset_bits(level)) - 1) &
>                       PAGE_MASK);
>                  if ( !target )