linux-kernel - Re: [PATCH] kvm: x86: mmu: Add cast to negated bitmasks in update_permission

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: <c852072704e82ac5342f344c5862d20ce5559c21.camel@perches.com>
Date:   Fri, 15 Jun 2018 11:40:25 -0700
From:   Joe Perches <joe@...ches.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>, pbonzini@...hat.com,
        rkrcmar@...hat.com, Thomas Gleixner <tglx@...utronix.de>,
        hpa@...or.com, x86@...nel.org, kvm@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kvm: x86: mmu: Add cast to negated bitmasks in
 update_permission_bitmask()

On Fri, 2018-06-15 at 11:29 -0700, Matthias Kaehlcke wrote:
> On Fri, Jun 15, 2018 at 11:18:12AM -0700, Joe Perches wrote:
> > On Fri, 2018-06-15 at 11:04 -0700, Nick Desaulniers wrote:
> > > On Fri, Jun 15, 2018 at 10:47 AM Matthias Kaehlcke <mka@...omium.org> wrote:
> > > > 
> > > > update_permission_bitmask() negates u8 bitmask values and assigns them
> > > > to variables of type u8. Since the MSB is set in the bitmask values the
> > > > compiler expands the negated values to int, which then are assigned to
> > > > u8 variables. Cast the negated values back to u8.
> > > > 
> > > > This fixes several warnings like this when building with clang:
> > > > 
> > > > arch/x86/kvm/mmu.c:4266:39: error: implicit conversion from 'int' to 'u8'
> > > >   (aka 'unsigned char') changes value from -205 to 51 [-Werror,
> > > >   -Wconstant-conversion]
> > > >     u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> > > >        ~~                               ^~
> > > > 
> > > > (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> > > > doesn't seem to be universally enabled)
> > 
> > Perhaps it's better to turn off the warning.
> > There are more of these in the kernel too.
> > 
> > At least:
> > drivers/regulator/max8660.c:	u8 mask = (rdev_get_id(rdev) == MAX8660_V3) ? ~1 : ~4;
> > drivers/regulator/max8660.c:	u8 mask = (rdev_get_id(rdev) == MAX8660_V6) ? ~2 : ~4;
> > fs/ext4/resize.c:	__u16 uninit_mask = (flexbg_size > 1) ? ~EXT4_BG_BLOCK_UNINIT : ~0;
> 
> In my experience neither clang nor gcc should promote these values to
> int, since the MSB/sign bit is not set.

Well, that is what the c90 standard requires.

6.5.3.3 Unary arithmetic operators
Constraints
4 The result of the ~ operator is the bitwise complement of its (promoted) operand (that is,
each bit in the result is set if and only if the corresponding bit in the converted operand is
not set). The integer promotions are performed on the operand, and the result has the
promoted type. If the promoted type is an unsigned type, the expression ~E is equivalent
to the maximum value representable in that type minus E.

> In any case I think it it preferable to fix the code over disabling
> the warning, unless the warning is bogus or there are just too many
> occurrences.

Maybe.