linux-kernel - Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ae778a81-a500-d8d2-5c7e-10630fe42734@amd.com>
Date:   Wed, 13 Dec 2017 10:41:47 -0600
From:   "Hook, Gary" <ghook@....com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        Peter Xu <peterx@...hat.com>
Cc:     iommu@...ts.linux-foundation.org, dwmw2@...radead.org,
        linux-kernel@...r.kernel.org, tursulin@...ulin.net
Subject: Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb

On 12/13/2017 9:58 AM, Alex Williamson wrote:
> On Wed, 13 Dec 2017 15:13:55 +0800
> Peter Xu <peterx@...hat.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?