Messages in this thread Patch in this message | | | Date | Thu, 17 Nov 2022 20:28:32 +0100 | Subject | Re: [PATCH v2 1/2] x86/stackprotector/32: Make the canary into a regular percpu variable | From | Ferry Toth <> |
| |
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@gmail.com> 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@intel.com> 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
| |