Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1dbe4ea5-837f-e1a6-6415-bdc0aee96fc4@gmail.com>
Date:   Mon, 14 Nov 2022 23:16:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v2 1/2] x86/stackprotector/32: Make the canary into a
 regular percpu variable
Content-Language: en-US
To:     Brian Gerst <brgerst@gmail.com>
Cc:     Andy Shevchenko <andriy.shevchenko@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Andy Lutomirski <luto@kernel.org>, x86@kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Sedat Dilek <sedat.dilek@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Sean Christopherson <seanjc@google.com>,
        Joerg Roedel <jroedel@suse.de>
References: <cover.1613243844.git.luto@kernel.org>
 <c0ff7dba14041c7e5d1cae5d4df052f03759bef3.1613243844.git.luto@kernel.org>
 <YzWj9zjTJI3RCDf2@smile.fi.intel.com> <YzWpob6MOf1SJr5I@smile.fi.intel.com>
 <c554f363-05db-b185-1a7d-ce8ae27890da@gmail.com>
 <d1f76ef7-2d91-2be2-285c-8dbd208239a2@gmail.com>
 <Y2u+J5EEyxRuusSr@smile.fi.intel.com>
 <CAMzpN2iEQVQWsPX58ONxaH=gkUACtq1t2OpozZ55+f=Ub+AwyQ@mail.gmail.com>
 <d0d8d5ea-a752-d082-d8fe-68ff888afb59@gmail.com>
 <CAMzpN2gYdwLmeqh+=MAkA2ALaB1PK_YD_5tpZMA09M6NCrvOjg@mail.gmail.com>
From:   Ferry Toth <fntoth@gmail.com>
In-Reply-To: <CAMzpN2gYdwLmeqh+=MAkA2ALaB1PK_YD_5tpZMA09M6NCrvOjg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.
> --
> Brian Gerst