Re: [PATCH 1/5] iommu/vt-d: Fix shift overflow in qi_desc_dev_iotlb_pasid()

On Fri, Jul 31, 2026 at 01:43:25PM +0800, Lu Baolu wrote:
>Callers request a full Device-TLB flush by passing MAX_AGAW_PFN_WIDTH
>(64 - VTD_PAGE_SHIFT == 52) as @size_order.  Two shifts in
>qi_desc_dev_iotlb_pasid() are not prepared for a value that large:
>
>  unsigned long mask = 1UL << (VTD_PAGE_SHIFT + size_order - 1);
>  ...
>  if (!IS_ALIGNED(addr, VTD_PAGE_SIZE << size_order))
>
>The first evaluates to 1UL << 63.  On 32-bit builds this is undefined
>behaviour; in practice x86 masks the shift count to 5 bits, so the
>expression yields 1UL << 31 and ~mask becomes 0x7fffffff.  That value is
>zero-extended when it is applied to the 64-bit descriptor, so
>
>  desc->qw1 &= ~mask;
>
>clears qw1[63:32] as well as bit 31.  The ADDR field, which had just been
>filled with ones to request the widest possible range, collapses to
>0x7ffff000.  As the S bit remains set, hardware decodes the least
>significant zero bit of ADDR and invalidates only 2GiB instead of the
>entire address space.  Device-TLB entries above that boundary survive the
>unmap, leaving an ATS-capable device able to keep accessing memory that
>has already been freed.
>
>The second shift, VTD_PAGE_SIZE << size_order, is 1UL << 64 and is
>therefore undefined on 64-bit builds too.  On x86_64 the shift count
>masks to zero, IS_ALIGNED(addr, 1) is trivially true and the sanity check
>silently degrades into a no-op.
>
>Compute both quantities in 64-bit and clamp @size_order to the largest
>range the ADDR field can encode.  Capping at 63 - VTD_PAGE_SHIFT keeps
>the intended "flush everything" behaviour: qw1[62:12] is set, bit 62 is
>cleared as the size indicator and the S bit is set.  The non-PASID
>variant qi_desc_dev_iotlb() already uses 1ULL and is unaffected.
>
>Fixes: f701c9f36bcb7 ("iommu/vt-d: Factor out invalidation descriptor composition")
>Cc: [email protected]
>Reported-by: Sashiko <[email protected]>
>Closes: https://sashiko.dev/#/patchset/20260623060122.3796325-1-guanghuifeng%40linux.alibaba.com
>Assisted-by: Claude:claude-opus-5
>Signed-off-by: Lu Baolu <[email protected]>
>---
> drivers/iommu/intel/iommu.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>index c00f44db0020..8a59c7c9d0a6 100644
>--- a/drivers/iommu/intel/iommu.h
>+++ b/drivers/iommu/intel/iommu.h
>@@ -1105,12 +1105,20 @@ static inline void qi_desc_dev_iotlb_pasid(u16 sid, u16 pfsid, u32 pasid,
> 					   unsigned int size_order,
> 					   struct qi_desc *desc)
> {
>-	unsigned long mask = 1UL << (VTD_PAGE_SHIFT + size_order - 1);
>-
> 	desc->qw0 = QI_DEV_EIOTLB_PASID(pasid) | QI_DEV_EIOTLB_SID(sid) |
> 		QI_DEV_EIOTLB_QDEP(qdep) | QI_DEIOTLB_TYPE |
> 		QI_DEV_IOTLB_PFSID(pfsid);
>
>+	/*
>+	 * The invalidation range is encoded in the ADDR field, which only
>+	 * covers bits 63:12.  Clamp @size_order so that callers asking for a
>+	 * full flush (e.g. with MAX_AGAW_PFN_WIDTH) do not overflow the
>+	 * shifts below.  The clamped value still spans the whole range that
>+	 * the descriptor is able to express.
>+	 */
>+	if (size_order > 63 - VTD_PAGE_SHIFT)
>+		size_order = 63 - VTD_PAGE_SHIFT;
>+
> 	/*
> 	 * If S bit is 0, we only flush a single page. If S bit is set,
> 	 * The least significant zero bit indicates the invalidation address
>@@ -1120,7 +1128,7 @@ static inline void qi_desc_dev_iotlb_pasid(u16 sid, u16 pfsid, u32 pasid,
> 	 * Max Invs Pending (MIP) is set to 0 for now until we have DIT in
> 	 * ECAP.
> 	 */
>-	if (!IS_ALIGNED(addr, VTD_PAGE_SIZE << size_order))
>+	if (!IS_ALIGNED(addr, BIT_ULL(VTD_PAGE_SHIFT + size_order)))
> 		pr_warn_ratelimited("Invalidate non-aligned address %llx, order %d\n",
> 				    addr, size_order);
>
>@@ -1136,7 +1144,7 @@ static inline void qi_desc_dev_iotlb_pasid(u16 sid, u16 pfsid, u32 pasid,
> 		desc->qw1 |= GENMASK_ULL(size_order + VTD_PAGE_SHIFT - 1,
> 					VTD_PAGE_SHIFT);
> 		/* Clear size_order bit to indicate size */
>-		desc->qw1 &= ~mask;
>+		desc->qw1 &= ~BIT_ULL(VTD_PAGE_SHIFT + size_order - 1);
> 		/* Set the S bit to indicate flushing more than 1 page */
> 		desc->qw1 |= QI_DEV_EIOTLB_SIZE;
> 	}
>-- 
>2.43.0
>
>

Reviewed-by: Samiullah Khawaja <[email protected]>