[PATCH v4 0/5] arm64: ARMv8.5-A: MTE: Add async mode support

This patchset implements the asynchronous mode support for ARMv8.5-A
Memory Tagging Extension (MTE), which is a debugging feature that allows
to detect with the help of the architecture the C and C++ programmatic
memory errors like buffer overflow, use-after-free, use-after-return, etc.

MTE is built on top of the AArch64 v8.0 virtual address tagging TBI
(Top Byte Ignore) feature and allows a task to set a 4 bit tag on any
subset of its address space that is multiple of a 16 bytes granule. MTE
is based on a lock-key mechanism where the lock is the tag associated to
the physical memory and the key is the tag associated to the virtual
address.
When MTE is enabled and tags are set for ranges of address space of a task,
the PE will compare the tag related to the physical memory with the tag
related to the virtual address (tag check operation). Access to the memory
is granted only if the two tags match. In case of mismatch the PE will raise
an exception.

The exception can be handled synchronously or asynchronously. When the
asynchronous mode is enabled:
  - Upon fault the PE updates the TFSR_EL1 register.
  - The kernel detects the change during one of the following:
    - Context switching
    - Return to user/EL0
    - Kernel entry from EL1
    - Kernel exit to EL1
  - If the register has been updated by the PE the kernel clears it and
    reports the error.

The series contains as well an optimization to mte_assign_mem_tag_range().

The series is based on linux 5.11-rc3.

To simplify the testing a tree with the new patches on top has been made
available at [1].

[1] https://git.gitlab.arm.com/linux-arm/linux-vf.git mte/v10.async

Changes:
--------
v4:
  - Added support for kasan.mode (sync/async) kernel
    command line parameter.
  - Addressed review comments.
v3:
  - Exposed kasan_hw_tags_mode to convert the internal
    KASAN represenetation.
  - Added dsb() for kernel exit paths in arm64.
  - Addressed review comments.
v2:
  - Fixed a compilation issue reported by krobot.
  - General cleanup.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <redacted>
Cc: Alexander Potapenko <glider@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Evgenii Stepanov <redacted>
Cc: Branislav Rankov <redacted>
Cc: Andrey Konovalov <redacted>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

Vincenzo Frascino (5):
  arm64: mte: Add asynchronous mode support
  kasan: Add KASAN mode kernel parameter
  kasan: Add report for async mode
  arm64: mte: Enable async tag check fault
  arm64: mte: Inline mte_assign_mem_tag_range()

 Documentation/dev-tools/kasan.rst  |  3 ++
 arch/arm64/include/asm/memory.h    |  3 +-
 arch/arm64/include/asm/mte-kasan.h |  9 ++++-
 arch/arm64/include/asm/mte.h       | 58 ++++++++++++++++++++++++++-
 arch/arm64/kernel/entry-common.c   |  6 +++
 arch/arm64/kernel/mte.c            | 63 +++++++++++++++++++++++++++++-
 arch/arm64/lib/mte.S               | 15 -------
 include/linux/kasan.h              |  3 ++
 mm/kasan/hw_tags.c                 | 31 ++++++++++++++-
 mm/kasan/kasan.h                   |  3 +-
 mm/kasan/report.c                  | 16 +++++++-
 11 files changed, 185 insertions(+), 25 deletions(-)

-- 
2.30.0


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

Architectures supported by KASAN HW can provide a sync or async mode of
execution. On an MTE enabled arm64 hw for example this can be identified
with the synchronous or asynchronous tagging mode of execution.
In synchronous mode, an exception is triggered if a tag check fault occurs.
In asynchronous mode, if a tag check fault occurs, the TFSR_EL1 register is
updated asynchronously. The kernel checks the corresponding bits
periodically.

KASAN requires a specific kernel command line parameter to make use of this
hw features.

Add KASAN HW execution mode kernel command line parameter.

Note: This patch adds the kasan.mode kernel parameter and the
sync/async kernel command line options to enable the described features.

Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <redacted>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <redacted>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 Documentation/dev-tools/kasan.rst |  3 +++
 mm/kasan/hw_tags.c                | 31 ++++++++++++++++++++++++++++++-
 mm/kasan/kasan.h                  |  3 ++-
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst
index 1651d961f06a..60ad73c2a33c 100644
--- a/Documentation/dev-tools/kasan.rst
+++ b/Documentation/dev-tools/kasan.rst

@@ -162,6 +162,9 @@ particular KASAN features.
 
 - ``kasan=off`` or ``=on`` controls whether KASAN is enabled (default: ``on``).
 
+- ``kasan.mode=sync`` or ``=async`` controls whether KASAN is configured in
+  synchronous or asynchronous mode of execution (default: ``sync``).
+
 - ``kasan.stacktrace=off`` or ``=on`` disables or enables alloc and free stack
   traces collection (default: ``on`` for ``CONFIG_DEBUG_KERNEL=y``, otherwise
   ``off``).

diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index e529428e7a11..344aeec05d43 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c

@@ -25,6 +25,11 @@ enum kasan_arg {
 	KASAN_ARG_ON,
 };
 
+enum kasan_arg_mode {
+	KASAN_ARG_MODE_SYNC,
+	KASAN_ARG_MODE_ASYNC,
+};
+
 enum kasan_arg_stacktrace {
 	KASAN_ARG_STACKTRACE_DEFAULT,
 	KASAN_ARG_STACKTRACE_OFF,

@@ -38,6 +43,7 @@ enum kasan_arg_fault {
 };
 
 static enum kasan_arg kasan_arg __ro_after_init;
+static enum kasan_arg_mode kasan_arg_mode __ro_after_init;
 static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init;
 static enum kasan_arg_fault kasan_arg_fault __ro_after_init;
 

@@ -68,6 +74,21 @@ static int __init early_kasan_flag(char *arg)
 }
 early_param("kasan", early_kasan_flag);
 
