Message-ID: <202509050950.8D39B0D@keescook>
Date: Fri, 5 Sep 2025 10:15:33 -0700
From: Kees Cook <kees@...nel.org>
To: Jakub Jelinek <jakub@...hat.com>
Cc: Qing Zhao <qing.zhao@...cle.com>, Andrew Pinski <pinskia@...il.com>,
	Richard Biener <rguenther@...e.de>,
	Joseph Myers <josmyers@...hat.com>, Jan Hubicka <hubicka@....cz>,
	Richard Earnshaw <richard.earnshaw@....com>,
	Richard Sandiford <richard.sandiford@....com>,
	Marcus Shawcroft <marcus.shawcroft@....com>,
	Kyrylo Tkachov <kyrylo.tkachov@....com>,
	Kito Cheng <kito.cheng@...il.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Andrew Waterman <andrew@...ive.com>,
	Jim Wilson <jim.wilson.gcc@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Dan Li <ashimida.1990@...il.com>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Ramon de C Valle <rcvalle@...gle.com>,
	Joao Moreira <joao@...rdrivepizza.com>,
	Nathan Chancellor <nathan@...nel.org>,
	Bill Wendling <morbo@...gle.com>, gcc-patches@....gnu.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 7/7] kcfi: Add regression test suite

On Fri, Sep 05, 2025 at 09:06:41AM +0200, Jakub Jelinek wrote:
> On Thu, Sep 04, 2025 at 05:24:15PM -0700, Kees Cook wrote:
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/kcfi/kcfi-adjacency.c
> > @@ -0,0 +1,73 @@
> > +/* Test KCFI check/transfer adjacency - regression test for instruction
> > +   insertion.  */
> > +/* { dg-do compile } */
> > +/* { dg-options "-fsanitize=kcfi -O2" } */
> > +/* { dg-options "-fsanitize=kcfi -O2 -march=armv7-a -mfloat-abi=soft" { target arm32 } } */
> 
> For stuff like this you should be using dg-additional-options.
> /* { dg-options "-fsanitize=kcfi -O2" } */
> /* { dg-additional-options "-march=armv7-a -mfloat-abi=soft" { target arm32 } } */
> (in various other tests too).

Ah, perfect; thanks!

> > +/* Should have KCFI instrumentation for all indirect calls.  */
> > +
> > +/* x86_64: Complete KCFI check sequence should be present.  */
> > +/* { dg-final { scan-assembler {movl\t\$-?[0-9]+, %r1[01]d\n\taddl\t[^,]+, %r1[01]d\n\tje\t\.Lkcfi_call[0-9]+\n\.Lkcfi_trap[0-9]+:\n\tud2} { target x86_64-*-* } } } */
> 
> This at least needs
> /* { dg-additional-options "-masm=att" { target x86_64-*-* } } */
> because Intel syntax wouldn't match.

Ah, okay. Is the test suite ever run with -masm != att?

> Does this match with all possible -march/-mtune settings?

I was just running this with "default" state. I didn't think there was
value is testing all the combinations -- all the sequence tests are
basically validating that nothing surprising happened during emission,
etc. What's the best practice for this? Should I add specific
-march/-mtune options for each arch?

> Peope very often do test
> make check RUNTESTFLAGS='--target_board=unix/-march=skylake-avx512'
> etc. so if the test depends on a particular ISA or tuning, better
> add it explicitly to dg-options.

How does that end up meshing? i.e. if I have -mtune=generic in
dg-options, but someone runs with a different -mtune, what happens?

> Also, we try not to use triplets like x86_64-*-* but instead
> { i?86-*-* x86_64-*-* } && lp64
> or
> { i?86-*-* x86_64-*-* } && { ! ia32 }
> depending on whether it is only for -m64, or for both -m64 and -mx32,
> because on some targets the multilib compiler is i?86-*-* defaulting
> to -m32, on most obviously x86_64-*-* defaulting to -m64.

Okay, sounds good. I'll update all of these (for this we only care about
64-bit x86). Out of curiosity what triple matches i?86-*-* and lp64? I
thought x86_64 was sufficient here.

(Though I suddenly realize I think I have nothing in the KCFI patches
can that rejects working under -m32 ... I only do careful target checks
under arm.)

> > +/* x86_64: Should have 0 entry NOPs - function starts immediately with
> > +   pushq.  */
> > +/* { dg-final { scan-assembler {test_function:\n\.LFB[0-9]+:\n\t*\.cfi_startproc\n\t*pushq\t*%rbp} { target x86_64-*-* } } } */
> > +/* { dg-final { scan-assembler-not {\t*\.weak\t*__kcfi_typeid_test_function\n} { target x86_64-*-* } } } */
> 
> .weak is ELF specific, not all targets have it, are the tests restricted to
> targets that do support it and in this syntax?  We have
> /* { dg-require-weak "" } */
> but that doesn't imply a particular function.

Oh, er, this is just for ELF targets. Is there a way to globally
restrict all of these tests to just the 4 arch combos? I'm suspecting
now that these tests will all universally fail for the archs that don't
support -fsanitize=kcfi. I thought dg-options was handling filtering
this, but maybe I've misunderstood?

I'm guessing I need to declare an alias like "lp64" or what I think I
saw asan doing for this feature?

> Also, not all configurations will support .cfi_* directives, that depends
> both on command line parameters and on whether assembler supports those.
> If you expect them in all tests, perhaps you should test for those in
> kcfi.exp and not run the tests at all if the directives aren't supported
> (or if weak isn't supported etc.).

Yeah, this sounds like the place I need to limit the tests from?
Everything I know about dg I've learned in the last month. :P

Studying this some more, it looks like some .exp files use "istarget". I
found, e.g.:

if { [istarget nvptx-*-*] } {
    return
}

So maybe I need that as a top-level filter in kcfi.exp:

if { ![istarget arm*-*-*] && ![istarget x86_64-*-*] && ... } {
    unsupported "KCFI tests not supported on this target"
    return
}

?

I will build some 5th target and see what happens when I run these
tests. :P

> Also, there are targets with different line endings, so usually one scans
> for [\n\r]* instead of just \n.

Okay -- I'm expecting this will go away once I limit to just the 4 targets
I want, or do you want me to universally update the \n patterns to
[\n\r]*?

> No idea why you're using \t*, the compiler emits just one tab.

Ah, I'm not sure where that came from (I will fix it). There has been
a lot of automation on my end to get all these patterns converted from
.s output into dg patterns.

Thanks for looking this over!

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists