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

From: Catalin Marinas <catalin.marinas@arm.com>
To: Peter Collingbourne <pcc@google.com>
Cc: Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Alexander Potapenko <glider@google.com>,
	Andrey Konovalov <andreyknvl@gmail.com>
Subject: Re: [PATCH v2 6/8] arm64: Import latest memcpy()/memmove() implementation
Date: Fri, 10 Sep 2021 12:36:26 +0100	[thread overview]
Message-ID: <YTtDOq7dXhndRgNh@arm.com> (raw)
In-Reply-To: <CAMn1gO7rJzUg53cet8ocN0aMrEgQ2iqUN2pB-iQ=nBT7dafdtA@mail.gmail.com>

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