Message-ID: <YkG7zqwFRtmUKVFc@lakrids>
Date:   Mon, 28 Mar 2022 14:44:46 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Jakub Jelinek <jakub@...hat.com>
Cc:     Segher Boessenkool <segher@...nel.crashing.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Borislav Petkov <bp@...en8.de>,
        Nathan Chancellor <nathan@...nel.org>, x86-ml <x86@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>, llvm@...ts.linux.dev,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        linux-toolchains@...r.kernel.org
Subject: Re: clang memcpy calls

On Mon, Mar 28, 2022 at 03:12:14PM +0200, Jakub Jelinek wrote:
> On Mon, Mar 28, 2022 at 01:55:38PM +0100, Mark Rutland wrote:
> > > If coexistence of instrumented and non-instrumented memcpy etc. was the goal
> > > (it clearly wasn't), then the sanitizer libraries wouldn't be overriding
> > > memcpy calls, but instead the compiler would redirect calls to memcpy in
> > > instrumented functions to say __asan_memcpy which then would be
> > > instrumented.
> > 
> > FWIW, I think that approach would be fine for kernel usage.
> > 
> > > > Given the standard doesn't say *anything* about instrumentation, what does GCC
> > > > *require* instrumentation-wise of the memcpy implementation? What happens *in
> > > > practice* today?
> > > > 
> > > > For example, is the userspace implementation of memcpy() instrumented for
> > > > AddressSanitizer, or not?
> > > 
> > > It is, for all functions, whether compiled with -fsanitize=address or not,
> > > if user app is linked with -fsanitize=address, libasan is linked in and
> > > overrides the libc memcpy with its instrumented version.
> > 
> > Thanks for confirming! Just to check, how does libasan prevent recursing
> > within itself on implicitly generated calls to memcpy and friends? Is
> > anything special done to build the libasan code, is that all asm, or
> > something else?
> 
> Generally, most of the libasan wrappers look like
>   do something
>   call the original libc function (address from dlsym/dlvsym)
>   do something
> and the "do something" code isn't that complicated.  

I see; thanks!

> The compiler doesn't add calls to memcpy/memset etc. just to screw up
> users, they are added for a reason, such as copying or clearing very
> large aggregates (including for passing them by value), without -Os it
> will rarely use calls for smaller sizes and will instead expand them
> inline.

Sure; I understand that and (from my side at least) I'm not arguing that
there's malice or so on, just that I don't think we currently have the
tools for the kernel to be able to do the right thing reliably and
robustly. Thanks for helping! :)

> For malloc and the like wrappers I think it uses some TLS recursion
> counters so that say malloc called from dlsym doesn't cause problems.
> 
> Now, one way for the kernel to make kasan work (more) reliably even with
> existing compilers without special tweaks for this might be if those
> calls to no_sanitize_address code aren't mixed with sanitized code all the
> time might be set some per-thread flag when starting a "no sanitized" code
> execution and clear it at the end of the region (or vice versa) and test
> those flags in the kernel's memcpy/memset etc. implementation to decide if
> they should be sanitized or not.

Unfortunately, I don't think the setting a flag is workable, since e.g.
we need to ensure the flag is set before any implicitly-generated calls,
and I don't think we have a reliable way to do that from C. There's also
a number of portions of uninstrumentable code, so from a maintainability
and robustness PoV this option isn't great.

For `noinstr` code specifically (which gets placed into a distinct
section and can be identified by virtual address) we could have the
out-of-line functions look at their return address, but that's not going
to cover the general case of `__attribute__((no_sanitize_address))` or
compilation units built without `-fsanitize=address`.

>From my PoV, distinguishing instrumentable/uninstrumentable calls at
compile time would be ideal. That, or placing the instrumentation into
the caller (omitting it when instrumentation is disabled for that
caller), and expecting the out-of-line forms are never instrumented. I
appreciate that latter option may not be workable due to potential size
bloat, though.

Similar will apply to TSAN, MSAN, etc, and I'm not sure whether those
are mutually exclusive from the compiler's PoV.

> As KASAN is (hopefully) just a kernel debugging aid and not something meant
> for production (in the userspace at least GCC strongly recommends against
> using the sanitizers in production), perhaps allocating one per-thread bool
> or int and changing it in a few spots and testing in the library functions
> might be acceptable.

I'm not sure whether folk are using KASAN in production, but IIRC there
was a desire to do so.

Thanks,
Mark.

Powered by blists - more mailing lists