[PATCH v3 0/2] string: Add load_unaligned_zeropad() code path to sized_strscpy()

[PATCH v3 0/2] string: Add load_unaligned_zeropad() code path to sized_strscpy()

[Peter Collingbourne]

This series fixes an issue where strscpy() would sometimes trigger
a false positive KASAN report with MTE.

v3:
- simplify test case

Peter Collingbourne (1):
  string: Add load_unaligned_zeropad() code path to sized_strscpy()

Vincenzo Frascino (1):
  kasan: Add strscpy() test to trigger tag fault on arm64

 lib/string.c            | 13 ++++++++++---
 mm/kasan/kasan_test_c.c | 13 +++++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.49.0.395.g12beb8f557-goog

[PATCH v3 1/2] string: Add load_unaligned_zeropad() code path to sized_strscpy()

[Peter Collingbourne]

The call to read_word_at_a_time() in sized_strscpy() is problematic
with MTE because it may trigger a tag check fault when reading
across a tag granule (16 bytes) boundary. To make this code
MTE compatible, let's start using load_unaligned_zeropad()
on architectures where it is available (i.e. architectures that
define CONFIG_DCACHE_WORD_ACCESS). Because load_unaligned_zeropad()
takes care of page boundaries as well as tag granule boundaries,
also disable the code preventing crossing page boundaries when using
load_unaligned_zeropad().

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/If4b22e43b5a4ca49726b4bf98ada827fdf755548
Fixes: 94ab5b61ee16 ("kasan, arm64: enable CONFIG_KASAN_HW_TAGS")
Cc: stable@vger.kernel.org
---
v2:
- new approach

 lib/string.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index eb4486ed40d25..b632c71df1a50 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -119,6 +119,7 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 	if (count == 0 || WARN_ON_ONCE(count > INT_MAX))
 		return -E2BIG;
 
+#ifndef CONFIG_DCACHE_WORD_ACCESS
 #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
 	/*
 	 * If src is unaligned, don't cross a page boundary,
@@ -133,12 +134,14 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 	/* If src or dest is unaligned, don't do word-at-a-time. */
 	if (((long) dest | (long) src) & (sizeof(long) - 1))
 		max = 0;
+#endif
 #endif
 
 	/*
-	 * read_word_at_a_time() below may read uninitialized bytes after the
-	 * trailing zero and use them in comparisons. Disable this optimization
-	 * under KMSAN to prevent false positive reports.
+	 * load_unaligned_zeropad() or read_word_at_a_time() below may read
+	 * uninitialized bytes after the trailing zero and use them in
+	 * comparisons. Disable this optimization under KMSAN to prevent
+	 * false positive reports.
 	 */
 	if (IS_ENABLED(CONFIG_KMSAN))
 		max = 0;
