Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753749AbdLMRP0 (ORCPT ); Wed, 13 Dec 2017 12:15:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39024 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752816AbdLMRPW (ORCPT ); Wed, 13 Dec 2017 12:15:22 -0500 Date: Wed, 13 Dec 2017 10:15:18 -0700 From: Alex Williamson To: "Hook, Gary" Cc: Peter Xu , iommu@lists.linux-foundation.org, dwmw2@infradead.org, linux-kernel@vger.kernel.org, tursulin@ursulin.net Subject: Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb Message-ID: <20171213101518.7807ae4b@t450s.home> In-Reply-To: References: <20171212224250.22173.74201.stgit@gimli.home> <20171213071355.GD7780@xz-mi> <20171213085847.3d3245c6@t450s.home> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 13 Dec 2017 17:15:22 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1396 Lines: 38 On Wed, 13 Dec 2017 10:41:47 -0600 "Hook, Gary" wrote: > On 12/13/2017 9:58 AM, Alex Williamson wrote: > > On Wed, 13 Dec 2017 15:13:55 +0800 > > Peter Xu 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. Maybe I'm relying too much on stackoverflow, but: https://stackoverflow.com/questions/11270492/what-does-the-c-standard-say-about-bitshifting-more-bits-than-the-width-of-type Thanks, Alex