linux-kernel - Re: [PATCH] x86/cpu: correct values for GDT_ENTRY

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]

Date:   Thu, 26 Nov 2020 11:15:54 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc:     Andrew Cooper <andrew.cooper3@...rix.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        X86 ML <x86@...nel.org>, Akinobu Mita <akinobu.mita@...il.com>,
        Joerg Roedel <jroedel@...e.de>,
        "H . Peter Anvin" <hpa@...or.com>, kernel-janitors@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/cpu: correct values for GDT_ENTRY_INIT

On Thu, Nov 26, 2020 at 11:07 AM Lukas Bulwahn <lukas.bulwahn@...il.com> wrote:
>
> On Thu, Nov 26, 2020 at 6:16 PM Andrew Cooper <andrew.cooper3@...rix.com> wrote:
> >
> > On 26/11/2020 11:54, Lukas Bulwahn wrote:
> > > Commit 1e5de18278e6 ("x86: Introduce GDT_ENTRY_INIT()") unintentionally
> > > transformed a few 0xffff values to 0xfffff (note: five times "f" instead of
> > > four) as part of the refactoring.
> >
> > The transformation in that change is correct.
> >
> > Segment bases are 20 bits wide in x86, but the top nibble is folded into
> > the middle of the attributes, which is why the transformation also has
> > xfxx => x0xx for the attributes field.
> >
> > >
> > > A quick check with:
> > >
> > >   git show 1e5de18278e6 | grep "fffff"
> > >
> > > reveals all those 14 occurrences:
> > >
> > >     12 in ./arch/x86/kernel/cpu/common.c, and
> > >     2  in ./arch/x86/include/asm/lguest.h.
> > >
> > > The two occurrences in ./arch/x86/include/asm/lguest.h were deleted with
> > > commit ecda85e70277 ("x86/lguest: Remove lguest support").
> > > Correct the remaining twelve occurrences in ./arch/x86/kernel/cpu/common.c
> > > back to the original values in the source code before the refactoring.
> > >
> > > Commit 866b556efa12 ("x86/head/64: Install startup GDT") probably simply
> > > copied the required startup gdt information from
> > > ./arch/x86/kernel/cpu/common.c to ./arch/x86/kernel/head64.c.
> > > So, correct those three occurrences in ./arch/x86/kernel/head64.c as well.
> > >
> > > As this value is truncated anyway, the object code has not changed when
> > > introducing the mistake and is also not changed with this correction now.
> > >
> > > This was discovered with sparse, which warns with:
> > >
> > >   warning: cast truncates bits from constant value (fffff becomes ffff)
> >
> > Does:
> >
> > diff --git a/arch/x86/include/asm/desc_defs.h
> > b/arch/x86/include/asm/desc_defs.h
> > index f7e7099af595..9561f3c66e9e 100644
> > --- a/arch/x86/include/asm/desc_defs.h
> > +++ b/arch/x86/include/asm/desc_defs.h
> > @@ -22,7 +22,7 @@ struct desc_struct {
> >
> >  #define GDT_ENTRY_INIT(flags, base, limit)                     \
> >         {                                                       \
> > -               .limit0         = (u16) (limit),                \
> > +               .limit0         = (u16) (limit) & 0xFFFF,       \
> >                 .limit1         = ((limit) >> 16) & 0x0F,       \
> >                 .base0          = (u16) (base),                 \
> >                 .base1          = ((base) >> 16) & 0xFF,        \
> >
> > fix the warning?
> >
>
> Thanks, I will try that out, and try compiling a 32-bit kernel as well.

You should also try comparing the objdump output before and after your
patch.  objdump -D will produce bizarre output but should work.