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]

Message-ID: <947b02eb-536f-16a0-fbb1-87b62ab8c13e@citrix.com>
Date:   Thu, 26 Nov 2020 17:16:33 +0000
From:   Andrew Cooper <andrew.cooper3@...rix.com>
To:     Lukas Bulwahn <lukas.bulwahn@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        <x86@...nel.org>, Akinobu Mita <akinobu.mita@...il.com>,
        Joerg Roedel <jroedel@...e.de>
CC:     "H . Peter Anvin" <hpa@...or.com>,
        <kernel-janitors@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/cpu: correct values for GDT_ENTRY_INIT

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?

Changing the limit from 4G to 128M isn't going to be compatible with a
32bit kernel trying to boot :).

~Andrew