diff mbox series

[v15,5/8] arm64: mte: Enable TCO in functions that can read beyond buffer limits

Message ID 20210312142210.21326-6-vincenzo.frascino@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: ARMv8.5-A: MTE: Add async mode support | expand

Commit Message

Vincenzo Frascino March 12, 2021, 2:22 p.m. UTC 
load_unaligned_zeropad() and __get/put_kernel_nofault() functions can
read past some buffer limits which may include some MTE granule with a
different tag.

When MTE async mode is enabled, the load operation crosses the boundaries
and the next granule has a different tag the PE sets the TFSR_EL1.TF1 bit
as if an asynchronous tag fault is happened.

Enable Tag Check Override (TCO) in these functions  before the load and
disable it afterwards to prevent this to happen.

Note: The same condition can be hit in MTE sync mode but we deal with it
through the exception handling.
In the current implementation, mte_async_mode flag is set only at boot
time but in future kasan might acquire some runtime features that
that change the mode dynamically, hence we disable it when sync mode is
selected for future proof.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reported-by: Branislav Rankov <Branislav.Rankov@arm.com>
Tested-by: Branislav Rankov <Branislav.Rankov@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h            | 15 +++++++++++++++
 arch/arm64/include/asm/uaccess.h        | 22 ++++++++++++++++++++++
 arch/arm64/include/asm/word-at-a-time.h |  4 ++++
 arch/arm64/kernel/mte.c                 | 22 ++++++++++++++++++++++
 4 files changed, 63 insertions(+)

Comments

Catalin Marinas March 12, 2021, 3:13 p.m. UTC | #1 
On Fri, Mar 12, 2021 at 02:22:07PM +0000, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 9b557a457f24..8603c6636a7d 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -90,5 +90,20 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>  
>  #endif /* CONFIG_ARM64_MTE */
>  
> +#ifdef CONFIG_KASAN_HW_TAGS
> +/* Whether the MTE asynchronous mode is enabled. */
> +DECLARE_STATIC_KEY_FALSE(mte_async_mode);
> +
> +static inline bool system_uses_mte_async_mode(void)
> +{
> +	return static_branch_unlikely(&mte_async_mode);
> +}
> +#else
> +static inline bool system_uses_mte_async_mode(void)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_KASAN_HW_TAGS */

You can write this with fewer lines:

DECLARE_STATIC_KEY_FALSE(mte_async_mode);

static inline bool system_uses_mte_async_mode(void)
{
	return IS_ENABLED(CONFIG_KASAN_HW_TAGS) &&
		static_branch_unlikely(&mte_async_mode);
}

The compiler will ensure that mte_async_mode is not referred when
!CONFIG_KASAN_HW_TAGS and therefore doesn't need to be defined.

> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index fa755cf94e01..9362928ba0d5 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -26,6 +26,10 @@ u64 gcr_kernel_excl __ro_after_init;
>  
>  static bool report_fault_once = true;
>  
> +/* Whether the MTE asynchronous mode is enabled. */
> +DEFINE_STATIC_KEY_FALSE(mte_async_mode);
> +EXPORT_SYMBOL_GPL(mte_async_mode);

Maybe keep these bracketed by #ifdef CONFIG_KASAN_HW_TAGS. I think the
mte_enable_kernel_*() aren't needed either if KASAN_HW is disabled (you
can do it with an additional patch).