+/* kasan.mode=sync/async */
+static int __init early_kasan_mode(char *arg)
+{
+	/* If arg is not set the default mode is sync */
+	if ((!arg) || !strcmp(arg, "sync"))
+		kasan_arg_mode = KASAN_ARG_MODE_SYNC;
+	else if (!strcmp(arg, "async"))
+		kasan_arg_mode = KASAN_ARG_MODE_ASYNC;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+early_param("kasan.mode", early_kasan_mode);
+
 /* kasan.stacktrace=off/on */
 static int __init early_kasan_flag_stacktrace(char *arg)
 {

@@ -102,6 +123,14 @@ static int __init early_kasan_fault(char *arg)
 }
 early_param("kasan.fault", early_kasan_fault);
 
+static inline void hw_enable_tagging_mode(void)
+{
+	if (kasan_arg_mode == KASAN_ARG_MODE_ASYNC)
+		hw_enable_tagging_async();
+	else
+		hw_enable_tagging_sync();
+}
+
 /* kasan_init_hw_tags_cpu() is called for each CPU. */
 void kasan_init_hw_tags_cpu(void)
 {

@@ -115,7 +144,7 @@ void kasan_init_hw_tags_cpu(void)
 		return;
 
 	hw_init_tags(KASAN_TAG_MAX);
-	hw_enable_tagging();
+	hw_enable_tagging_mode();
 }
 
 /* kasan_init_hw_tags() is called once on boot CPU. */

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index cc4d9e1d49b1..7db7bd42fe97 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h

@@ -284,7 +284,8 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
 #define arch_set_mem_tag_range(addr, size, tag) ((void *)(addr))
 #endif
 
-#define hw_enable_tagging()			arch_enable_tagging()
+#define hw_enable_tagging_sync()		arch_enable_tagging_sync()
+#define hw_enable_tagging_async()		arch_enable_tagging_async()
 #define hw_init_tags(max_tag)			arch_init_tags(max_tag)
 #define hw_get_random_tag()			arch_get_random_tag()
 #define hw_get_mem_tag(addr)			arch_get_mem_tag(addr)

-- 
2.30.0


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

MTE provides an asynchronous mode for detecting tag exceptions. In
particular instead of triggering a fault the arm64 core updates a
register which is checked by the kernel after the asynchronous tag
check fault has occurred.

Add support for MTE asynchronous mode.

The exception handling mechanism will be added with a future patch.

Note: KASAN HW activates async mode via kasan.mode kernel parameter.
The default mode is set to synchronous.
The code that verifies the status of TFSR_EL1 will be added with a
future patch.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/memory.h    |  3 ++-
 arch/arm64/include/asm/mte-kasan.h |  9 +++++++--
 arch/arm64/kernel/mte.c            | 16 ++++++++++++++--
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 18fce223b67b..233d9feec45c 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h

@@ -231,7 +231,8 @@ static inline const void *__tag_set(const void *addr, u8 tag)
 }
 
 #ifdef CONFIG_KASAN_HW_TAGS
-#define arch_enable_tagging()			mte_enable_kernel()
+#define arch_enable_tagging_sync()		mte_enable_kernel_sync()
+#define arch_enable_tagging_async()		mte_enable_kernel_async()
 #define arch_init_tags(max_tag)			mte_init_tags(max_tag)
 #define arch_get_random_tag()			mte_get_random_tag()
 #define arch_get_mem_tag(addr)			mte_get_mem_tag(addr)

diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
index 26349a4b5e2e..9a5e30dbe12a 100644
--- a/arch/arm64/include/asm/mte-kasan.h
+++ b/arch/arm64/include/asm/mte-kasan.h

@@ -29,7 +29,8 @@ u8 mte_get_mem_tag(void *addr);
 u8 mte_get_random_tag(void);
 void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag);
 
-void mte_enable_kernel(void);
+void mte_enable_kernel_sync(void);
+void mte_enable_kernel_async(void);
 void mte_init_tags(u64 max_tag);
 
 #else /* CONFIG_ARM64_MTE */

@@ -52,7 +53,11 @@ static inline void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
 	return addr;
 }
 
-static inline void mte_enable_kernel(void)
+static inline void mte_enable_kernel_sync(void)
+{
+}
+
+static inline void mte_enable_kernel_sync(void)
 {
 }
 

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index dc9ada64feed..78fc079a3b1e 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -151,11 +151,23 @@ void mte_init_tags(u64 max_tag)
 	write_sysreg_s(SYS_GCR_EL1_RRND | gcr_kernel_excl, SYS_GCR_EL1);
 }
 
-void mte_enable_kernel(void)
+static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
 {
 	/* Enable MTE Sync Mode for EL1. */
-	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
+	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, tcf);
 	isb();
+
+	pr_info_once("MTE: enabled in %s mode at EL1\n", mode);
+}
+
+void mte_enable_kernel_sync(void)
+{
+	__mte_enable_kernel("synchronous", SCTLR_ELx_TCF_SYNC);
+}
+
+void mte_enable_kernel_async(void)
+{
+	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
 }
 
 static void update_sctlr_el1_tcf0(u64 tcf0)

-- 
2.30.0


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

KASAN provides an asynchronous mode of execution.

Add reporting functionality for this mode.

Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <redacted>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <redacted>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 include/linux/kasan.h |  3 +++
 mm/kasan/report.c     | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index fe1ae73ff8b5..8f43836ccdac 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h

@@ -336,6 +336,9 @@ static inline void *kasan_reset_tag(const void *addr)
 bool kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 
+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip);
+
 #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */
 
 static inline void *kasan_reset_tag(const void *addr)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index c0fb21797550..946016ead6a9 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c

@@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write,
 	start_report(&flags);
 
 	print_error_description(&info);
-	if (addr_has_metadata(untagged_addr))
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0))
 		print_tags(get_tag(tagged_addr), info.first_bad_addr);
 	pr_err("\n");
 
-	if (addr_has_metadata(untagged_addr)) {
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) {
 		print_address_description(untagged_addr, get_tag(tagged_addr));
 		pr_err("\n");
 		print_memory_metadata(info.first_bad_addr);

@@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write,
 	return ret;
 }
 
