Re: [PATCH v2] kasan: test: add memcpy test that avoids out-of-bounds write

On Mon, Sep 13, 2021 at 2:42 AM Robin Murphy [off-list ref] wrote:

On 2021-09-10 22:13, Peter Collingbourne wrote:

quoted

With HW tag-based KASAN, error checks are performed implicitly by the
load and store instructions in the memcpy implementation.  A failed check
results in tag checks being disabled and execution will keep going. As a
result, under HW tag-based KASAN, prior to commit 1b0668be62cf ("kasan:
test: disable kmalloc_memmove_invalid_size for HW_TAGS"), this memcpy
would end up corrupting memory until it hits an inaccessible page and
causes a kernel panic.

This is a pre-existing issue that was revealed by commit 285133040e6c
("arm64: Import latest memcpy()/memmove() implementation") which changed
the memcpy implementation from using signed comparisons (incorrectly,
resulting in the memcpy being terminated early for negative sizes)
to using unsigned comparisons.

It is unclear how this could be handled by memcpy itself in a reasonable
way. One possibility would be to add an exception handler that would force
memcpy to return if a tag check fault is detected -- this would make the
behavior roughly similar to generic and SW tag-based KASAN. However,
this wouldn't solve the problem for asynchronous mode and also makes
memcpy behavior inconsistent with manually copying data.

This test was added as a part of a series that taught KASAN to detect
negative sizes in memory operations, see commit 8cceeff48f23 ("kasan:
detect negative size in memory operation function"). Therefore we
should keep testing for negative sizes with generic and SW tag-based
KASAN. But there is some value in testing small memcpy overflows, so
let's add another test with memcpy that does not destabilize the kernel
by performing out-of-bounds writes, and run it in all modes.

The only thing is, that's nonsense. You can't pass a negative size to
memmove()/memcpy(), any more than you could pass a negative address. You
can use the usual integer conversions to pass a very large size, but
that's no different from just passing a very large size, and the
language does not make any restrictions on the validity of very large
sizes. Indeed in general a 32-bit program could legitimately memcpy()
exactly half its address space to the other half, or memmove() a 3GB
buffer a small distance.

I'm not sure what we're trying to enforce there, other than arbitrary
restrictions on how we think it makes sense to call library functions.
The only way to say that a size is actually invalid is if it leads to an
out-of-bounds access relative to the source or destination buffer, but
to provoke that the given size only ever needs to be at least 1 byte
larger than the object - making it excessively large only generates
excessively large numbers of invalid accesses, and I fail to see what
use that has. By all means introduce KAROHWTIMSTCLFSAN, but I'm not
convinced it's meaningfully within the scope of *address* sanitisation.

This is an orthogonal issue, isn't it? It may make sense to make the
memmove()/memcpy() behavior controllable separately, but that can be
done separately from this change.

Peter