With these, you can add:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Vincenzo Frascino March 12, 2021, 3:23 p.m. UTC | #2 
On 3/12/21 3:13 PM, Catalin Marinas wrote:
> On Fri, Mar 12, 2021 at 02:22:07PM +0000, Vincenzo Frascino wrote:
>> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
>> index 9b557a457f24..8603c6636a7d 100644
>> --- a/arch/arm64/include/asm/mte.h
>> +++ b/arch/arm64/include/asm/mte.h
>> @@ -90,5 +90,20 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>>  
>>  #endif /* CONFIG_ARM64_MTE */
>>  
>> +#ifdef CONFIG_KASAN_HW_TAGS
>> +/* Whether the MTE asynchronous mode is enabled. */
>> +DECLARE_STATIC_KEY_FALSE(mte_async_mode);
>> +
>> +static inline bool system_uses_mte_async_mode(void)
>> +{
>> +	return static_branch_unlikely(&mte_async_mode);
>> +}
>> +#else
>> +static inline bool system_uses_mte_async_mode(void)
>> +{
>> +	return false;
>> +}
>> +#endif /* CONFIG_KASAN_HW_TAGS */
> 
> You can write this with fewer lines:
> 
> DECLARE_STATIC_KEY_FALSE(mte_async_mode);
> 
> static inline bool system_uses_mte_async_mode(void)
> {
> 	return IS_ENABLED(CONFIG_KASAN_HW_TAGS) &&
> 		static_branch_unlikely(&mte_async_mode);
> }
> 
> The compiler will ensure that mte_async_mode is not referred when
> !CONFIG_KASAN_HW_TAGS and therefore doesn't need to be defined.
>

Yes, I agree, but I introduce "#ifdef CONFIG_KASAN_HW_TAGS" in the successive
patch anyway, according to me the overall code looks more uniform like this. But
I do not have a strong opinion or preference on this.

>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index fa755cf94e01..9362928ba0d5 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -26,6 +26,10 @@ u64 gcr_kernel_excl __ro_after_init;
>>  
>>  static bool report_fault_once = true;
>>  
>> +/* Whether the MTE asynchronous mode is enabled. */
>> +DEFINE_STATIC_KEY_FALSE(mte_async_mode);
>> +EXPORT_SYMBOL_GPL(mte_async_mode);
> 
> Maybe keep these bracketed by #ifdef CONFIG_KASAN_HW_TAGS. I think the
> mte_enable_kernel_*() aren't needed either if KASAN_HW is disabled (you
> can do it with an additional patch).
>

Makes sense, I will add it in the next version.

> With these, you can add:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
Catalin Marinas March 12, 2021, 3:29 p.m. UTC | #3 
On Fri, Mar 12, 2021 at 03:23:44PM +0000, Vincenzo Frascino wrote:
> On 3/12/21 3:13 PM, Catalin Marinas wrote:
> > On Fri, Mar 12, 2021 at 02:22:07PM +0000, Vincenzo Frascino wrote:
> >> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> >> index 9b557a457f24..8603c6636a7d 100644
> >> --- a/arch/arm64/include/asm/mte.h
> >> +++ b/arch/arm64/include/asm/mte.h
> >> @@ -90,5 +90,20 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
> >>  
> >>  #endif /* CONFIG_ARM64_MTE */
> >>  
> >> +#ifdef CONFIG_KASAN_HW_TAGS
> >> +/* Whether the MTE asynchronous mode is enabled. */
> >> +DECLARE_STATIC_KEY_FALSE(mte_async_mode);
> >> +
> >> +static inline bool system_uses_mte_async_mode(void)
> >> +{
> >> +	return static_branch_unlikely(&mte_async_mode);
> >> +}
> >> +#else
> >> +static inline bool system_uses_mte_async_mode(void)
> >> +{
> >> +	return false;
> >> +}
> >> +#endif /* CONFIG_KASAN_HW_TAGS */
> > 
> > You can write this with fewer lines:
> > 
> > DECLARE_STATIC_KEY_FALSE(mte_async_mode);
> > 
> > static inline bool system_uses_mte_async_mode(void)
> > {
> > 	return IS_ENABLED(CONFIG_KASAN_HW_TAGS) &&
> > 		static_branch_unlikely(&mte_async_mode);
> > }
> > 
> > The compiler will ensure that mte_async_mode is not referred when
> > !CONFIG_KASAN_HW_TAGS and therefore doesn't need to be defined.
> 
> Yes, I agree, but I introduce "#ifdef CONFIG_KASAN_HW_TAGS" in the successive
> patch anyway, according to me the overall code looks more uniform like this. But
> I do not have a strong opinion or preference on this.

