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>
---
Changes since v1:
 - Return all the PTE bits except for the contiguous count ones.
---
 xen/drivers/passthrough/vtd/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 130a159cde07..d7ba9a9c349f 100644
--- 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) +
                     (addr & ((1UL << level_to_offset_bits(level)) - 1) &
                      PAGE_MASK);
                 if ( !target )
-- 
2.40.0

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

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

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.