+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip)
+{
+	pr_info("==================================================================\n");
+	pr_info("KASAN: set in asynchronous mode\n");
+	pr_info("KASAN: some information might not be accurate\n");
+	pr_info("KASAN: fault address is ignored\n");
+	pr_info("KASAN: write/read distinction is ignored\n");
+
+	return kasan_report(addr, size, is_write, ip);
+}
+
 #ifdef CONFIG_KASAN_INLINE
 /*
  * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high

-- 
2.30.0


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

MTE provides a mode that asynchronously updates the TFSR_EL1 register
when a tag check exception is detected.

To take advantage of this mode the kernel has to verify the status of
the register at:
  1. Context switching
  2. Return to user/EL0 (Not required in entry from EL0 since the kernel
  did not run)
  3. Kernel entry from EL1
  4. Kernel exit to EL1

If the register is non-zero a trace is reported.

Add the required features for EL1 detection and reporting.

Note: ITFSB bit is set in the SCTLR_EL1 register hence it guaranties that
the indirect writes to TFSR_EL1 are synchronized at exception entry to
EL1. On the context switch path the synchronization is guarantied by the
dsb() in __switch_to().
The dsb(nsh) in mte_check_tfsr_exit() is provisional pending
confirmation by the architects.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h     | 32 ++++++++++++++++++++++
 arch/arm64/kernel/entry-common.c |  6 ++++
 arch/arm64/kernel/mte.c          | 47 ++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index d02aff9f493d..237bb2f7309d 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h

@@ -92,5 +92,37 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
 
 #endif /* CONFIG_ARM64_MTE */
 
+#ifdef CONFIG_KASAN_HW_TAGS
+void mte_check_tfsr_el1(void);
+
+static inline void mte_check_tfsr_entry(void)
+{
+	mte_check_tfsr_el1();
+}
+
+static inline void mte_check_tfsr_exit(void)
+{
+	/*
+	 * The asynchronous faults are sync'ed automatically with
+	 * TFSR_EL1 on kernel entry but for exit an explicit dsb()
+	 * is required.
+	 */
+	dsb(nsh);
+	isb();
+
+	mte_check_tfsr_el1();
+}
+#else
+static inline void mte_check_tfsr_el1(void)
+{
+}
+static inline void mte_check_tfsr_entry(void)
+{
+}
+static inline void mte_check_tfsr_exit(void)
+{
+}
+#endif /* CONFIG_KASAN_HW_TAGS */
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_MTE_H  */

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 5346953e4382..31666511ba67 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c

@@ -37,6 +37,8 @@ static void noinstr enter_from_kernel_mode(struct pt_regs *regs)
 	lockdep_hardirqs_off(CALLER_ADDR0);
 	rcu_irq_enter_check_tick();
 	trace_hardirqs_off_finish();
+
+	mte_check_tfsr_entry();
 }
 
 /*

@@ -47,6 +49,8 @@ static void noinstr exit_to_kernel_mode(struct pt_regs *regs)
 {
 	lockdep_assert_irqs_disabled();
 
+	mte_check_tfsr_exit();
+
 	if (interrupts_enabled(regs)) {
 		if (regs->exit_rcu) {
 			trace_hardirqs_on_prepare();

@@ -243,6 +247,8 @@ asmlinkage void noinstr enter_from_user_mode(void)
 
 asmlinkage void noinstr exit_to_user_mode(void)
 {
+	mte_check_tfsr_exit();
+
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	user_enter_irqoff();

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 78fc079a3b1e..0a9cc82a5301 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -170,6 +170,44 @@ void mte_enable_kernel_async(void)
 	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
 }
 
+#ifdef CONFIG_KASAN_HW_TAGS
+static inline void mte_report_async(void)
+{
+	u64 pc = (u64)__builtin_return_address(0);
+
+	kasan_report_async(0, 0, false, pc);
+}
+
+void mte_check_tfsr_el1(void)
+{
+	u64 tfsr_el1;
+
+	if (!system_supports_mte())
+		return;
+
+	tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
+
+	/*
+	 * The kernel should never trigger an asynchronous fault on a
+	 * TTBR0 address, so we should never see TF0 set.
+	 * For futexes we disable checks via PSTATE.TCO.
+	 */
+	WARN_ONCE(tfsr_el1 & SYS_TFSR_EL1_TF0,
+		  "Kernel async tag fault on TTBR0 address");
+
+	if (unlikely(tfsr_el1 & SYS_TFSR_EL1_TF1)) {
+		/*
+		 * Note: isb() is not required after this direct write
+		 * because there is no indirect read subsequent to it
+		 * (per ARM DDI 0487F.c table D13-1).
+		 */
+		write_sysreg_s(0, SYS_TFSR_EL1);
+
+		mte_report_async();
+	}
+}
+#endif
+
 static void update_sctlr_el1_tcf0(u64 tcf0)
 {
 	/* ISB required for the kernel uaccess routines */

@@ -235,6 +273,15 @@ void mte_thread_switch(struct task_struct *next)
 	/* avoid expensive SCTLR_EL1 accesses if no change */
 	if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0)
 		update_sctlr_el1_tcf0(next->thread.sctlr_tcf0);
+
+	/*
+	 * Check if an async tag exception occurred at EL1.
+	 *
+	 * Note: On the context switch path we rely on the dsb() present
+	 * in __switch_to() to guarantee that the indirect writes to TFSR_EL1
+	 * are synchronized before this point.
+	 */
+	mte_check_tfsr_el1();
 }
 
 void mte_suspend_exit(void)

-- 
2.30.0


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

mte_assign_mem_tag_range() is called on production KASAN HW hot
paths. It makes sense to inline it in an attempt to reduce the
overhead.

Inline mte_assign_mem_tag_range() based on the indications provided at
[1].

[1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
 arch/arm64/lib/mte.S         | 15 ---------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 237bb2f7309d..1a6fd53f82c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h

@@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
 			 unsigned long addr, unsigned long data);
 
-void mte_assign_mem_tag_range(void *addr, size_t size);
+static inline void mte_assign_mem_tag_range(void *addr, size_t size)
+{
+	u64 _addr = (u64)addr;
+	u64 _end = _addr + size;
+
+	/*
+	 * This function must be invoked from an MTE enabled context.
+	 *
+	 * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
+	 * size must be non-zero and MTE_GRANULE_SIZE aligned.
+	 */
+	do {
+		/*
+		 * 'asm volatile' is required to prevent the compiler to move
+		 * the statement outside of the loop.
+		 */
+		asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
+			     :
+			     : "r" (_addr)
+			     : "memory");
+
+		_addr += MTE_GRANULE_SIZE;
+	} while (_addr != _end);
+}
+
 
 #else /* CONFIG_ARM64_MTE */
 

diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
index 9e1a12e10053..a0a650451510 100644
--- a/arch/arm64/lib/mte.S
+++ b/arch/arm64/lib/mte.S

@@ -150,18 +150,3 @@ SYM_FUNC_START(mte_restore_page_tags)
 	ret
 SYM_FUNC_END(mte_restore_page_tags)
 
-/*
- * Assign allocation tags for a region of memory based on the pointer tag
- *   x0 - source pointer
- *   x1 - size
- *
- * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
- * size must be non-zero and MTE_GRANULE_SIZE aligned.
- */
-SYM_FUNC_START(mte_assign_mem_tag_range)
-1:	stg	x0, [x0]
-	add	x0, x0, #MTE_GRANULE_SIZE
-	subs	x1, x1, #MTE_GRANULE_SIZE
-	b.gt	1b
-	ret
-SYM_FUNC_END(mte_assign_mem_tag_range)

-- 
2.30.0


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

On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino wrote:

We have no address, no size and no is_write information. Do we have a
reason to pass all these arguments here? Not sure what SPARC ADI does
but they may not have all this information either. We can pass ip as the
point where we checked the TFSR reg but that's about it.

So just call kasan_report (0, 0, 0, ip) here.

-- 
Catalin

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

On Mon, Jan 18, 2021 at 06:30:29PM +0000, Vincenzo Frascino wrote:

MTE provides an asynchronous mode for detecting tag exceptions. In
particular instead of triggering a fault the arm64 core updates a
register which is checked by the kernel after the asynchronous tag
check fault has occurred.

Add support for MTE asynchronous mode.

The exception handling mechanism will be added with a future patch.

Note: KASAN HW activates async mode via kasan.mode kernel parameter.
The default mode is set to synchronous.
The code that verifies the status of TFSR_EL1 will be added with a
future patch.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

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

On 1/19/21 2:34 PM, Catalin Marinas wrote:

On Mon, Jan 18, 2021 at 06:30:32PM +0000, Vincenzo Frascino wrote:

quoted

 static void update_sctlr_el1_tcf0(u64 tcf0)
 {
 	/* ISB required for the kernel uaccess routines */

@@ -235,6 +273,15 @@ void mte_thread_switch(struct task_struct *next)
 	/* avoid expensive SCTLR_EL1 accesses if no change */
 	if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0)
 		update_sctlr_el1_tcf0(next->thread.sctlr_tcf0);
+
+	/*
+	 * Check if an async tag exception occurred at EL1.
+	 *
+	 * Note: On the context switch path we rely on the dsb() present
+	 * in __switch_to() to guarantee that the indirect writes to TFSR_EL1
+	 * are synchronized before this point.
+	 */
+	mte_check_tfsr_el1();
 }

We need an isb() before mte_check_tfsr_el1() here as well, we only have
a dsb() in __switch_to(). We do have an isb() in update_sctlr_el1_tcf0()
but only if the check passed. Now, it's worth benchmarking how expensive
update_sctlr_el1_tcf0() is (i.e. an SCTLR_EL1 access + isb with
something like hackbench) and we could probably remove the check
altogether. In the meantime, you can add an isb() on the "else" path of
the above check.

Good catch, I saw the isb() in update_sctlr_el1_tcf0() and for some reasons that
it is not escaping me I thought it was sufficient, but clearly it is not.

I am happy to benchmark what you are suggesting and provide some data after this
series is merged (if it works for you) so that we can decide. In the meantime as
you suggested I will fix the "else" for v5.

-- 
Regards,
Vincenzo

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

On Mon, Jan 18, 2021 at 06:30:33PM +0000, Vincenzo Frascino wrote:

While I'm ok with moving this function to C, I don't think it solves the
inlining in the kasan code. The only interface we have to kasan is via
mte_{set,get}_mem_tag_range(), so the above function doesn't need to
live in a header.

If you do want inlining all the way to the kasan code, we should
probably move the mte_{set,get}_mem_tag_range() functions to the header
as well (and ideally backed by some numbers to show that it matters).

Moving it to mte.c also gives us more control on how it's called (we
have the WARN_ONs in place in the callers).

-- 
Catalin

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

On Tue, Jan 19, 2021 at 02:23:03PM +0000, Vincenzo Frascino wrote:

On 1/19/21 1:04 PM, Catalin Marinas wrote:

quoted

On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino wrote:

quoted

quoted

+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip);

We have no address, no size and no is_write information. Do we have a
reason to pass all these arguments here? Not sure what SPARC ADI does
but they may not have all this information either. We can pass ip as the
point where we checked the TFSR reg but that's about it.

I kept the interface generic for future development and mainly to start a
discussion. I do not have a strong opinion either way. If Andrey agrees as well
I am happy to change it to what you are suggesting in v5.

For now, I think it's preferable that this only has parameters that we
can actually provide. That way it's clearer what's going on in both
callers and callees, and we can always rework the prototype later or add
separate variants of the function that can take additional parameters.

I don't think we even need to use __kasan_report() -- more on that
below.

[...]

quoted

quoted

@@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write,
 	start_report(&flags);
 
 	print_error_description(&info);
-	if (addr_has_metadata(untagged_addr))
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0))
 		print_tags(get_tag(tagged_addr), info.first_bad_addr);
 	pr_err("\n");
 
-	if (addr_has_metadata(untagged_addr)) {
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) {
 		print_address_description(untagged_addr, get_tag(tagged_addr));
 		pr_err("\n");
 		print_memory_metadata(info.first_bad_addr);

@@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write,
 	return ret;
 }
 
+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip)
+{
+	pr_info("==================================================================\n");
+	pr_info("KASAN: set in asynchronous mode\n");
+	pr_info("KASAN: some information might not be accurate\n");
+	pr_info("KASAN: fault address is ignored\n");
+	pr_info("KASAN: write/read distinction is ignored\n");
+
+	return kasan_report(addr, size, is_write, ip);

So just call kasan_report (0, 0, 0, ip) here.

Given there's no information available, I think it's simpler and
preferable to handle the logging separately, as is done for
kasan_report_invalid_free(). For example, we could do something roughly
like:

void kasan_report_async(void)
{
	unsigned long flags;

	start_report(&flags);
	pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n");
	pr_err("KASAN: no fault information available\n");
	dump_stack();
	end_report(&flags);
}

... which is easier to consume, since there's no misleading output,
avoids complicating the synchronous reporting path, and we could
consider adding information that's only of use for debugging
asynchronous faults here.

Since the callside is logged in the backtrace, we don't even need the
synthetic IP parameter.

Thanks,
Mark.

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

Hi Catalin,

On 1/19/21 2:45 PM, Catalin Marinas wrote:

On Mon, Jan 18, 2021 at 06:30:33PM +0000, Vincenzo Frascino wrote:

quoted

mte_assign_mem_tag_range() is called on production KASAN HW hot
paths. It makes sense to inline it in an attempt to reduce the
overhead.

Inline mte_assign_mem_tag_range() based on the indications provided at
[1].

[1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
 arch/arm64/lib/mte.S         | 15 ---------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 237bb2f7309d..1a6fd53f82c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h

@@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
 			 unsigned long addr, unsigned long data);
 
-void mte_assign_mem_tag_range(void *addr, size_t size);
+static inline void mte_assign_mem_tag_range(void *addr, size_t size)
+{
+	u64 _addr = (u64)addr;
+	u64 _end = _addr + size;
+
+	/*
+	 * This function must be invoked from an MTE enabled context.
+	 *
+	 * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
+	 * size must be non-zero and MTE_GRANULE_SIZE aligned.
+	 */
+	do {
+		/*
+		 * 'asm volatile' is required to prevent the compiler to move
+		 * the statement outside of the loop.
+		 */
+		asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
+			     :
+			     : "r" (_addr)
+			     : "memory");
+
+		_addr += MTE_GRANULE_SIZE;
+	} while (_addr != _end);
+}

While I'm ok with moving this function to C, I don't think it solves the
inlining in the kasan code. The only interface we have to kasan is via
mte_{set,get}_mem_tag_range(), so the above function doesn't need to
live in a header.

If you do want inlining all the way to the kasan code, we should
probably move the mte_{set,get}_mem_tag_range() functions to the header
as well (and ideally backed by some numbers to show that it matters).

Moving it to mte.c also gives us more control on how it's called (we
have the WARN_ONs in place in the callers).

Based on the thread [1] this patch contains only an intermediate step to allow
KASAN to call directly mte_assign_mem_tag_range() in future. At that point I
think that mte_set_mem_tag_range() can be removed.

If you agree, I would live the things like this to give to Andrey a chance to
execute on the original plan with a separate series.

I agree though that this change alone does not bring huge benefits but
regressions neither.

If you want I can add something to the commit message in the next version to
make this more explicit.

Let me know how do you want me to proceed.

-- 
Regards,
Vincenzo

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

On 1/19/21 2:46 PM, Mark Rutland wrote:

On Tue, Jan 19, 2021 at 02:23:03PM +0000, Vincenzo Frascino wrote:

quoted

On 1/19/21 1:04 PM, Catalin Marinas wrote:

quoted

On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino wrote:

quoted

quoted

quoted

+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip);

We have no address, no size and no is_write information. Do we have a
reason to pass all these arguments here? Not sure what SPARC ADI does
but they may not have all this information either. We can pass ip as the
point where we checked the TFSR reg but that's about it.

I kept the interface generic for future development and mainly to start a
discussion. I do not have a strong opinion either way. If Andrey agrees as well
I am happy to change it to what you are suggesting in v5.

For now, I think it's preferable that this only has parameters that we
can actually provide. That way it's clearer what's going on in both
callers and callees, and we can always rework the prototype later or add
separate variants of the function that can take additional parameters.

I don't think we even need to use __kasan_report() -- more on that
below.

[...]

quoted

quoted

quoted

@@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write,
 	start_report(&flags);
 
 	print_error_description(&info);
-	if (addr_has_metadata(untagged_addr))
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0))
 		print_tags(get_tag(tagged_addr), info.first_bad_addr);
 	pr_err("\n");
 
-	if (addr_has_metadata(untagged_addr)) {
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) {
 		print_address_description(untagged_addr, get_tag(tagged_addr));
 		pr_err("\n");
 		print_memory_metadata(info.first_bad_addr);

@@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write,
 	return ret;
 }
 
+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip)
+{
+	pr_info("==================================================================\n");
+	pr_info("KASAN: set in asynchronous mode\n");
+	pr_info("KASAN: some information might not be accurate\n");
+	pr_info("KASAN: fault address is ignored\n");
+	pr_info("KASAN: write/read distinction is ignored\n");
+
+	return kasan_report(addr, size, is_write, ip);

So just call kasan_report (0, 0, 0, ip) here.

Given there's no information available, I think it's simpler and
preferable to handle the logging separately, as is done for
kasan_report_invalid_free(). For example, we could do something roughly
like:

void kasan_report_async(void)
{
	unsigned long flags;

	start_report(&flags);
	pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n");
	pr_err("KASAN: no fault information available\n");
	dump_stack();
	end_report(&flags);
}

... which is easier to consume, since there's no misleading output,
avoids complicating the synchronous reporting path, and we could
consider adding information that's only of use for debugging
asynchronous faults here.

Since the callside is logged in the backtrace, we don't even need the
synthetic IP parameter.

Agree, especially because I tend to not like to rely on compiler builtins and
what you proposed solves the problem ;)

I will refactor my code once Andrey had a chance to take a look as well.

Thanks,
Mark.

-- 
Regards,
Vincenzo

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

On Tue, Jan 19, 2021 at 4:45 PM Vincenzo Frascino
[off-list ref] wrote:

Hi Catalin,

On 1/19/21 2:45 PM, Catalin Marinas wrote:

quoted

On Mon, Jan 18, 2021 at 06:30:33PM +0000, Vincenzo Frascino wrote:

quoted

mte_assign_mem_tag_range() is called on production KASAN HW hot
paths. It makes sense to inline it in an attempt to reduce the
overhead.