Ah, yes, I didn't look at patch 6 again as it was already reviewed and I
forgot the context. Leave it as it is then, my reviewed-by still stands.
Vincenzo Frascino March 12, 2021, 3:44 p.m. UTC | #4 
On 3/12/21 3:29 PM, Catalin Marinas wrote:
> On Fri, Mar 12, 2021 at 03:23:44PM +0000, Vincenzo Frascino wrote:
>> On 3/12/21 3:13 PM, Catalin Marinas wrote:
>>> On Fri, Mar 12, 2021 at 02:22:07PM +0000, Vincenzo Frascino wrote:
>>>> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
>>>> index 9b557a457f24..8603c6636a7d 100644
>>>> --- a/arch/arm64/include/asm/mte.h
>>>> +++ b/arch/arm64/include/asm/mte.h
>>>> @@ -90,5 +90,20 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>>>>  
>>>>  #endif /* CONFIG_ARM64_MTE */
>>>>  
>>>> +#ifdef CONFIG_KASAN_HW_TAGS
>>>> +/* Whether the MTE asynchronous mode is enabled. */
>>>> +DECLARE_STATIC_KEY_FALSE(mte_async_mode);
>>>> +
>>>> +static inline bool system_uses_mte_async_mode(void)
>>>> +{
>>>> +	return static_branch_unlikely(&mte_async_mode);
>>>> +}
>>>> +#else
>>>> +static inline bool system_uses_mte_async_mode(void)
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +#endif /* CONFIG_KASAN_HW_TAGS */
>>>
>>> You can write this with fewer lines:
>>>
>>> DECLARE_STATIC_KEY_FALSE(mte_async_mode);
>>>
>>> static inline bool system_uses_mte_async_mode(void)
>>> {
>>> 	return IS_ENABLED(CONFIG_KASAN_HW_TAGS) &&
>>> 		static_branch_unlikely(&mte_async_mode);
>>> }
>>>
>>> The compiler will ensure that mte_async_mode is not referred when
>>> !CONFIG_KASAN_HW_TAGS and therefore doesn't need to be defined.
>>
>> Yes, I agree, but I introduce "#ifdef CONFIG_KASAN_HW_TAGS" in the successive
>> patch anyway, according to me the overall code looks more uniform like this. But
>> I do not have a strong opinion or preference on this.
> 
> Ah, yes, I didn't look at patch 6 again as it was already reviewed and I
> forgot the context. Leave it as it is then, my reviewed-by still stands.
> 

Ok, thanks.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 9b557a457f24..8603c6636a7d 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -90,5 +90,20 @@  static inline void mte_assign_mem_tag_range(void *addr, size_t size)
 
 #endif /* CONFIG_ARM64_MTE */
 
+#ifdef CONFIG_KASAN_HW_TAGS
+/* Whether the MTE asynchronous mode is enabled. */
+DECLARE_STATIC_KEY_FALSE(mte_async_mode);
+
+static inline bool system_uses_mte_async_mode(void)
+{
+	return static_branch_unlikely(&mte_async_mode);
+}
+#else
+static inline bool system_uses_mte_async_mode(void)
+{
+	return false;
+}
+#endif /* CONFIG_KASAN_HW_TAGS */
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_MTE_H  */
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 0deb88467111..b5f08621fa29 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -20,6 +20,7 @@ 
 
 #include <asm/cpufeature.h>
 #include <asm/mmu.h>
+#include <asm/mte.h>
 #include <asm/ptrace.h>
 #include <asm/memory.h>
 #include <asm/extable.h>
@@ -188,6 +189,23 @@  static inline void __uaccess_enable_tco(void)
 				 ARM64_MTE, CONFIG_KASAN_HW_TAGS));
 }
 
