From: Andrey Konovalov <andreyknvl@gmail.com>
To: Peter Collingbourne <pcc@google.com>
Cc: Robin Murphy <robin.murphy@arm.com>,
Will Deacon <will@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Marco Elver <elver@google.com>,
Mark Rutland <mark.rutland@arm.com>,
Evgenii Stepanov <eugenis@google.com>,
Alexander Potapenko <glider@google.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Linux Memory Management List <linux-mm@kvack.org>
Subject: Re: [PATCH v2] kasan: test: add memcpy test that avoids out-of-bounds write
Date: Fri, 10 Sep 2021 23:17:38 +0200 [thread overview]
Message-ID: <CA+fCnZcGg7bsX-7DXcrrggfDe-wJiMsieNQSVB39iZPXBmeP7A@mail.gmail.com> (raw)
In-Reply-To: <20210910211356.3603758-1-pcc@google.com>
On Fri, Sep 10, 2021 at 11:14 PM Peter Collingbourne <pcc@google.com> 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.
>
> Link: https://linux-review.googlesource.com/id/I048d1e6a9aff766c4a53f989fb0c83de68923882
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> ---
> lib/test_kasan.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 8835e0784578..aa8e42250219 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -493,7 +493,7 @@ static void kmalloc_oob_in_memset(struct kunit *test)
> kfree(ptr);
> }
>
> -static void kmalloc_memmove_invalid_size(struct kunit *test)
> +static void kmalloc_memmove_negative_size(struct kunit *test)
> {
> char *ptr;
> size_t size = 64;
> @@ -515,6 +515,21 @@ static void kmalloc_memmove_invalid_size(struct kunit *test)
> kfree(ptr);
> }
>
> +static void kmalloc_memmove_invalid_size(struct kunit *test)
> +{
> + char *ptr;
> + size_t size = 64;
> + volatile size_t invalid_size = size;
> +
> + ptr = kmalloc(size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> +
> + memset((char *)ptr, 0, 64);
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + memmove((char *)ptr, (char *)ptr + 4, invalid_size));
> + kfree(ptr);
> +}
> +
> static void kmalloc_uaf(struct kunit *test)
> {
> char *ptr;
> @@ -1129,6 +1144,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
> KUNIT_CASE(kmalloc_oob_memset_4),
> KUNIT_CASE(kmalloc_oob_memset_8),
> KUNIT_CASE(kmalloc_oob_memset_16),
> + KUNIT_CASE(kmalloc_memmove_negative_size),
> KUNIT_CASE(kmalloc_memmove_invalid_size),
> KUNIT_CASE(kmalloc_uaf),
> KUNIT_CASE(kmalloc_uaf_memset),
> --
> 2.33.0.309.g3052b89438-goog
>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Thanks!
next prev parent reply other threads:[~2021-09-10 21:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-10 21:13 [PATCH v2] kasan: test: add memcpy test that avoids out-of-bounds write Peter Collingbourne
2021-09-10 21:17 ` Andrey Konovalov [this message]
2021-09-13 6:00 ` Marco Elver
2021-09-13 18:19 ` Peter Collingbourne
2021-09-13 9:42 ` Robin Murphy
2021-09-13 18:18 ` Peter Collingbourne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+fCnZcGg7bsX-7DXcrrggfDe-wJiMsieNQSVB39iZPXBmeP7A@mail.gmail.com \
--to=andreyknvl@gmail.com \
--cc=catalin.marinas@arm.com \
--cc=elver@google.com \
--cc=eugenis@google.com \
--cc=glider@google.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-mm@kvack.org \
--cc=mark.rutland@arm.com \
--cc=pcc@google.com \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).