Re: [PATCH v2 6/8] arm64: Import latest memcpy()/memmove() implementation

On Fri, Sep 10, 2021 at 4:42 AM Robin Murphy [off-list ref] wrote:

On 2021-09-10 01:35, Peter Collingbourne wrote:

quoted

Hi Robin,

(apologies for breaking the threading, I wasn't subscribed to
linux-arm-kernel when you sent this)

It looks like this patch breaks the KASAN unit tests when running in
HW tags mode. You can construct a config that reproduces the problem
with something like:

make O=out defconfig
scripts/config --file out/.config -e CONFIG_KUNIT -e CONFIG_KASAN -e
CONFIG_KASAN_HW_TAGS -e CONFIG_KASAN_KUNIT_TEST
yes '' | make O=out syncconfig

With that the "kmalloc_memmove_invalid_size" test fails and causes the
kernel panic below.

What appears to be going on is that whereas the old memcpy
implementation ends up returning early after not copying very much
data if supplied a "negative" size as a side effect of using
conditional branches that implement signed comparisons (e.g. b.ge on
line 75), the new implementation does not exit early and attempts to
copy a large amount of data backwards 4 bytes (after having disabled
MTE early on as a result of a tag mismatch) until it hits a read-only
page and causes a panic.

If the test depends on the implementation of memmove() being buggy, it's
clearly not a very good test :/

quoted

I'm not sure what the correct fix should be. It seems that at least
when KASAN is enabled we should be able to catch these invalid memcpys
somehow, print an error report and ideally abort the entire operation.
But we should do so without causing a performance impact in the usual
case.

Not sure I follow - the top of the log below clearly shows that KASASN
*is* detecting and reporting a bad access. The fact that code intended
to deliberately corrupt memory goes on to corrupt memory doesn't seem
particularly surprising to me. A bug exists (intentionally or otherwise)
which led to that invalid access being attempted, and KASAN can't
magically change that.

That makes sense. So we want to make sure that:

a) memmove overflows trigger an error report
b) we can safely continue execution

It seems the best way to ensure both of these properties is to have
the memmove do an out-of-bounds read -- all writes would then be into
the correct allocation. That way, we should see an error report but no
memory corruption. We can do that by changing the memmove call to only
copy "size" bytes.

When I was about to send my patch I noticed that Andrey already sent a
fix for this in commit 1b0668be62cf that disables the memmove test
when KASAN HW tags is enabled. I reckon there's still some value in
testing a) in HW tags mode, so I went ahead and had my patch also
revert his.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel