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

From: Peter Collingbourne <pcc@google.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	 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>,
	Marco Elver <elver@google.com>
Subject: Re: [PATCH v2 6/8] arm64: Import latest memcpy()/memmove() implementation
Date: Fri, 10 Sep 2021 13:32:03 -0700	[thread overview]
Message-ID: <CAMn1gO5hHHtiLRjhnBhcp2yZ0UVoDSfS_M_AZxGo69F78KLJvA@mail.gmail.com> (raw)
In-Reply-To: <3142ff7a-13d2-951a-089a-c2cf9027dda8@arm.com>

On Fri, Sep 10, 2021 at 4:42 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-09-10 01:35, Peter Collingbourne wrote:
> > 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 :/
>
> > 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