Inline mte_assign_mem_tag_range() based on the indications provided at
[1].

[1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
 arch/arm64/lib/mte.S         | 15 ---------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 237bb2f7309d..1a6fd53f82c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h

@@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
                      unsigned long addr, unsigned long data);

-void mte_assign_mem_tag_range(void *addr, size_t size);
+static inline void mte_assign_mem_tag_range(void *addr, size_t size)
+{
+    u64 _addr = (u64)addr;
+    u64 _end = _addr + size;
+
+    /*
+     * This function must be invoked from an MTE enabled context.
+     *
+     * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
+     * size must be non-zero and MTE_GRANULE_SIZE aligned.
+     */
+    do {
+            /*
+             * 'asm volatile' is required to prevent the compiler to move
+             * the statement outside of the loop.
+             */
+            asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
+                         :
+                         : "r" (_addr)
+                         : "memory");
+
+            _addr += MTE_GRANULE_SIZE;
+    } while (_addr != _end);
+}

While I'm ok with moving this function to C, I don't think it solves the
inlining in the kasan code. The only interface we have to kasan is via
mte_{set,get}_mem_tag_range(), so the above function doesn't need to
live in a header.

If you do want inlining all the way to the kasan code, we should
probably move the mte_{set,get}_mem_tag_range() functions to the header
as well (and ideally backed by some numbers to show that it matters).

Moving it to mte.c also gives us more control on how it's called (we
have the WARN_ONs in place in the callers).

Based on the thread [1] this patch contains only an intermediate step to allow
KASAN to call directly mte_assign_mem_tag_range() in future. At that point I
think that mte_set_mem_tag_range() can be removed.

If you agree, I would live the things like this to give to Andrey a chance to
execute on the original plan with a separate series.

I think we should drop this patch from this series as it's unrelated.

I will pick it up into my future optimization series. Then it will be
easier to discuss it in the context. The important part that I needed
is an inlinable C implementation of mte_assign_mem_tag_range(), which
I now have with this patch.

Thanks, Vincenzo!

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

On Tue, Jan 19, 2021 at 3:46 PM Mark Rutland [off-list ref] wrote:

Given there's no information available, I think it's simpler and
preferable to handle the logging separately, as is done for
kasan_report_invalid_free(). For example, we could do something roughly
like:

void kasan_report_async(void)
{
        unsigned long flags;

        start_report(&flags);
        pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n");

"BUG: KASAN: invalid-access"

It also might make sense to pass the ip, even though it's not exactly
related to the access:

pr_err("BUG: KASAN: invalid-access in %pS\n", (void *)ip);

Up to you.

        pr_err("KASAN: no fault information available\n");

pr_err("Asynchronous mode enabled: no access details available\n");

        dump_stack();
        end_report(&flags);
}

This approach with a dedicated function is better. Thanks, Mark!

Please put it next to kasan_report_invalid_free().

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

On Mon, Jan 18, 2021 at 7:30 PM Vincenzo Frascino
[off-list ref] wrote:

This needs to be expanded with a short explanation of the difference.

+static inline void hw_enable_tagging_mode(void)
+{
+       if (kasan_arg_mode == KASAN_ARG_MODE_ASYNC)
+               hw_enable_tagging_async();
+       else
+               hw_enable_tagging_sync();
+}

It's OK to open-code this in kasan_init_hw_tags_cpu(), no need for an
additional function.

This is one of the places that conflicts with the testing patches.
You'll need to: add an else case definition of
hw_enable_tagging_sync(); change lib/test_kasan.c to use
hw_enable_tagging_sync().

I'll later add a patch on top that forbids running the tests with the
async mode.

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

On Mon, Jan 18, 2021 at 7:30 PM Vincenzo Frascino
[off-list ref] wrote:

Reviewed-by: Andrey Konovalov <redacted>

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

On Mon, Jan 18, 2021 at 7:30 PM Vincenzo Frascino
[off-list ref] wrote:

This patchset implements the asynchronous mode support for ARMv8.5-A
Memory Tagging Extension (MTE), which is a debugging feature that allows
to detect with the help of the architecture the C and C++ programmatic
memory errors like buffer overflow, use-after-free, use-after-return, etc.

MTE is built on top of the AArch64 v8.0 virtual address tagging TBI
(Top Byte Ignore) feature and allows a task to set a 4 bit tag on any
subset of its address space that is multiple of a 16 bytes granule. MTE
is based on a lock-key mechanism where the lock is the tag associated to
the physical memory and the key is the tag associated to the virtual
address.
When MTE is enabled and tags are set for ranges of address space of a task,
the PE will compare the tag related to the physical memory with the tag
related to the virtual address (tag check operation). Access to the memory
is granted only if the two tags match. In case of mismatch the PE will raise
an exception.

The exception can be handled synchronously or asynchronously. When the
asynchronous mode is enabled:
  - Upon fault the PE updates the TFSR_EL1 register.
  - The kernel detects the change during one of the following:
    - Context switching
    - Return to user/EL0
    - Kernel entry from EL1
    - Kernel exit to EL1
  - If the register has been updated by the PE the kernel clears it and
    reports the error.

The series contains as well an optimization to mte_assign_mem_tag_range().

The series is based on linux 5.11-rc3.

To simplify the testing a tree with the new patches on top has been made
available at [1].

[1] https://git.gitlab.arm.com/linux-arm/linux-vf.git mte/v10.async

Hi Vincenzo,

This change has multiple conflicts with the KASAN testing patches that
are currently in the mm tree. If Andrew decides to send all of them
during RC, then this should be good to go through arm64. Otherwise, I
guess this will need to go through mm as well. So you probably need to
rebase this on top of those patches in any case.

Thanks!

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

On Tue, Jan 19, 2021 at 07:12:40PM +0100, Andrey Konovalov wrote:

On Tue, Jan 19, 2021 at 4:45 PM Vincenzo Frascino
[off-list ref] wrote:

quoted

On 1/19/21 2:45 PM, Catalin Marinas wrote:

quoted

On Mon, Jan 18, 2021 at 06:30:33PM +0000, Vincenzo Frascino wrote:

quoted

