linux-kernel - Re: native_save_fl() causes a warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Aug 2018 10:49:51 -0700
From:   Alistair Strachan <astrachan@...gle.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     jdelvare@...e.de, linux-kernel@...r.kernel.org, mka@...omium.org,
        Arnd Bergmann <arnd@...db.de>, hpa@...or.com,
        tstellar@...hat.com, sedat.dilek@...il.com, jgross@...e.com,
        mingo@...nel.org, David.Laight@...lab.com
Subject: Re: native_save_fl() causes a warning

On Fri, Aug 3, 2018 at 9:38 AM Nick Desaulniers <ndesaulniers@...gle.com> wrote:
>
> On Fri, Aug 3, 2018 at 6:10 AM Jean Delvare <jdelvare@...e.de> wrote:
> >
> > Hi Nick,
> >
> > It seems that this linux kernel commit of yours:
> >
> > commit d0a8d9378d16eb3c69bd8e6d23779fbdbee3a8c7
> > Author: Nick Desaulniers
> > Date:   Thu Jun 21 09:23:24 2018 -0700
> >
> >     x86/paravirt: Make native_save_fl() extern inline
> >
> > introduced a new warning (with W=1):
> >
> > ./arch/x86/include/asm/irqflags.h:16:29: warning: no previous prototype for ‘native_save_fl’ [-Wmissing-prototypes]
> >  extern inline unsigned long native_save_fl(void)
> >                              ^
> >
> > Please fix it.
>
> Hi Jean, thanks for the report.  David Laight also reported this
> warning; he tested a patch I sent him overnight.
>
> Let me guess, you're using a version of GCC < 4.9?  It seems that GCC
> < 4.9 will produce that warning for extern inline functions without
> previous declarations.
>
> I'll add your Reported-By tag to the patch that I will send out in a
> few minutes.
>
> > Secondly, I am quite curious why you changed only native_save_fl() from
> > static inline to extern inline, when native_restore_fl(),
> > native_irq_disable() and native_irq_enable() are equally referenced by
> > address in arch/x86/kernel/paravirt.c and thus should suffer from the
> > same problem. Can you explain?
>
> This is a good point.  With native_save_fl, we were not able to boot
> the kernel at all.  Maybe this was called from the boot sequence
> (maybe Juergen knows more)?  It seems that the other functions aren't
> preventing us from booting, but maybe exercising other paths in
> paravirt would expose such an issue?  Or maybe paravirt doesn't have
> the same calling convention requirements for those functions?

The core issue these patches worked around was the automatic/heuristic
generation of stack guard code by clang, which ended up clobbering
%ecx/%rcx in a way not expected by the contract of the paravirt code.
The only function affected by this problem was native_save_fl(),
because only it has a C stack (and thus, has a stack that requires
guarding).

The other functions could have been converted at the same time, and
they will have to be converted if (down the line) somebody adds C
stack variables to them. But, for now, the patch series seems to
correctly work around this issue.

> Is there a standard testing procedure for paravirt? I'd be happy to
> try it to see if we can expose more things that should have the same
> cleanup.

This bug was so obvious you just enabled CONFIG_PARAVIRT, built with
CC=clang, and booted the x86_64 bzImage on any qemu. The minute the
paravirt alternatives were patched in, it exploded.

> --
> Thanks,
> ~Nick Desaulniers