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

[Peter Collingbourne <pcc@google.com>]

On Mon, Sep 13, 2021 at 2:42 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-09-10 22:13, Peter Collingbourne wrote:
> > 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