linux-kernel - Re: [PATCH v4] x86, mem: move memmove to out of line assembler

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: <202209281431.C5EF6C32A@keescook>
Date:   Wed, 28 Sep 2022 15:03:43 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        llvm@...ts.linux.dev, Andy Lutomirski <luto@...nel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH v4] x86, mem: move memmove to out of line assembler

On Wed, Sep 28, 2022 at 02:05:12PM -0700, Nick Desaulniers wrote:
> When building ARCH=i386 with CONFIG_LTO_CLANG_FULL=y, it's possible
> (depending on additional configs which I have not been able to isolate)
> to observe a failure during register allocation:
> 
>   error: inline assembly requires more registers than available
> 
> when memmove is inlined into tcp_v4_fill_cb() or tcp_v6_fill_cb().
> 
> memmove is quite large and probably shouldn't be inlined due to size
> alone. A noinline function attribute would be the simplest fix, but
> there's a few things that stand out with the current definition:
> 
> In addition to having complex constraints that can't always be resolved,
> the clobber list seems to be missing %bx and %dx, and possibly %cl. By
> using numbered operands rather than symbolic operands, the constraints
> are quite obnoxious to refactor.
> 
> Having a large function be 99% inline asm is a code smell that this
> function should simply be written in stand-alone out-of-line assembler.
> That gives the opportunity for other cleanups like fixing the
> inconsistent use of tabs vs spaces and instruction suffixes, and the
> label 3 appearing twice.  Symbolic operands and local labels would
> provide this code with a fresh coat of paint.
> 
> Moving this to out of line assembler guarantees that the
> compiler cannot inline calls to memmove.
> 
> This has been done previously for 64b:
> commit 9599ec0471de ("x86-64, mem: Convert memmove() to assembly file
> and fix return value bug")
> 
> Also, add a test that tickles the `rep movsl` implementation to test it
> for correctness, since it has implicit operands.

Yeah, thanks for poking this in particular. I was bothered that the
side-effect test caught a corner case and was planning to expand the
memcpy tests even more; thank you for doing that! I've got some more
coming and can confirm they tickled the same bug.

> Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>

This time I've looked at the binary differences between the functions
generated by both GCC[1] and Clang[2]. GCC is a little more difficult to
compare, since it does some register swaps, but the Clang output is same
excepting the order of push/pop, and different nops.

Reviewed-by: Kees Cook <keescook@...omium.org>

Nick's tests pass, and my newly written tests also pass; I'll send those
as a follow-up.

Tested-by: Kees Cook <keescook@...omium.org>

-Kees

[1] https://paste.debian.net/hidden/b6298e62/
[2] https://paste.debian.net/hidden/d8343143/

-- 
Kees Cook