mte_assign_mem_tag_range() is called on production KASAN HW hot
paths. It makes sense to inline it in an attempt to reduce the
overhead.

Inline mte_assign_mem_tag_range() based on the indications provided at
[1].

[1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
 arch/arm64/lib/mte.S         | 15 ---------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 237bb2f7309d..1a6fd53f82c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h

@@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
                      unsigned long addr, unsigned long data);

-void mte_assign_mem_tag_range(void *addr, size_t size);
+static inline void mte_assign_mem_tag_range(void *addr, size_t size)
+{
+    u64 _addr = (u64)addr;
+    u64 _end = _addr + size;
+
+    /*
+     * This function must be invoked from an MTE enabled context.
+     *
+     * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
+     * size must be non-zero and MTE_GRANULE_SIZE aligned.
+     */
+    do {
+            /*
+             * 'asm volatile' is required to prevent the compiler to move
+             * the statement outside of the loop.
+             */
+            asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
+                         :
+                         : "r" (_addr)
+                         : "memory");
+
+            _addr += MTE_GRANULE_SIZE;
+    } while (_addr != _end);
+}

While I'm ok with moving this function to C, I don't think it solves the
inlining in the kasan code. The only interface we have to kasan is via
mte_{set,get}_mem_tag_range(), so the above function doesn't need to
live in a header.

If you do want inlining all the way to the kasan code, we should
probably move the mte_{set,get}_mem_tag_range() functions to the header
as well (and ideally backed by some numbers to show that it matters).

Moving it to mte.c also gives us more control on how it's called (we
have the WARN_ONs in place in the callers).

Based on the thread [1] this patch contains only an intermediate step to allow
KASAN to call directly mte_assign_mem_tag_range() in future. At that point I
think that mte_set_mem_tag_range() can be removed.

If you agree, I would live the things like this to give to Andrey a chance to
execute on the original plan with a separate series.

I think we should drop this patch from this series as it's unrelated.

I will pick it up into my future optimization series. Then it will be
easier to discuss it in the context. The important part that I needed
is an inlinable C implementation of mte_assign_mem_tag_range(), which
I now have with this patch.

That's fine by me but we may want to add some forced-alignment on the
addr and size as the loop here depends on them being aligned, otherwise
it gets stuck. The mte_set_mem_tag_range() at least had a WARN_ON in
place. Here we could do:

	addr &= MTE_GRANULE_MASK;
	size = ALIGN(size, MTE_GRANULE_SIZE);

(or maybe trim "size" with MTE_GRANULE_MASK)

That's unless the call places are well known and guarantee this
alignment (only had a very brief look).

-- 
Catalin

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

On Tue, Jan 19, 2021 at 8:00 PM Catalin Marinas [off-list ref] wrote:

On Tue, Jan 19, 2021 at 07:12:40PM +0100, Andrey Konovalov wrote:

quoted

On Tue, Jan 19, 2021 at 4:45 PM Vincenzo Frascino
[off-list ref] wrote:

quoted

On 1/19/21 2:45 PM, Catalin Marinas wrote:

quoted

On Mon, Jan 18, 2021 at 06:30:33PM +0000, Vincenzo Frascino wrote:

quoted

mte_assign_mem_tag_range() is called on production KASAN HW hot
paths. It makes sense to inline it in an attempt to reduce the
overhead.

Inline mte_assign_mem_tag_range() based on the indications provided at
[1].

[1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
 arch/arm64/lib/mte.S         | 15 ---------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 237bb2f7309d..1a6fd53f82c3 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h

@@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
                      unsigned long addr, unsigned long data);

-void mte_assign_mem_tag_range(void *addr, size_t size);
+static inline void mte_assign_mem_tag_range(void *addr, size_t size)
+{
+    u64 _addr = (u64)addr;
+    u64 _end = _addr + size;
+
+    /*
+     * This function must be invoked from an MTE enabled context.
+     *
+     * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
+     * size must be non-zero and MTE_GRANULE_SIZE aligned.
+     */
+    do {
+            /*
+             * 'asm volatile' is required to prevent the compiler to move
+             * the statement outside of the loop.
+             */
+            asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
+                         :
+                         : "r" (_addr)
+                         : "memory");
+
+            _addr += MTE_GRANULE_SIZE;
+    } while (_addr != _end);
+}

While I'm ok with moving this function to C, I don't think it solves the
inlining in the kasan code. The only interface we have to kasan is via
mte_{set,get}_mem_tag_range(), so the above function doesn't need to
live in a header.

If you do want inlining all the way to the kasan code, we should
probably move the mte_{set,get}_mem_tag_range() functions to the header
as well (and ideally backed by some numbers to show that it matters).

Moving it to mte.c also gives us more control on how it's called (we
have the WARN_ONs in place in the callers).

Based on the thread [1] this patch contains only an intermediate step to allow
KASAN to call directly mte_assign_mem_tag_range() in future. At that point I
think that mte_set_mem_tag_range() can be removed.

If you agree, I would live the things like this to give to Andrey a chance to
execute on the original plan with a separate series.

I think we should drop this patch from this series as it's unrelated.

I will pick it up into my future optimization series. Then it will be
easier to discuss it in the context. The important part that I needed
is an inlinable C implementation of mte_assign_mem_tag_range(), which
I now have with this patch.

That's fine by me but we may want to add some forced-alignment on the
addr and size as the loop here depends on them being aligned, otherwise
it gets stuck. The mte_set_mem_tag_range() at least had a WARN_ON in
place. Here we could do:

        addr &= MTE_GRANULE_MASK;
        size = ALIGN(size, MTE_GRANULE_SIZE);

(or maybe trim "size" with MTE_GRANULE_MASK)

That's unless the call places are well known and guarantee this
alignment (only had a very brief look).

No problem. I'll either add the ALIGN or change the call site to
ensure alignment.

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

On Mon, Jan 18, 2021 at 06:30:32PM +0000, Vincenzo Frascino wrote:

We need an isb() before mte_check_tfsr_el1() here as well, we only have
a dsb() in __switch_to(). We do have an isb() in update_sctlr_el1_tcf0()
but only if the check passed. Now, it's worth benchmarking how expensive
update_sctlr_el1_tcf0() is (i.e. an SCTLR_EL1 access + isb with
something like hackbench) and we could probably remove the check
altogether. In the meantime, you can add an isb() on the "else" path of
the above check.

