[PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Mark Rutland]

When the kernel is built with CONFIG_KASAN_HW_TAGS and the CPU supports
MTE, memory accesses are checked at 16-byte granularity, and
out-of-bounds accesses can result in tag check faults. Our current
implementation of strlen() makes unaligned 16-byte accesses (within a
naturally aligned 4096-byte window), and can trigger tag check faults.

This can be seen at boot time, e.g.

| BUG: KASAN: invalid-access in __pi_strlen+0x14/0x150
| Read at addr f4ff0000c0028300 by task swapper/0/0
| Pointer tag: [f4], memory tag: [fe]
|
| CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09550-g03c2813535a2-dirty #20
| Hardware name: linux,dummy-virt (DT)
| Call trace:
|  dump_backtrace+0x0/0x1b0
|  show_stack+0x1c/0x30
|  dump_stack_lvl+0x68/0x84
|  print_address_description+0x7c/0x2b4
|  kasan_report+0x138/0x38c
|  __do_kernel_fault+0x190/0x1c4
|  do_tag_check_fault+0x78/0x90
|  do_mem_abort+0x44/0xb4
|  el1_abort+0x40/0x60
|  el1h_64_sync_handler+0xb0/0xd0
|  el1h_64_sync+0x78/0x7c
|  __pi_strlen+0x14/0x150
|  __register_sysctl_table+0x7c4/0x890
|  register_leaf_sysctl_tables+0x1a4/0x210
|  register_leaf_sysctl_tables+0xc8/0x210
|  __register_sysctl_paths+0x22c/0x290
|  register_sysctl_table+0x2c/0x40
|  sysctl_init+0x20/0x30
|  proc_sys_init+0x3c/0x48
|  proc_root_init+0x80/0x9c
|  start_kernel+0x640/0x69c
|  __primary_switched+0xc0/0xc8

To fix this, we can reduce the (strlen-internal) MIN_PAGE_SIZE to 16
bytes when CONFIG_KASAN_HW_TAGS is selected. This will cause strlen() to
align the base pointer downwards to a 16-byte boundary, and to discard
the additional prefix bytes without counting them. All subsequent
accesses will be 16-byte aligned 16-byte LDPs. While the comments say
the body of the loop will access 32 bytes, this is performed as two
16-byte acceses, with the second made only if the first did not
encounter a NUL byte, so the body of the loop will not over-read across
a 16-byte boundary.

No other string routines are affected. The other str*() routines will
not make any access which straddles a 16-byte boundary, and the mem*()
routines will only make acceses which straddle a 16-byte boundary when
which is entirely within the bounds of the relevant base and size
arguments.

Fixes: 325a1de81287a3d4 (“arm64: Import updated version of Cortex Strings' strlen”)
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Potapenko <glider@google.com
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/lib/strlen.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

Note: to build-test this, you'll also need to apply a fix from Marco [1], which
is on its way to mainline via akpm. I've locally aplied that, and tested this
patch with a recent build of QEMU with MTE enabled.

Mark.

diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
index 35fbdb7d6e1a..1648790e91b3 100644
--- a/arch/arm64/lib/strlen.S
+++ b/arch/arm64/lib/strlen.S
@@ -8,6 +8,7 @@
 
 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#include <asm/mte-def.h>
 
 /* Assumptions:
  *
@@ -42,7 +43,16 @@
 #define REP8_7f 0x7f7f7f7f7f7f7f7f
 #define REP8_80 0x8080808080808080
 
+/*
+ * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
+ * (16-byte) granularity, and we must ensure that no access straddles this
+ * alignment boundary.
+ */
+#ifdef CONFIG_KASAN_HW_TAGS
+#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
+#else
 #define MIN_PAGE_SIZE 4096
+#endif
 
 	/* Since strings are short on average, we check the first 16 bytes
 	   of the string for a NUL character.  In order to do an unaligned ldp
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Marco Elver]

On Mon, 12 Jul 2021 at 11:00, Mark Rutland <mark.rutland@arm.com> wrote:
>
> When the kernel is built with CONFIG_KASAN_HW_TAGS and the CPU supports
> MTE, memory accesses are checked at 16-byte granularity, and
> out-of-bounds accesses can result in tag check faults. Our current
> implementation of strlen() makes unaligned 16-byte accesses (within a
> naturally aligned 4096-byte window), and can trigger tag check faults.
>
> This can be seen at boot time, e.g.
>
> | BUG: KASAN: invalid-access in __pi_strlen+0x14/0x150
> | Read at addr f4ff0000c0028300 by task swapper/0/0
> | Pointer tag: [f4], memory tag: [fe]
> |
> | CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09550-g03c2813535a2-dirty #20
> | Hardware name: linux,dummy-virt (DT)
> | Call trace:
> |  dump_backtrace+0x0/0x1b0
> |  show_stack+0x1c/0x30
> |  dump_stack_lvl+0x68/0x84
> |  print_address_description+0x7c/0x2b4
> |  kasan_report+0x138/0x38c
> |  __do_kernel_fault+0x190/0x1c4
> |  do_tag_check_fault+0x78/0x90
> |  do_mem_abort+0x44/0xb4
> |  el1_abort+0x40/0x60
> |  el1h_64_sync_handler+0xb0/0xd0
> |  el1h_64_sync+0x78/0x7c
> |  __pi_strlen+0x14/0x150
> |  __register_sysctl_table+0x7c4/0x890
> |  register_leaf_sysctl_tables+0x1a4/0x210
> |  register_leaf_sysctl_tables+0xc8/0x210
> |  __register_sysctl_paths+0x22c/0x290
> |  register_sysctl_table+0x2c/0x40
> |  sysctl_init+0x20/0x30
> |  proc_sys_init+0x3c/0x48
> |  proc_root_init+0x80/0x9c
> |  start_kernel+0x640/0x69c
> |  __primary_switched+0xc0/0xc8
>
> To fix this, we can reduce the (strlen-internal) MIN_PAGE_SIZE to 16
> bytes when CONFIG_KASAN_HW_TAGS is selected. This will cause strlen() to
> align the base pointer downwards to a 16-byte boundary, and to discard
> the additional prefix bytes without counting them. All subsequent
> accesses will be 16-byte aligned 16-byte LDPs. While the comments say
> the body of the loop will access 32 bytes, this is performed as two
> 16-byte acceses, with the second made only if the first did not
> encounter a NUL byte, so the body of the loop will not over-read across
> a 16-byte boundary.
>
> No other string routines are affected. The other str*() routines will
> not make any access which straddles a 16-byte boundary, and the mem*()
> routines will only make acceses which straddle a 16-byte boundary when
> which is entirely within the bounds of the relevant base and size
> arguments.
>
> Fixes: 325a1de81287a3d4 (“arm64: Import updated version of Cortex Strings' strlen”)
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Potapenko <glider@google.com
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/lib/strlen.S | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> Note: to build-test this, you'll also need to apply a fix from Marco [1], which
> is on its way to mainline via akpm. I've locally aplied that, and tested this
> patch with a recent build of QEMU with MTE enabled.
>
> Mark.
>
> diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
> index 35fbdb7d6e1a..1648790e91b3 100644
> --- a/arch/arm64/lib/strlen.S
> +++ b/arch/arm64/lib/strlen.S
> @@ -8,6 +8,7 @@
>
>  #include <linux/linkage.h>
>  #include <asm/assembler.h>
> +#include <asm/mte-def.h>
>
>  /* Assumptions:
>   *
> @@ -42,7 +43,16 @@
>  #define REP8_7f 0x7f7f7f7f7f7f7f7f
>  #define REP8_80 0x8080808080808080
>
> +/*
> + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
> + * (16-byte) granularity, and we must ensure that no access straddles this
> + * alignment boundary.
> + */
> +#ifdef CONFIG_KASAN_HW_TAGS
> +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
> +#else
>  #define MIN_PAGE_SIZE 4096
> +#endif
>
>         /* Since strings are short on average, we check the first 16 bytes
>            of the string for a NUL character.  In order to do an unaligned ldp

Glancing at the code below this hunk, I see some hard coded 16,
presumably referencing the comment here. Is the algorithm tolerant to
MIN_PAGE_SIZE<16? I suppose that's only an issue if the granule size
might become less than 16 for some config in future.

Thanks,
-- Marco

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Catalin Marinas]

On Mon, Jul 12, 2021 at 11:24:14AM +0200, Marco Elver wrote:
> On Mon, 12 Jul 2021 at 11:00, Mark Rutland <mark.rutland@arm.com> wrote:
> > diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
> > index 35fbdb7d6e1a..1648790e91b3 100644
> > --- a/arch/arm64/lib/strlen.S
> > +++ b/arch/arm64/lib/strlen.S
> > @@ -8,6 +8,7 @@
> >
> >  #include <linux/linkage.h>
> >  #include <asm/assembler.h>
> > +#include <asm/mte-def.h>
> >
> >  /* Assumptions:
> >   *
> > @@ -42,7 +43,16 @@
> >  #define REP8_7f 0x7f7f7f7f7f7f7f7f
> >  #define REP8_80 0x8080808080808080
> >
> > +/*
> > + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
> > + * (16-byte) granularity, and we must ensure that no access straddles this
> > + * alignment boundary.
> > + */
> > +#ifdef CONFIG_KASAN_HW_TAGS
> > +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
> > +#else
> >  #define MIN_PAGE_SIZE 4096
> > +#endif
> >
> >         /* Since strings are short on average, we check the first 16 bytes
> >            of the string for a NUL character.  In order to do an unaligned ldp
> 
> Glancing at the code below this hunk, I see some hard coded 16,
> presumably referencing the comment here. Is the algorithm tolerant to
> MIN_PAGE_SIZE<16? I suppose that's only an issue if the granule size
> might become less than 16 for some config in future.

It's probably not tolerant as we have a cmp with MIN_PAGE_SIZE - 16. But
I'm not worried, the architecture won't go below this without some
explicit opt-in. Anyway, we can add an #error MTE_GRANULE_SIZE < 16,
just in case anyone plays with this macro.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Catalin Marinas]

On Mon, Jul 12, 2021 at 10:00:43AM +0100, Mark Rutland wrote:
> diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
> index 35fbdb7d6e1a..1648790e91b3 100644
> --- a/arch/arm64/lib/strlen.S
> +++ b/arch/arm64/lib/strlen.S
> @@ -8,6 +8,7 @@
>  
>  #include <linux/linkage.h>
>  #include <asm/assembler.h>
> +#include <asm/mte-def.h>
>  
>  /* Assumptions:
>   *
> @@ -42,7 +43,16 @@
>  #define REP8_7f 0x7f7f7f7f7f7f7f7f
>  #define REP8_80 0x8080808080808080
>  
> +/*
> + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
> + * (16-byte) granularity, and we must ensure that no access straddles this
> + * alignment boundary.
> + */
> +#ifdef CONFIG_KASAN_HW_TAGS
> +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
> +#else
>  #define MIN_PAGE_SIZE 4096
> +#endif

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Robin Murphy]

