Re: [PATCH v2 1/2] x86/stackprotector/32: Make the canary into a regular percpu variable

From: Ferry Toth
Date: Thu Nov 17 2022 - 14:28:40 EST

Next message: Bjorn Helgaas: "Re: [PATCH 7/9] PCI: qcom: add support for IPQ8074 Gen3 port"
Previous message: Isaku Yamahata: "Re: [PATCH v10 049/108] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi

Op 14-11-2022 om 23:16 schreef Ferry Toth:

Hi

Op 14-11-2022 om 22:43 schreef Brian Gerst:

On Thu, Nov 10, 2022 at 2:36 PM Ferry Toth <fntoth@xxxxxxxxx> wrote:

Hi,

Op 09-11-2022 om 23:33 schreef Brian Gerst:

On Wed, Nov 9, 2022 at 9:50 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:

On Fri, Sep 30, 2022 at 11:18:51PM +0200, Ferry Toth wrote:

Op 30-09-2022 om 22:30 schreef Ferry Toth:

Op 29-09-2022 om 16:20 schreef Andy Shevchenko:

On Thu, Sep 29, 2022 at 04:56:07PM +0300, Andy Shevchenko wrote:

On Sat, Feb 13, 2021 at 11:19:44AM -0800, Andy Lutomirski wrote:

On 32-bit kernels, the stackprotector canary is quite nasty -- it is
stored at %gs:(20), which is nasty because 32-bit kernels use %fs for
percpu storage. It's even nastier because it means that whether %gs
contains userspace state or kernel state while running kernel code
depends on whether stackprotector is enabled (this is
CONFIG_X86_32_LAZY_GS), and this setting radically changes the way
that segment selectors work. Supporting both variants is a
maintenance and testing mess.

Merely rearranging so that percpu and the stack canary
share the same segment would be messy as the 32-bit percpu address
layout isn't currently compatible with putting a variable at a fixed
offset.

Fortunately, GCC 8.1 added options that allow the stack canary to be
accessed as %fs:__stack_chk_guard, effectively turning it
into an ordinary
percpu variable. This lets us get rid of all of the code to
manage the
stack canary GDT descriptor and the CONFIG_X86_32_LAZY_GS mess.

(That name is special. We could use any symbol we want for the
    %fs-relative mode, but for CONFIG_SMP=n, gcc refuses to
let us use any
    name other than __stack_chk_guard.)

This patch forcibly disables stackprotector on older compilers that
don't support the new options and makes the stack canary into a
percpu variable. The "lazy GS" approach is now used for all 32-bit
configurations.

This patch also makes load_gs_index() work on 32-bit kernels. On
64-bit kernels, it loads the GS selector and updates the user
GSBASE accordingly. (This is unchanged.) On 32-bit kernels,
it loads the GS selector and updates GSBASE, which is now
always the user base. This means that the overall effect is
the same on 32-bit and 64-bit, which avoids some ifdeffery.

This patch broke 32-bit boot on Intel Merrifield

git bisect start
# good: [9f4ad9e425a1d3b6a34617b8ea226d56a119a717] Linux 5.12
git bisect good 9f4ad9e425a1d3b6a34617b8ea226d56a119a717
# bad: [62fb9874f5da54fdb243003b386128037319b219] Linux 5.13
git bisect bad 62fb9874f5da54fdb243003b386128037319b219
# bad: [85f3f17b5db2dd9f8a094a0ddc665555135afd22] Merge branch
'md-fixes' of
https://git.kernel.org/pub/scm/linux/kernel/git/song/md into
block-5.13
git bisect bad 85f3f17b5db2dd9f8a094a0ddc665555135afd22
# good: [ca62e9090d229926f43f20291bb44d67897baab7] Merge tag
'regulator-v5.13' of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator
git bisect good ca62e9090d229926f43f20291bb44d67897baab7
# bad: [68a32ba14177d4a21c4a9a941cf1d7aea86d436f] Merge tag
'drm-next-2021-04-28' of git://anongit.freedesktop.org/drm/drm
git bisect bad 68a32ba14177d4a21c4a9a941cf1d7aea86d436f
# good: [49c70ece54b0d1c51bc31b2b0c1070777c992c26]
drm/amd/display: Change input parameter for set_drr
git bisect good 49c70ece54b0d1c51bc31b2b0c1070777c992c26
# good: [0b276e470a4d43e1365d3eb53c608a3d208cabd4] media: coda:
fix macroblocks count control usage
git bisect good 0b276e470a4d43e1365d3eb53c608a3d208cabd4
# bad: [c6536676c7fe3f572ba55842e59c3c71c01e7fb3] Merge tag
'x86_core_for_v5.13' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad c6536676c7fe3f572ba55842e59c3c71c01e7fb3
# good: [d1466bc583a81830cef2399a4b8a514398351b40] Merge branch
'work.inode-type-fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
git bisect good d1466bc583a81830cef2399a4b8a514398351b40
# good: [fafe1e39ed213221c0bce6b0b31669334368dc97] Merge tag
'afs-netfs-lib-20210426' of
git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs
git bisect good fafe1e39ed213221c0bce6b0b31669334368dc97
# bad: [b1f480bc0686e65d5413c035bd13af2ea4888784] Merge branch
'x86/cpu' into WIP.x86/core, to merge the NOP changes & resolve
a semantic conflict
git bisect bad b1f480bc0686e65d5413c035bd13af2ea4888784
# bad: [0c925c61dae18ee3cb93a61cc9dd9562a066034d]
x86/tools/insn_decoder_test: Convert to insn_decode()
git bisect bad 0c925c61dae18ee3cb93a61cc9dd9562a066034d
# bad: [514ef77607b9ff184c11b88e8f100bc27f07460d]
x86/boot/compressed/sev-es: Convert to insn_decode()
git bisect bad 514ef77607b9ff184c11b88e8f100bc27f07460d
# bad: [9e761296c52dcdb1aaa151b65bd39accb05740d9] x86/insn:
Rename insn_decode() to insn_decode_from_regs()
git bisect bad 9e761296c52dcdb1aaa151b65bd39accb05740d9
# bad: [d0962f2b24c99889a386f0658c71535f56358f77] x86/entry/32:
Remove leftover macros after stackprotector cleanups
git bisect bad d0962f2b24c99889a386f0658c71535f56358f77
# bad: [3fb0fdb3bbe7aed495109b3296b06c2409734023]
x86/stackprotector/32: Make the canary into a regular percpu
variable
git bisect bad 3fb0fdb3bbe7aed495109b3296b06c2409734023
# first bad commit: [3fb0fdb3bbe7aed495109b3296b06c2409734023]
x86/stackprotector/32: Make the canary into a regular percpu
variable