-- 
Catalin

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

On 1/19/21 1:04 PM, Catalin Marinas wrote:

On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino wrote:

quoted

KASAN provides an asynchronous mode of execution.

Add reporting functionality for this mode.

Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <redacted>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <redacted>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 include/linux/kasan.h |  3 +++
 mm/kasan/report.c     | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index fe1ae73ff8b5..8f43836ccdac 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h

@@ -336,6 +336,9 @@ static inline void *kasan_reset_tag(const void *addr)
 bool kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 
+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip);

We have no address, no size and no is_write information. Do we have a
reason to pass all these arguments here? Not sure what SPARC ADI does
but they may not have all this information either. We can pass ip as the
point where we checked the TFSR reg but that's about it.

I kept the interface generic for future development and mainly to start a
discussion. I do not have a strong opinion either way. If Andrey agrees as well
I am happy to change it to what you are suggesting in v5.

quoted

+
 #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */
 
 static inline void *kasan_reset_tag(const void *addr)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index c0fb21797550..946016ead6a9 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c

@@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write,
 	start_report(&flags);
 
 	print_error_description(&info);
-	if (addr_has_metadata(untagged_addr))
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0))
 		print_tags(get_tag(tagged_addr), info.first_bad_addr);
 	pr_err("\n");
 
-	if (addr_has_metadata(untagged_addr)) {
+	if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) {
 		print_address_description(untagged_addr, get_tag(tagged_addr));
 		pr_err("\n");
 		print_memory_metadata(info.first_bad_addr);

@@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write,
 	return ret;
 }
 
+bool kasan_report_async(unsigned long addr, size_t size,
+			bool is_write, unsigned long ip)
+{
+	pr_info("==================================================================\n");
+	pr_info("KASAN: set in asynchronous mode\n");
+	pr_info("KASAN: some information might not be accurate\n");
+	pr_info("KASAN: fault address is ignored\n");
+	pr_info("KASAN: write/read distinction is ignored\n");
+
+	return kasan_report(addr, size, is_write, ip);

So just call kasan_report (0, 0, 0, ip) here.

Fine by me.

-- 
Regards,
Vincenzo

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

Hi Andrey,

On 1/19/21 6:10 PM, Andrey Konovalov wrote:

On Mon, Jan 18, 2021 at 7:30 PM Vincenzo Frascino
[off-list ref] wrote:

quoted

--- a/Documentation/dev-tools/kasan.rst
+++ b/Documentation/dev-tools/kasan.rst

@@ -162,6 +162,9 @@ particular KASAN features.

 - ``kasan=off`` or ``=on`` controls whether KASAN is enabled (default: ``on``).

+- ``kasan.mode=sync`` or ``=async`` controls whether KASAN is configured in
+  synchronous or asynchronous mode of execution (default: ``sync``).

This needs to be expanded with a short explanation of the difference.

Ok, I will extend it in v5.

quoted

+static inline void hw_enable_tagging_mode(void)
+{
+       if (kasan_arg_mode == KASAN_ARG_MODE_ASYNC)
+               hw_enable_tagging_async();
+       else
+               hw_enable_tagging_sync();
+}

It's OK to open-code this in kasan_init_hw_tags_cpu(), no need for an
additional function.

I added the new function to keep the code cleaner, but I do not have strong
opinion hence it is fine by me to have open-code here.

-- 
Regards,
Vincenzo

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

On 1/19/21 6:12 PM, Andrey Konovalov wrote:

On Tue, Jan 19, 2021 at 3:46 PM Mark Rutland [off-list ref] wrote:

quoted

Given there's no information available, I think it's simpler and
preferable to handle the logging separately, as is done for
kasan_report_invalid_free(). For example, we could do something roughly
like:

void kasan_report_async(void)
{
        unsigned long flags;

        start_report(&flags);
        pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n");

"BUG: KASAN: invalid-access"

Ok, I will do in v5. It looks more uniform with what we have for the sync exception.

It also might make sense to pass the ip, even though it's not exactly
related to the access:

I would like to avoid to add a builtin for something that has not a real meaning
as you are correctly pointing out.

pr_err("BUG: KASAN: invalid-access in %pS\n", (void *)ip);

Up to you.

quoted

        pr_err("KASAN: no fault information available\n");

pr_err("Asynchronous mode enabled: no access details available\n");

quoted

        dump_stack();
        end_report(&flags);
}

This approach with a dedicated function is better. Thanks, Mark!

Please put it next to kasan_report_invalid_free().

Will do in v5.

Thanks!

-- 
Regards,
Vincenzo

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

Hi Andrey,

On 1/19/21 6:09 PM, Andrey Konovalov wrote:

Hi Vincenzo,

This change has multiple conflicts with the KASAN testing patches that
are currently in the mm tree. If Andrew decides to send all of them
during RC, then this should be good to go through arm64. Otherwise, I
guess this will need to go through mm as well. So you probably need to
rebase this on top of those patches in any case.

Could you please let me know on which tree do you want me to rebase my patches?
I almost completed the requested changes.

Thank you!

Thanks!

-- 
Regards,
Vincenzo

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

On Thu, Jan 21, 2021 at 12:31 PM Vincenzo Frascino
[off-list ref] wrote:

Hi Andrey,

On 1/19/21 6:09 PM, Andrey Konovalov wrote:

quoted

Hi Vincenzo,

This change has multiple conflicts with the KASAN testing patches that
are currently in the mm tree. If Andrew decides to send all of them
during RC, then this should be good to go through arm64. Otherwise, I
guess this will need to go through mm as well. So you probably need to
rebase this on top of those patches in any case.

Could you please let me know on which tree do you want me to rebase my patches?
I almost completed the requested changes.

linux-next/akpm should work. Thanks!

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

Hi Andrey,

On 1/19/21 6:10 PM, Andrey Konovalov wrote:

I'll later add a patch on top that forbids running the tests with the
async mode.

Sorry, I misread this part, I thought you wanted me to do this. Anyway I added
the check to my last series.

Please have a look.

-- 
Regards,
Vincenzo

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