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

On 8/4/2026 10:37 AM, 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]>
> Reviewed-by: Samiullah Khawaja <[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;
> +

Sashiko reported a critical issue with this change. I will drop this
patch from the series and spend more time investigating and reworking
the fix.

https://sashiko.dev/#/patchset/20260804023714.3080506-1-baolu.lu%40linux.intel.com

>   	/*
>   	 * 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;
>   	}

Thanks,
baolu