On 2021-07-12 10:00, Mark Rutland wrote:
> When the kernel is built with CONFIG_KASAN_HW_TAGS and the CPU supports
> MTE, memory accesses are checked at 16-byte granularity, and
> out-of-bounds accesses can result in tag check faults. Our current
> implementation of strlen() makes unaligned 16-byte accesses (within a
> naturally aligned 4096-byte window), and can trigger tag check faults.
> 
> This can be seen at boot time, e.g.
> 
> | BUG: KASAN: invalid-access in __pi_strlen+0x14/0x150
> | Read at addr f4ff0000c0028300 by task swapper/0/0
> | Pointer tag: [f4], memory tag: [fe]
> |
> | CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09550-g03c2813535a2-dirty #20
> | Hardware name: linux,dummy-virt (DT)
> | Call trace:
> |  dump_backtrace+0x0/0x1b0
> |  show_stack+0x1c/0x30
> |  dump_stack_lvl+0x68/0x84
> |  print_address_description+0x7c/0x2b4
> |  kasan_report+0x138/0x38c
> |  __do_kernel_fault+0x190/0x1c4
> |  do_tag_check_fault+0x78/0x90
> |  do_mem_abort+0x44/0xb4
> |  el1_abort+0x40/0x60
> |  el1h_64_sync_handler+0xb0/0xd0
> |  el1h_64_sync+0x78/0x7c
> |  __pi_strlen+0x14/0x150
> |  __register_sysctl_table+0x7c4/0x890
> |  register_leaf_sysctl_tables+0x1a4/0x210
> |  register_leaf_sysctl_tables+0xc8/0x210
> |  __register_sysctl_paths+0x22c/0x290
> |  register_sysctl_table+0x2c/0x40
> |  sysctl_init+0x20/0x30
> |  proc_sys_init+0x3c/0x48
> |  proc_root_init+0x80/0x9c
> |  start_kernel+0x640/0x69c
> |  __primary_switched+0xc0/0xc8
> 
> To fix this, we can reduce the (strlen-internal) MIN_PAGE_SIZE to 16
> bytes when CONFIG_KASAN_HW_TAGS is selected. This will cause strlen() to
> align the base pointer downwards to a 16-byte boundary, and to discard
> the additional prefix bytes without counting them. All subsequent
> accesses will be 16-byte aligned 16-byte LDPs. While the comments say
> the body of the loop will access 32 bytes, this is performed as two
> 16-byte acceses, with the second made only if the first did not
> encounter a NUL byte, so the body of the loop will not over-read across
> a 16-byte boundary.
> 
> No other string routines are affected. The other str*() routines will
> not make any access which straddles a 16-byte boundary, and the mem*()
> routines will only make acceses which straddle a 16-byte boundary when
> which is entirely within the bounds of the relevant base and size
> arguments.