@@ -146,7 +149,11 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 	while (max >= sizeof(unsigned long)) {
 		unsigned long c, data;
 
+#ifdef CONFIG_DCACHE_WORD_ACCESS
+		c = load_unaligned_zeropad(src+res);
+#else
 		c = read_word_at_a_time(src+res);
+#endif
 		if (has_zero(c, &data, &constants)) {
 			data = prep_zero_mask(c, data, &constants);
 			data = create_zero_mask(data);
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v3 2/2] kasan: Add strscpy() test to trigger tag fault on arm64

[Peter Collingbourne]

From: Vincenzo Frascino <vincenzo.frascino@arm.com>

When we invoke strscpy() with a maximum size of N bytes, it assumes
that:
- It can always read N bytes from the source.
- It always write N bytes (zero-padded) to the destination.

On aarch64 with Memory Tagging Extension enabled if we pass an N that is
bigger then the source buffer, it triggers an MTE fault.

Implement a KASAN KUnit test that triggers the issue with the current
implementation of read_word_at_a_time() on aarch64 with MTE enabled.

Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Co-developed-by: Peter Collingbourne <pcc@google.com>
Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/If88e396b9e7c058c1a4b5a252274120e77b1898a
---
v3:
- simplify test case

v2:
- rebased
- fixed test failure

 mm/kasan/kasan_test_c.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
index 59d673400085f..b69f66b7eda1d 100644
--- a/mm/kasan/kasan_test_c.c
+++ b/mm/kasan/kasan_test_c.c
@@ -1570,6 +1570,7 @@ static void kasan_memcmp(struct kunit *test)
 static void kasan_strings(struct kunit *test)
 {
 	char *ptr;
+	char *src;
 	size_t size = 24;
 
 	/*
@@ -1581,6 +1582,18 @@ static void kasan_strings(struct kunit *test)
 	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
 
+	src = kmalloc(KASAN_GRANULE_SIZE, GFP_KERNEL | __GFP_ZERO);
+	strscpy(src, "f0cacc1a0000000", KASAN_GRANULE_SIZE);
+
+	/*
+	 * The expected size does not include the terminator '\0'
+	 * so it is (KASAN_GRANULE_SIZE - 2) ==
+	 * KASAN_GRANULE_SIZE - ("initial removed character" + "\0").
+	 */
+	KUNIT_EXPECT_EQ(test, KASAN_GRANULE_SIZE - 2,
+			strscpy(ptr, src + 1, KASAN_GRANULE_SIZE));
+
+	kfree(src);
 	kfree(ptr);
 
 	/*
-- 
2.49.0.395.g12beb8f557-goog

Re: [PATCH v3 0/2] string: Add load_unaligned_zeropad() code path to sized_strscpy()

[Kees Cook]

On Mon, Mar 24, 2025 at 06:56:13PM -0700, Peter Collingbourne wrote:
> This series fixes an issue where strscpy() would sometimes trigger
> a false positive KASAN report with MTE.

Thanks! Should this go via string API (me) or KASAN?

-Kees

-- 
Kees Cook

Re: [PATCH v3 0/2] string: Add load_unaligned_zeropad() code path to sized_strscpy()

[Peter Collingbourne]

On Tue, Mar 25, 2025 at 12:43 PM Kees Cook <kees@kernel.org> wrote:
>
> On Mon, Mar 24, 2025 at 06:56:13PM -0700, Peter Collingbourne wrote:
> > This series fixes an issue where strscpy() would sometimes trigger
> > a false positive KASAN report with MTE.
>
> Thanks! Should this go via string API (me) or KASAN?

Let's take this via string API.

Peter

Re: [PATCH v3 2/2] kasan: Add strscpy() test to trigger tag fault on arm64

[Andrey Konovalov]

On Tue, Mar 25, 2025 at 2:56 AM Peter Collingbourne <pcc@google.com> wrote:
>
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> When we invoke strscpy() with a maximum size of N bytes, it assumes
> that:
> - It can always read N bytes from the source.
> - It always write N bytes (zero-padded) to the destination.
>
> On aarch64 with Memory Tagging Extension enabled if we pass an N that is
> bigger then the source buffer, it triggers an MTE fault.
>
> Implement a KASAN KUnit test that triggers the issue with the current
> implementation of read_word_at_a_time() on aarch64 with MTE enabled.

I think the wording here is confusing, makes it sound like the issue
is not fixed.

> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Co-developed-by: Peter Collingbourne <pcc@google.com>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/If88e396b9e7c058c1a4b5a252274120e77b1898a
> ---
> v3:
> - simplify test case
>
> v2:
> - rebased
> - fixed test failure
>
>  mm/kasan/kasan_test_c.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index 59d673400085f..b69f66b7eda1d 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1570,6 +1570,7 @@ static void kasan_memcmp(struct kunit *test)
>  static void kasan_strings(struct kunit *test)
>  {
>         char *ptr;
> +       char *src;
>         size_t size = 24;
>
>         /*
> @@ -1581,6 +1582,18 @@ static void kasan_strings(struct kunit *test)
>         ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> +       src = kmalloc(KASAN_GRANULE_SIZE, GFP_KERNEL | __GFP_ZERO);
> +       strscpy(src, "f0cacc1a0000000", KASAN_GRANULE_SIZE);
> +
> +       /*

Please also extend the comment here to say something like: "Make sure
that strscpy() does not trigger KASAN if it overreads into poisoned
memory".

> +        * The expected size does not include the terminator '\0'
> +        * so it is (KASAN_GRANULE_SIZE - 2) ==
> +        * KASAN_GRANULE_SIZE - ("initial removed character" + "\0").
> +        */
> +       KUNIT_EXPECT_EQ(test, KASAN_GRANULE_SIZE - 2,
> +                       strscpy(ptr, src + 1, KASAN_GRANULE_SIZE));
> +
> +       kfree(src);
>         kfree(ptr);
>
>         /*
> --
> 2.49.0.395.g12beb8f557-goog
>

The code looks much better!

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>