With the bad commit the last words in dmesg are:

mem auto-init: stack:off, heap alloc:off, heap free:off
Initializing HighMem for node 0 (00036ffe:0003f500)
Initializing Movable for node 0 (00000000:00000000)
Checking if this processor honours the WP bit even in supervisor
mode...Ok.
Memory: 948444K/1004124K available (12430K kernel code, 2167K rwdata,
4948K rodata, 716K init, 716K bss, 55680K reserved, 0K cma-reserved,
136200K highmem)
SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
trace event string verifier disabled
Dynamic Preempt: voluntary
rcu: Preemptible hierarchical RCU implementation.
rcu:     RCU event tracing is enabled.
rcu:     RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=2.
   Trampoline variant of Tasks RCU enabled.
   Tracing variant of Tasks RCU enabled.
rcu: RCU calculated value of scheduler-enlistment delay is 100 jiffies.
rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=2
NR_IRQS: 2304, nr_irqs: 512, preallocated irqs: 0

without the bad commit dmesg continues:

random: get_random_bytes called from start_kernel+0x492/0x65a with
crng_init=0
Console: colour dummy device 80x25
printk: console [tty0] enabled
printk: bootconsole [uart0] disabled

....

Any suggestions how to fix are welcome!

Interesting. I added the following fragment to the kernel config:

# CONFIG_STACKPROTECTOR is not set

And this resolves the boot issue (tested with v5.17 i686 on Intel
Merrifield)

I'm not sure that's the correct approach.

I didn't intend as a resolution, merely as a workaround. And since
revert was not possible, as proof issue is localized in stack protector.
Any answer from the Andy Lutomirski?

And in general to x86 maintainers, do we support all features on x86 32-bit? If
no, can it be said explicitly, please?

What compiler version are you using?

I built with Yocto Honister which builds it's own cross-compiler gcc
11.2. For completeness:

root@yuna:~# uname -a
Linux yuna 5.17.0-edison-acpi-standard #1 SMP PREEMPT Sun Mar 20
20:14:17 UTC 2022 i686 i686 i386 GNU/Linux

root@yuna:~# cat /proc/version
Linux version 5.17.0-edison-acpi-standard (oe-user@oe-host)
(i686-poky-linux-gcc (GCC) 11.2.0, GNU ld (GNU Binutils) 2.37.20210721)
#1 SMP PREEMPT Sun Mar 20 20:14:17 UTC 2022

root@yuna:~# cat /etc/os-release
ID=poky-edison
NAME="Poky (Yocto Project Reference Distro)"
VERSION="3.4.4 (honister)"
VERSION_ID=3.4.4
PRETTY_NAME="Poky (Yocto Project Reference Distro) 3.4.4 (honister)"

--
Brian Gerst

What exactly happens when it fails (hang/reboot/oops)?

It just hangs. Rebooting into another kernel with CONFIG_STACKPROTECTOR not set and displaying journal of the failed boot show the last words mentioned above.

Does removing the call to boot_init_stack_canary() in init/main.c fix
the problem?

I will try tomorrow.

I just tried this on top of v6.0:

diff --git a/init/main.c b/init/main.c
index 1fe7942f5d4a..f30ec221f473 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1046,7 +1046,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
     * - random_init() to initialize the RNG from from early entropy sources
     */
     random_init(command_line);
-    boot_init_stack_canary();

     perf_event_init();
     profile_init();

and it just hangs, same as with 5.17. Again tried (without this patch) "CONFIG_STACKPROTECTOR not set" and that boots fine.

--
Brian Gerst

Next message: Bjorn Helgaas: "Re: [PATCH 7/9] PCI: qcom: add support for IPQ8074 Gen3 port"
Previous message: Isaku Yamahata: "Re: [PATCH v10 049/108] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]