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

From: Ferry Toth
Date: Fri Sep 30 2022 - 16:30:46 EST


Hi,

Op 29-09-2022 om 16:20 schreef Andy Shevchenko:
On Thu, Sep 29, 2022 at 04:56:07PM +0300, Andy Shevchenko wrote:
+Cc: Ferry

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!

Configuration is based on in-tree i386_defconfig with some drivers enabled
on top (no core stuff was altered, but if you wish to check, it's here:
https://github.com/andy-shev/linux/blob/eds-acpi/arch/x86/configs/i386_defconfig).
For the record (and preventing some questions) the v6.0-rc7 still has this issue.

I can't test reverts, because it's huge pile of changes in that area happened
for the last year or so.

I just tested this by reverting 3fb0fdb3 "x86/stackprotector/32: Make the canary into a regular percpu variable" and it's prerequisite d0962f2b "x86/entry/32: Remove leftover macros after stackprotector cleanups" on top of v5.13 and indeed this resolves the boot issue.

I can also confirm the 2 reverts will not apply on top of v6.0-rc7.