linux-kernel - Re: [PATCH v2] x86: disable non-instrumented version of copy

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]

Message-ID: <CAHk-=whGn2hDpHDrgHEzGdicXLZMTgFq8iaH8p+HnZVWj32_VQ@mail.gmail.com>
Date: Tue, 5 Mar 2024 09:57:57 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, 
	Alexander Potapenko <glider@...gle.com>, Marco Elver <elver@...gle.com>, Dmitry Vyukov <dvyukov@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, "the arch/x86 maintainers" <x86@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v2] x86: disable non-instrumented version of copy_mc when
 KMSAN is enabled

[ For the KMSAN people I brought in: this is the patch I'm NAK'ing:

    https://lore.kernel.org/all/3b7dbd88-0861-4638-b2d2-911c97a4cadf@I-love.SAKURA.ne.jp/

  and it looks like you were already cc'd on earlier versions (which
were even more broken) ]

On Tue, 5 Mar 2024 at 03:31, Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
>
> Ping?

Please don't add new people and 'ping' without context. Very annoying.

That said, after having to search for it that whole patch is
disgusting. Why make duplicated complex conditionals when you could
have just had the tests inside one #ifndef.

Also, that patch means that a KMSAN kernel potentially simply no
longer works on admittedly crappy hardware that almost doesn't exist.

So now a debug feature changes actual semantics in a big way. Not ok.

So I think this patch is ugly but also doubly incorrect.

I think the KMSAN people need to tell us how to tell kmsan that it's a
memcpy (and about the "I'm going to touch this part of memory", needed
for the "copy_mv_to_user" side).

So somebody needs to abstract out that

        depot_stack_handle_t origin;

        if (!kmsan_enabled || kmsan_in_runtime())
                return;

        kmsan_enter_runtime();
        /* Using memmove instead of memcpy doesn't affect correctness. */
        kmsan_internal_memmove_metadata(dst, (void *)src, n);
        kmsan_leave_runtime();

        set_retval_metadata(shadow, origin);

kind of thing, and expose it as a helper function for "I did something
that looks like a memory copy", the same way that we currently have
kmsan_copy_page_meta()

Because NO, IT IS NEVER CORRECT TO USE __msan_memcpy FOR THE MC COPIES.

So no. NAK on that patch. It's completely and utterly wrong.

The onus is firmly on the KMSAN people to give kernel people a way to
tell KMSAN to shut the f&%^ up about that.

End result: don't bother the x86 people until KMSAN has the required support.

               Linus