Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Sep 2022 11:41:02 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H . Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        llvm@lists.linux.dev, Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCH v2] x86, mem: move memmove to out of line assembler
Message-ID: <202209271126.605B4FF@keescook>
References: <CAKwvOd=a+0VNP-d=mHnbM4ujdtPrujru4dxHtfDo+EEM+b6deg@mail.gmail.com>
 <20220927172839.3708280-1-ndesaulniers@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220927172839.3708280-1-ndesaulniers@google.com>
Precedence: bulk

On Tue, Sep 27, 2022 at 10:28:39AM -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")
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Unfortunately, it seems something has gone wrong with this
implementation. Before the patch:

$ ./tools/testing/kunit/kunit.py run --arch=i386 memcpy
...
[11:26:24] [PASSED] memmove_test
...

After the patch:

$ ./tools/testing/kunit/kunit.py run --arch=i386 memcpy
...
[11:25:59] # memmove_test: ok: memmove() static initializers
[11:25:59] # memmove_test: ok: memmove() direct assignment
[11:25:59] # memmove_test: ok: memmove() complete overwrite
[11:25:59] # memmove_test: ok: memmove() middle overwrite
[11:25:59] # memmove_test: EXPECTATION FAILED at lib/memcpy_kunit.c:176
[11:25:59] Expected dest.data[i] == five.data[i], but
[11:25:59] dest.data[i] == 136
[11:25:59] five.data[i] == 0
[11:25:59] line 176: dest.data[10] (0x88) != five.data[10] (0x00)
[11:25:59] # memmove_test: ok: memmove() argument side-effects
[11:25:59] # memmove_test: ok: memmove() overlapping wr\xf0te
[11:25:59] not ok 3 - memmove_test
[11:25:59] [FAILED] memmove_test
...

data[10] starts set as 0x99, and in theory gets 0x0 written to it, but
the self-test sees 0x88 there. (?!) It seems the macro side-effect test
caught something else entirely?

-Kees

--
Kees Cook