RE: [PATCH 1/1] iommu/vt-d: Fix aligned pages in calculate_psi_aligned_address()

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Tuesday, July 9, 2024 10:54 AM
> 
> On 7/8/24 8:14 PM, Lu Baolu wrote:
> > The helper calculate_psi_aligned_address() is used to convert an arbitrary
> > range into a size-aligned one.
> >
> > The aligned_pages variable is calculated from input start and end, but is
> > not adjusted when the start pfn is not aligned and the mask is adjusted,
> > which results in an incorrect number of pages returned.
> >
> > The number of pages is used by qi_flush_piotlb() to flush caches for the
> > first-stage translation. With the wrong number of pages, the cache is not
> > synchronized, leading to inconsistencies in some cases.
> >
> > Fixes: c4d27ffaa8eb ("iommu/vt-d: Add cache tag invalidation helpers")
> > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/intel/cache.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
> > index e8418cdd8331..113834742107 100644
> > --- a/drivers/iommu/intel/cache.c
> > +++ b/drivers/iommu/intel/cache.c
> > @@ -246,6 +246,7 @@ static unsigned long
> calculate_psi_aligned_address(unsigned long start,
> >   		 */
> >   		shared_bits = ~(pfn ^ end_pfn) & ~bitmask;
> >   		mask = shared_bits ? __ffs(shared_bits) : BITS_PER_LONG;
> > +		aligned_pages = 1UL << mask;
> 
> Hmm, it appears that if mask is equal to BITS_PER_LONG (which is
> typically 64), the left shift operation will overflow.
> 
> So perhaps we need another line of change:
> 
> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
> index 113834742107..44e92638c0cd 100644
> --- a/drivers/iommu/intel/cache.c
> +++ b/drivers/iommu/intel/cache.c
> @@ -245,7 +245,7 @@ static unsigned long
> calculate_psi_aligned_address(unsigned long start,
>                   * shared_bits are all equal in both pfn and end_pfn.
>                   */
>                  shared_bits = ~(pfn ^ end_pfn) & ~bitmask;
> -               mask = shared_bits ? __ffs(shared_bits) : BITS_PER_LONG;
> +               mask = shared_bits ? __ffs(shared_bits) :
> MAX_AGAW_PFN_WIDTH;
>                  aligned_pages = 1UL << mask;
>          }
> 
> I will make above another fix as it already causes overflow in another
> path.
> 
> Kevin, sound good to you?
> 

yes. for both:

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>