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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, May 24, 2023 at 06:11:03PM +0200, Jan Beulich wrote:
> 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@citrix.com>
> 
> 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)?

Gah no, i did assert that the iterations are OK, but completely forgot
about the r/w bits.

> 
> > --- 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)?

Right, I've just masked ~DMA_PTE_CONTIG_MASK like it's done below when
splitting a superpage, but for the use case here it does make more
sense to use PADDR_MASK.

Thanks, Roger.