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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 15 Dec 2017 15:29:57 +0800
From:   Peter Xu <peterx@...hat.com>
To:     "Hook, Gary" <ghook@....com>
Cc:     Alex Williamson <alex.williamson@...hat.com>,
        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 Wed, Dec 13, 2017 at 11:31:02AM -0600, Hook, Gary wrote:
> On 12/13/2017 11:15 AM, Alex Williamson wrote:
> > On Wed, 13 Dec 2017 10:41:47 -0600
> > "Hook, Gary" <ghook@....com> wrote:
> > 
> > > 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.
> > 
> > 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
> 
> No, probably not. I don't have my copy of c99 handy, so can't check it. But
> it is beyond me why any compiler implementation would choose to use a rotate
> instead of a shift... probably a performance issue.
> 
> So, yeah, when you have silly parameters, you get what you get.
> 
> I'll stick to my suggestion. Which seems unambiguous... but I could be
> wrong.

Hi, Alex, Hook,

I did a quick test:

xz-mi:tmp $ cat a.c
#include <stdio.h>

void main(void)
{
    unsigned long long val = 0x8000000000000001ULL;
    int shift; 

    printf("origin: 0x%llx\n", val);
    shift = 1; printf("shl 1: 0x%llx\n", val << shift);
    shift = 62; printf("shl 62: 0x%llx\n", val << shift);
    shift = 63; printf("shl 63: 0x%llx\n", val << shift);
    shift = 64; printf("shl 64: 0x%llx\n", val << shift);
    shift = 65; printf("shl 65: 0x%llx\n", val << shift);
}
xz-mi:tmp $ gcc -std=c99 a.c
xz-mi:tmp $ ./a.out 
origin: 0x8000000000000001
shl 1: 0x2
shl 62: 0x4000000000000000
shl 63: 0x8000000000000000
shl 64: 0x8000000000000001
shl 65: 0x2
xz-mi:tmp $ objdump -d a.out | grep -A20 "<main>"
00000000004004d7 <main>:
  4004d7:       55                      push   %rbp
  4004d8:       48 89 e5                mov    %rsp,%rbp
  4004db:       48 83 ec 10             sub    $0x10,%rsp
  4004df:       48 b8 01 00 00 00 00    movabs $0x8000000000000001,%rax
  4004e6:       00 00 80 
  4004e9:       48 89 45 f8             mov    %rax,-0x8(%rbp)
  4004ed:       48 8b 45 f8             mov    -0x8(%rbp),%rax
  4004f1:       48 89 c6                mov    %rax,%rsi
  4004f4:       bf 60 06 40 00          mov    $0x400660,%edi
  4004f9:       b8 00 00 00 00          mov    $0x0,%eax
  4004fe:       e8 ed fe ff ff          callq  4003f0 <printf@plt>
  400503:       c7 45 f4 01 00 00 00    movl   $0x1,-0xc(%rbp)
  40050a:       8b 45 f4                mov    -0xc(%rbp),%eax
  40050d:       48 8b 55 f8             mov    -0x8(%rbp),%rdx
  400511:       89 c1                   mov    %eax,%ecx
  400513:       48 d3 e2                shl    %cl,%rdx
  400516:       48 89 d0                mov    %rdx,%rax
  400519:       48 89 c6                mov    %rax,%rsi
  40051c:       bf 70 06 40 00          mov    $0x400670,%edi
  400521:       b8 00 00 00 00          mov    $0x0,%eax

So it seems not really a rotation operation, but it looks more like
convering a "shifting N" into "shifting N%64" when N>=64.  So now I
agree with Alex's change.  Thanks all.

-- 
Peter Xu