lkml.org 
[lkml]   [2017]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
From
Date
On 12/13/2017 9:58 AM, Alex Williamson wrote:
> On Wed, 13 Dec 2017 15:13:55 +0800
> Peter Xu <peterx@redhat.com> 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?

\
 
 \ /
  Last update: 2017-12-13 17:42    [W:0.071 / U:1.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site