+/*
+ * These functions disable tag checking only if in MTE async mode
+ * since the sync mode generates exceptions synchronously and the
+ * nofault or load_unaligned_zeropad can handle them.
+ */
+static inline void __uaccess_disable_tco_async(void)
+{
+	if (system_uses_mte_async_mode())
+		 __uaccess_disable_tco();
+}
+
+static inline void __uaccess_enable_tco_async(void)
+{
+	if (system_uses_mte_async_mode())
+		__uaccess_enable_tco();
+}
+
 static inline void uaccess_disable_privileged(void)
 {
 	__uaccess_disable_tco();
@@ -307,8 +325,10 @@  do {									\
 do {									\
 	int __gkn_err = 0;						\
 									\
+	__uaccess_enable_tco_async();					\
 	__raw_get_mem("ldr", *((type *)(dst)),				\
 		      (__force type *)(src), __gkn_err);		\
+	__uaccess_disable_tco_async();					\
 	if (unlikely(__gkn_err))					\
 		goto err_label;						\
 } while (0)
@@ -380,8 +400,10 @@  do {									\
 do {									\
 	int __pkn_err = 0;						\
 									\
+	__uaccess_enable_tco_async();					\
 	__raw_put_mem("str", *((type *)(src)),				\
 		      (__force type *)(dst), __pkn_err);		\
+	__uaccess_disable_tco_async();					\
 	if (unlikely(__pkn_err))					\
 		goto err_label;						\
 } while(0)
diff --git a/arch/arm64/include/asm/word-at-a-time.h b/arch/arm64/include/asm/word-at-a-time.h
index 3333950b5909..c62d9fa791aa 100644
--- a/arch/arm64/include/asm/word-at-a-time.h
+++ b/arch/arm64/include/asm/word-at-a-time.h
@@ -55,6 +55,8 @@  static inline unsigned long load_unaligned_zeropad(const void *addr)
 {
 	unsigned long ret, offset;
 
+	__uaccess_enable_tco_async();
+
 	/* Load word from unaligned pointer addr */
 	asm(
 	"1:	ldr	%0, %3\n"
@@ -76,6 +78,8 @@  static inline unsigned long load_unaligned_zeropad(const void *addr)
 	: "=&r" (ret), "=&r" (offset)
 	: "r" (addr), "Q" (*(unsigned long *)addr));
 
+	__uaccess_disable_tco_async();
+
 	return ret;
 }
 
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index fa755cf94e01..9362928ba0d5 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -26,6 +26,10 @@  u64 gcr_kernel_excl __ro_after_init;
 
 static bool report_fault_once = true;
 
+/* Whether the MTE asynchronous mode is enabled. */
+DEFINE_STATIC_KEY_FALSE(mte_async_mode);
+EXPORT_SYMBOL_GPL(mte_async_mode);
+
 static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
 {
 	pte_t old_pte = READ_ONCE(*ptep);
@@ -118,12 +122,30 @@  static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
 
 void mte_enable_kernel_sync(void)
 {
+	/*
+	 * Make sure we enter this function when no PE has set
+	 * async mode previously.
+	 */
+	WARN_ONCE(system_uses_mte_async_mode(),
+			"MTE async mode enabled system wide!");
+
 	__mte_enable_kernel("synchronous", SCTLR_ELx_TCF_SYNC);
 }
 
 void mte_enable_kernel_async(void)
 {
 	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
+
+	/*
+	 * MTE async mode is set system wide by the first PE that
+	 * executes this function.
+	 *
+	 * Note: If in future KASAN acquires a runtime switching
+	 * mode in between sync and async, this strategy needs
+	 * to be reviewed.
+	 */
+	if (!system_uses_mte_async_mode())
+		static_branch_enable(&mte_async_mode);
 }
 
 void mte_set_report_once(bool state)