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

Hi Peter,

On Thu, Sep 09, 2021 at 05:35:54PM -0700, Peter Collingbourne wrote:

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

lore.kernel.org has some instructions on how to download the message and
reply to it.

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.

Treating the size argument as unsigned long is the correct way, it
matches the function prototype.

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.

I'm not convinced that's a valid test. Do we have any statement
somewhere on what a valid size for memmove/memcpy is? We could
artificially limit it to VA_BITS or even to 63 bits so that it stays a
positive number but the generic memmove() implementation doesn't do such
checks either.

Since the memcpy/memmove routines don't have a fail-safe mode, the only
thing the kernel can do on an MTE fault is to disable tag checking,
report it and continue. What you need instead is copy_*_kernel_nofault
in the kasan tests as they return the number of bytes copied.

My suggestion for a quick fix would be to remove the kasan test.

-- 
Catalin

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