Ah, sorry for the oversight - I hadn't appreciated that in-kernel MTE 
was a thing now (and FWIW apparently the last time I refreshed Sam's 
original patches it still wasn't).

We can't use the "official" strlen-mte from Arm Optimized Routines since 
that uses ASIMD, but since I assume KASAN is still expected to have some 
performance penalty in general - even if hardware-assisted - this seems 
like a fair compromise. As for the general 16-byte assumptions, if there 
were ever an MTE4 or whatever with a smaller granule which doesn't break 
the rest of the world anyway, I think we could worry about that then.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Thanks,
Robin.

> Fixes: 325a1de81287a3d4 (“arm64: Import updated version of Cortex Strings' strlen”)
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Potapenko <glider@google.com
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>   arch/arm64/lib/strlen.S | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> Note: to build-test this, you'll also need to apply a fix from Marco [1], which
> is on its way to mainline via akpm. I've locally aplied that, and tested this
> patch with a recent build of QEMU with MTE enabled.
> 
> Mark.
> 
> diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
> index 35fbdb7d6e1a..1648790e91b3 100644
> --- a/arch/arm64/lib/strlen.S
> +++ b/arch/arm64/lib/strlen.S
> @@ -8,6 +8,7 @@
>   
>   #include <linux/linkage.h>
>   #include <asm/assembler.h>
> +#include <asm/mte-def.h>
>   
>   /* Assumptions:
>    *
> @@ -42,7 +43,16 @@
>   #define REP8_7f 0x7f7f7f7f7f7f7f7f
>   #define REP8_80 0x8080808080808080
>   
> +/*
> + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
> + * (16-byte) granularity, and we must ensure that no access straddles this
> + * alignment boundary.
> + */
> +#ifdef CONFIG_KASAN_HW_TAGS
> +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
> +#else
>   #define MIN_PAGE_SIZE 4096
> +#endif
>   
>   	/* Since strings are short on average, we check the first 16 bytes
>   	   of the string for a NUL character.  In order to do an unaligned ldp
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Will Deacon]

On Mon, 12 Jul 2021 10:00:43 +0100, Mark Rutland wrote:
> When the kernel is built with CONFIG_KASAN_HW_TAGS and the CPU supports
> MTE, memory accesses are checked at 16-byte granularity, and
> out-of-bounds accesses can result in tag check faults. Our current
> implementation of strlen() makes unaligned 16-byte accesses (within a
> naturally aligned 4096-byte window), and can trigger tag check faults.
> 
> This can be seen at boot time, e.g.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS
      https://git.kernel.org/arm64/c/5f34b1eb2f8d

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Mark Rutland]

On Mon, Jul 12, 2021 at 12:19:06PM +0100, Catalin Marinas wrote:
> On Mon, Jul 12, 2021 at 11:24:14AM +0200, Marco Elver wrote:
> > On Mon, 12 Jul 2021 at 11:00, Mark Rutland <mark.rutland@arm.com> wrote:
> > > diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
> > > index 35fbdb7d6e1a..1648790e91b3 100644
> > > --- a/arch/arm64/lib/strlen.S
> > > +++ b/arch/arm64/lib/strlen.S
> > > @@ -8,6 +8,7 @@
> > >
> > >  #include <linux/linkage.h>
> > >  #include <asm/assembler.h>
> > > +#include <asm/mte-def.h>
> > >
> > >  /* Assumptions:
> > >   *
> > > @@ -42,7 +43,16 @@
> > >  #define REP8_7f 0x7f7f7f7f7f7f7f7f
> > >  #define REP8_80 0x8080808080808080
> > >
> > > +/*
> > > + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
> > > + * (16-byte) granularity, and we must ensure that no access straddles this
> > > + * alignment boundary.
> > > + */
> > > +#ifdef CONFIG_KASAN_HW_TAGS
> > > +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
> > > +#else
> > >  #define MIN_PAGE_SIZE 4096
> > > +#endif
> > >
> > >         /* Since strings are short on average, we check the first 16 bytes
> > >            of the string for a NUL character.  In order to do an unaligned ldp
> > 
> > Glancing at the code below this hunk, I see some hard coded 16,
> > presumably referencing the comment here. Is the algorithm tolerant to
> > MIN_PAGE_SIZE<16? I suppose that's only an issue if the granule size
> > might become less than 16 for some config in future.

As-is, the algorithm is not tolerant to MIN_PAGE_SIZE<16, since it only
makes 16-byte accesses (an LDP of two 8-byte registers). Even if we
reworked the alignment logic it to say 8 bytes, it would still straddle
8-byte boundaries.

It's possible to create {8,4,2,1} byte versions, but that requires more
significant alteration to the assembly, including hte load instructions
used, the alignment masking, and all the byte checks.

> It's probably not tolerant as we have a cmp with MIN_PAGE_SIZE - 16. But
> I'm not worried, the architecture won't go below this without some
> explicit opt-in. Anyway, we can add an #error MTE_GRANULE_SIZE < 16,
> just in case anyone plays with this macro.

I assumed we'd catch this during review, since I don't see a sensible
reason to shrink this, but I can spin a patch for that if you'd like?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Robin Murphy]

On 2021-07-12 15:03, Mark Rutland wrote:
> On Mon, Jul 12, 2021 at 12:19:06PM +0100, Catalin Marinas wrote:
>> On Mon, Jul 12, 2021 at 11:24:14AM +0200, Marco Elver wrote:
>>> On Mon, 12 Jul 2021 at 11:00, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
>>>> index 35fbdb7d6e1a..1648790e91b3 100644
>>>> --- a/arch/arm64/lib/strlen.S
>>>> +++ b/arch/arm64/lib/strlen.S
>>>> @@ -8,6 +8,7 @@
>>>>
>>>>   #include <linux/linkage.h>
>>>>   #include <asm/assembler.h>
>>>> +#include <asm/mte-def.h>
>>>>
>>>>   /* Assumptions:
>>>>    *
>>>> @@ -42,7 +43,16 @@
>>>>   #define REP8_7f 0x7f7f7f7f7f7f7f7f
>>>>   #define REP8_80 0x8080808080808080
>>>>
>>>> +/*
>>>> + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
>>>> + * (16-byte) granularity, and we must ensure that no access straddles this
>>>> + * alignment boundary.
>>>> + */
>>>> +#ifdef CONFIG_KASAN_HW_TAGS
>>>> +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
>>>> +#else
>>>>   #define MIN_PAGE_SIZE 4096
>>>> +#endif
>>>>
>>>>          /* Since strings are short on average, we check the first 16 bytes
>>>>             of the string for a NUL character.  In order to do an unaligned ldp
>>>
>>> Glancing at the code below this hunk, I see some hard coded 16,
>>> presumably referencing the comment here. Is the algorithm tolerant to
>>> MIN_PAGE_SIZE<16? I suppose that's only an issue if the granule size
>>> might become less than 16 for some config in future.
> 
> As-is, the algorithm is not tolerant to MIN_PAGE_SIZE<16, since it only
> makes 16-byte accesses (an LDP of two 8-byte registers). Even if we
> reworked the alignment logic it to say 8 bytes, it would still straddle
> 8-byte boundaries.
> 
> It's possible to create {8,4,2,1} byte versions, but that requires more
> significant alteration to the assembly, including hte load instructions
> used, the alignment masking, and all the byte checks.
> 
>> It's probably not tolerant as we have a cmp with MIN_PAGE_SIZE - 16. But
>> I'm not worried, the architecture won't go below this without some
>> explicit opt-in. Anyway, we can add an #error MTE_GRANULE_SIZE < 16,
>> just in case anyone plays with this macro.
> 
> I assumed we'd catch this during review, since I don't see a sensible
> reason to shrink this, but I can spin a patch for that if you'd like?

FWIW even the MTE-safe userspace version is still operating on 16-byte 
chunks - AFAICS from skimming the Arm ARM it's a pretty fundamental 
assumption, so I can't see the architecture changing that in a way where 
we wouldn't have to do a bunch of explicit work and catch things like 
this in testing anyway.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix strlen() with CONFIG_KASAN_HW_TAGS

[Andrey Konovalov]

On Mon, Jul 12, 2021 at 11:00 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> When the kernel is built with CONFIG_KASAN_HW_TAGS and the CPU supports
> MTE, memory accesses are checked at 16-byte granularity, and
> out-of-bounds accesses can result in tag check faults. Our current
> implementation of strlen() makes unaligned 16-byte accesses (within a
> naturally aligned 4096-byte window), and can trigger tag check faults.
>
> This can be seen at boot time, e.g.
>
> | BUG: KASAN: invalid-access in __pi_strlen+0x14/0x150
> | Read at addr f4ff0000c0028300 by task swapper/0/0
> | Pointer tag: [f4], memory tag: [fe]
> |
> | CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09550-g03c2813535a2-dirty #20
> | Hardware name: linux,dummy-virt (DT)
> | Call trace:
> |  dump_backtrace+0x0/0x1b0
> |  show_stack+0x1c/0x30
> |  dump_stack_lvl+0x68/0x84
> |  print_address_description+0x7c/0x2b4
> |  kasan_report+0x138/0x38c
> |  __do_kernel_fault+0x190/0x1c4
> |  do_tag_check_fault+0x78/0x90
> |  do_mem_abort+0x44/0xb4
> |  el1_abort+0x40/0x60
> |  el1h_64_sync_handler+0xb0/0xd0
> |  el1h_64_sync+0x78/0x7c
> |  __pi_strlen+0x14/0x150
> |  __register_sysctl_table+0x7c4/0x890
> |  register_leaf_sysctl_tables+0x1a4/0x210
> |  register_leaf_sysctl_tables+0xc8/0x210
> |  __register_sysctl_paths+0x22c/0x290
> |  register_sysctl_table+0x2c/0x40
> |  sysctl_init+0x20/0x30
> |  proc_sys_init+0x3c/0x48
> |  proc_root_init+0x80/0x9c
> |  start_kernel+0x640/0x69c
> |  __primary_switched+0xc0/0xc8
>
> To fix this, we can reduce the (strlen-internal) MIN_PAGE_SIZE to 16
> bytes when CONFIG_KASAN_HW_TAGS is selected. This will cause strlen() to
> align the base pointer downwards to a 16-byte boundary, and to discard
> the additional prefix bytes without counting them. All subsequent
> accesses will be 16-byte aligned 16-byte LDPs. While the comments say
> the body of the loop will access 32 bytes, this is performed as two
> 16-byte acceses, with the second made only if the first did not
> encounter a NUL byte, so the body of the loop will not over-read across
> a 16-byte boundary.
>
> No other string routines are affected. The other str*() routines will
> not make any access which straddles a 16-byte boundary, and the mem*()
> routines will only make acceses which straddle a 16-byte boundary when
> which is entirely within the bounds of the relevant base and size
> arguments.
>
> Fixes: 325a1de81287a3d4 (“arm64: Import updated version of Cortex Strings' strlen”)
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Potapenko <glider@google.com
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/lib/strlen.S | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> Note: to build-test this, you'll also need to apply a fix from Marco [1], which
> is on its way to mainline via akpm. I've locally aplied that, and tested this
> patch with a recent build of QEMU with MTE enabled.
>
> Mark.
>
> diff --git a/arch/arm64/lib/strlen.S b/arch/arm64/lib/strlen.S
> index 35fbdb7d6e1a..1648790e91b3 100644
> --- a/arch/arm64/lib/strlen.S
> +++ b/arch/arm64/lib/strlen.S
> @@ -8,6 +8,7 @@
>
>  #include <linux/linkage.h>
>  #include <asm/assembler.h>
> +#include <asm/mte-def.h>
>
>  /* Assumptions:
>   *
> @@ -42,7 +43,16 @@
>  #define REP8_7f 0x7f7f7f7f7f7f7f7f
>  #define REP8_80 0x8080808080808080
>
> +/*
> + * When KASAN_HW_TAGS is in use, memory is checked at MTE_GRANULE_SIZE
> + * (16-byte) granularity, and we must ensure that no access straddles this
> + * alignment boundary.
> + */
> +#ifdef CONFIG_KASAN_HW_TAGS
> +#define MIN_PAGE_SIZE MTE_GRANULE_SIZE
> +#else
>  #define MIN_PAGE_SIZE 4096
> +#endif
>
>         /* Since strings are short on average, we check the first 16 bytes
>            of the string for a NUL character.  In order to do an unaligned ldp
> --
> 2.11.0
>

This patches the strnlen-realated boot-time crash for me, so:

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

Thank you, Mark!

While running KASAN tests with this fix applied, I noticed random
crashes in the tests. I bisected the issue to 285133040e6c ("arm64:
Import latest memcpy()/memmove() implementation").

However, this appears to be an issue in KASAN tests: some of them do
out-of-bounds and use-after-free writes, which corrupt memory and
cause crashes. Slight changes to KASAN tests to turn OOB/UAF writes
into reads fix the issue. The mentioned commit just exposes it.

I've filed a KASAN bug for this:
https://bugzilla.kernel.org/show_bug.cgi?id=213719

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel