Messages in this thread | | | Date | Tue, 27 Sep 2022 11:41:02 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v2] x86, mem: move memmove to out of line assembler |
| |
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
| |