Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb

[Hook, Gary]

On 12/13/2017 9:58 AM, Alex Williamson wrote:
> On Wed, 13 Dec 2017 15:13:55 +0800
> Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> > On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
> >
> > [...]
> >
> > > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> > > index 9a7ffd13c7f0..87888b102057 100644
> > > --- a/drivers/iommu/dmar.c
> > > +++ b/drivers/iommu/dmar.c
> > > @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
> > >   	struct qi_desc desc;
> > >   
> > >   	if (mask) {
> > > -		BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
> > > +		BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
> > > +		       ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
> > > +		       (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
> >
> > Could it work if we just use 1ULL instead of 1 here?  Thanks,
>
> In either case we're talking about shifting off the end of the
> variable, which I understand to be undefined.  Right?  Thanks,

How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB) 
end. I believe that behavior is pretty set.

The only problem here is that the promotion of integral types is (at the 
very least) unclear in this statement. As for the proposal, do we know 
that 1ULL is always going to be the same size as addr?

I would suggest, as pedantic as it sounds, creating a local u64 
variable, initialized to 1, and using that in the BUG_ON:

	u64 ubit = 1;

	...

 	if (mask) {
		BUG_ON(addr & ((ubit << (VTD_PAGE_SHIFT + mask)) - 1));

I believe this forces everything to be appropriately wide, and the same 
size?