linux-kernel - Re: [GIT pull] x86/urgent for v6.16-rc7

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAMj1kXFsC0yFm7nzpYFauAaKG2OEV4PNgCN8ThpxV3WGjcqj5w@mail.gmail.com>
Date: Mon, 21 Jul 2025 16:25:12 +1000
From: Ard Biesheuvel <ardb@...nel.org>
To: tglx <tglx@...utronix.de>, Marco Elver <elver@...gle.com>, Daniel Axtens <dja@...ens.net>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
	x86@...nel.org, Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [GIT pull] x86/urgent for v6.16-rc7

(cc Marco, Daniel)

Hi Thomas,

Thanks for getting to the bottom of this.

On Mon, 21 Jul 2025 at 16:08, tglx <tglx@...utronix.de> wrote:
>
> On Sun, Jul 20 2025 at 11:35, Linus Torvalds wrote:
> > On Sun, 20 Jul 2025 at 05:05, Thomas Gleixner <tglx@...utronix.de> wrote:
> >>
> >> A single fix for a GCC wreckage, which emits a KCSAN instrumentation call
> >> in __sev_es_nmi_complete() despite the function being annotated with
> >> 'noinstr'. As all functions in that source file are noinstr, exclude the
> >> whole file from KCSAN in the Makefile to cure it.
> >
> > Hmm. I'm not entirely sure if this counts as a gcc bug.
> >
> > In particular, look at the *declaration* of __sev_es_nmi_complete() in
> > <asm/sev.h>, and note the complete lack of 'noinstr':
> >
> >     extern void __sev_es_nmi_complete(void);
> >
> > and I wonder if it might be the case that gcc might pick up the lack
> > of 'noinstr' from the declaration, even if it's there in the
> > definition..
> >
> > The fix for this obviously ends up working and is fine regardless, but
> > it's _possible_ that this is self-inflicted pain rather than an
> > outright compiler bug.
>
> I agree. See below.
>
> > Because having a declaration and a definition that doesn't match
> > sounds like a bad idea in the first place.
>
> I disagree here. When the compiler evaluates the function body it must
> have evaluated noinstr on the definition, no?
>
> Unfortunately the data provided in the change log does not tell what was
> actually instrumented inside of that function. I just reproduced it
> locally.
>
...
> __set_bit() resolves to:
>
> static __always_inline void ___set_bit(unsigned long nr, volatile unsigned long *addr)
> {
>         instrument_write(addr + BIT_WORD(nr), sizeof(long));
>         arch___set_bit(nr, addr);
> }
>

So this is the issue right here: an __always_inline function that
unconditionally calls the KASAN/KCSAN check functions. And indeed, the
compiler is not to blame here, because these instrumentations were
emitted by our code, and in a manner that doesn't care about the
compiler attributes that disable KASAN or KCSAN.

The upshot of this is that all noinstr users of __set_bit() end up
calling the instrumented version if the kconfig happens to have KASAN
or KCSAN enabled, and I seriously doubt that this is what we want.
Including one header or the other obviously doesn't work when
disabling these instrumentations at the function level.

Ideally, we'd have __builtin_kasan_enabled() and
__builtin_kcsan_enabled() compiler builtins that provide the correct
answer for the current function, but that will take a while to land if
we start working on it now (for both Clang and GCC).