On Tue, Sep 15, 2020 at 10:46 AM Borislav Petkov <[email protected]> wrote:
Hi Borislav, thank you for a quick response.
> Ok, google guys, pls make sure you Cc LKML too as this is where *all*
> patches and discussions are archived. Adding it now to Cc.
Thank you, I did not know this.
> Ok, so why is the kernel supposed to take yet another ugly workaround
> because there's a bug in the compiler?
I believe the kernel makes a questionable assumption on how clang
uses registers (gs will not be used if stack protection is disabled).
Both kernel and clang behaves unfortunate here.
> disable LTO
CFI depends on LTO.
On Tue, Sep 15, 2020 at 10:57:16AM -0700, Roman Kiryanov wrote:
> I believe the kernel makes a questionable assumption on how clang
> uses registers (gs will not be used if stack protection is disabled).
> Both kernel and clang behaves unfortunate here.
If the kernel is at fault here and this same thing happens with GCC,
sure, but this is a clang-specific fix.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 15, 2020 at 11:27 AM Borislav Petkov <[email protected]> wrote:
> > I believe the kernel makes a questionable assumption on how clang
> > uses registers (gs will not be used if stack protection is disabled).
> > Both kernel and clang behaves unfortunate here.
>
> If the kernel is at fault here and this same thing happens with GCC,
> sure, but this is a clang-specific fix.
This is fair. Unfortunately I am not an x86 asm expert. I expect the proper
fix should land into arch/x86/kernel/acpi/wakeup_64.S to init %gs
(maybe some more registers) before "jmp restore_processor_state".
Regards,
Roman.
On Tue, Sep 15, 2020 at 11:36:13AM -0700, Roman Kiryanov wrote:
> This is fair. Unfortunately I am not an x86 asm expert. I expect the proper
> fix should land into arch/x86/kernel/acpi/wakeup_64.S to init %gs
> (maybe some more registers) before "jmp restore_processor_state".
... because "LLVM appears to be inlining functions with stack protectors
into functions compiled with -fno-stack-protector" and now the *kernel*
needs to init %gs?
How about LLVM stops doing those wrong inlining decisions?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 15, 2020 at 11:52 AM Borislav Petkov <[email protected]> wrote:
>
> On Tue, Sep 15, 2020 at 11:36:13AM -0700, Roman Kiryanov wrote:
> > This is fair. Unfortunately I am not an x86 asm expert. I expect the proper
> > fix should land into arch/x86/kernel/acpi/wakeup_64.S to init %gs
> > (maybe some more registers) before "jmp restore_processor_state".
>
> ... because "LLVM appears to be inlining functions with stack protectors
> into functions compiled with -fno-stack-protector" and now the *kernel*
> needs to init %gs?
>
> How about LLVM stops doing those wrong inlining decisions?
This will remove the issue for a while until clang/gcc/other decides to use
%gs for other purposes before the kernel initializes it.
Regards,
Roman.
On Tue 2020-09-15 11:36:13, Roman Kiryanov wrote:
> On Tue, Sep 15, 2020 at 11:27 AM Borislav Petkov <[email protected]> wrote:
> > > I believe the kernel makes a questionable assumption on how clang
> > > uses registers (gs will not be used if stack protection is disabled).
> > > Both kernel and clang behaves unfortunate here.
> >
> > If the kernel is at fault here and this same thing happens with GCC,
> > sure, but this is a clang-specific fix.
>
> This is fair. Unfortunately I am not an x86 asm expert. I expect the proper
> fix should land into arch/x86/kernel/acpi/wakeup_64.S to init %gs
> (maybe some more registers) before "jmp restore_processor_state".
That would certainly be nicer / more acceptable solution than patch
being proposed here.
Code was written with assumption compiler random C code would not use
%gs. If that's no longer true, fixing it in wakeup_64.S _with comments
explaining what goes on_ might be solution.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, Sep 18, 2020 at 3:25 PM Pavel Machek <[email protected]> wrote:
>
> On Tue 2020-09-15 11:36:13, Roman Kiryanov wrote:
> > On Tue, Sep 15, 2020 at 11:27 AM Borislav Petkov <[email protected]> wrote:
> > > > I believe the kernel makes a questionable assumption on how clang
> > > > uses registers (gs will not be used if stack protection is disabled).
> > > > Both kernel and clang behaves unfortunate here.
> > >
> > > If the kernel is at fault here and this same thing happens with GCC,
> > > sure, but this is a clang-specific fix.
> >
> > This is fair. Unfortunately I am not an x86 asm expert. I expect the proper
> > fix should land into arch/x86/kernel/acpi/wakeup_64.S to init %gs
> > (maybe some more registers) before "jmp restore_processor_state".
>
> That would certainly be nicer / more acceptable solution than patch
> being proposed here.
>
> Code was written with assumption compiler random C code would not use
> %gs. If that's no longer true, fixing it in wakeup_64.S _with comments
> explaining what goes on_ might be solution.
I looked and restore_processor_state is referenced in several places,
so changing
wakeup_64.S is not enough. Is moving the beginning of restore_processor_state
to an .S file ok? I see restore_processor_state initializes CR registers first,
do you know if there is a reason to do so (can I init segment
registers before CR ones)?
Regards,
Roman.
Hi!
> > > > > I believe the kernel makes a questionable assumption on how clang
> > > > > uses registers (gs will not be used if stack protection is disabled).
> > > > > Both kernel and clang behaves unfortunate here.
> > > >
> > > > If the kernel is at fault here and this same thing happens with GCC,
> > > > sure, but this is a clang-specific fix.
> > >
> > > This is fair. Unfortunately I am not an x86 asm expert. I expect the proper
> > > fix should land into arch/x86/kernel/acpi/wakeup_64.S to init %gs
> > > (maybe some more registers) before "jmp restore_processor_state".
> >
> > That would certainly be nicer / more acceptable solution than patch
> > being proposed here.
> >
> > Code was written with assumption compiler random C code would not use
> > %gs. If that's no longer true, fixing it in wakeup_64.S _with comments
> > explaining what goes on_ might be solution.
>
> I looked and restore_processor_state is referenced in several places,
> so changing
> wakeup_64.S is not enough. Is moving the beginning of restore_processor_state
> to an .S file ok? I see restore_processor_state initializes CR registers first,
> do you know if there is a reason to do so (can I init segment
> registers before CR ones)?
Yes, moving to .S file should be okay.
CR first... makes sense to me, they really select how segment registers will be
interpretted, etc. If it will work the other way... not sure.
I'd keep existing code if I were you. This is tricky to debug.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html