diff mbox series

[v4,29/39] arm64: mte: Switch GCR_EL1 in kernel entry and exit

Message ID 1f2681fdff1aa1096df949cb8634a9be6bf4acc4.1601593784.git.andreyknvl@google.com (mailing list archive)
State New, archived
Headers show
Series kasan: add hardware tag-based mode for arm64 | expand

Commit Message

Andrey Konovalov Oct. 1, 2020, 11:10 p.m. UTC 
From: Vincenzo Frascino <vincenzo.frascino@arm.com>

When MTE is present, the GCR_EL1 register contains the tags mask that
allows to exclude tags from the random generation via the IRG instruction.

With the introduction of the new Tag-Based KASAN API that provides a
mechanism to reserve tags for special reasons, the MTE implementation
has to make sure that the GCR_EL1 setting for the kernel does not affect
the userspace processes and viceversa.

Save and restore the kernel/user mask in GCR_EL1 in kernel entry and exit.

Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
Change-Id: I0081cba5ace27a9111bebb239075c9a466af4c84
---
 arch/arm64/include/asm/mte-kasan.h |  6 +++++
 arch/arm64/include/asm/mte.h       |  2 ++
 arch/arm64/kernel/asm-offsets.c    |  3 +++
 arch/arm64/kernel/cpufeature.c     |  3 +++
 arch/arm64/kernel/entry.S          | 41 ++++++++++++++++++++++++++++++
 arch/arm64/kernel/mte.c            | 18 ++++++++++---
 6 files changed, 70 insertions(+), 3 deletions(-)

Comments

Catalin Marinas Oct. 2, 2020, 2:06 p.m. UTC | #1 
On Fri, Oct 02, 2020 at 01:10:30AM +0200, Andrey Konovalov wrote:
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 7c67ac6f08df..d1847f29f59b 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -23,6 +23,8 @@
>  #include <asm/ptrace.h>
>  #include <asm/sysreg.h>
>  
> +u64 gcr_kernel_excl __ro_after_init;
> +
>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>  {
>  	pte_t old_pte = READ_ONCE(*ptep);
> @@ -120,6 +122,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>  	return ptr;
>  }
>  
> +void mte_init_tags(u64 max_tag)
> +{
> +	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);

Nitpick: it's not obvious that MTE_TAG_MAX is a mask, so better write
this as GENMASK(min(max_tag, MTE_TAG_MAX), 0).

Otherwise it looks fine.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Vincenzo Frascino Oct. 8, 2020, 6:24 p.m. UTC | #2 
Hi Catalin,

On 10/2/20 3:06 PM, Catalin Marinas wrote:
> On Fri, Oct 02, 2020 at 01:10:30AM +0200, Andrey Konovalov wrote:
>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index 7c67ac6f08df..d1847f29f59b 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -23,6 +23,8 @@
>>  #include <asm/ptrace.h>
>>  #include <asm/sysreg.h>
>>  
>> +u64 gcr_kernel_excl __ro_after_init;
>> +
>>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>>  {
>>  	pte_t old_pte = READ_ONCE(*ptep);
>> @@ -120,6 +122,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>>  	return ptr;
>>  }
>>  
>> +void mte_init_tags(u64 max_tag)
>> +{
>> +	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);
> 
> Nitpick: it's not obvious that MTE_TAG_MAX is a mask, so better write
> this as GENMASK(min(max_tag, MTE_TAG_MAX), 0).
> 

The two things do not seem equivalent because the format of the tags in KASAN is
0xFF and in MTE is 0xF, hence if extract the minimum whatever is the tag passed
by KASAN it will always be MTE_TAG_MAX.

To make it cleaner I propose: GENMASK(FIELD_GET(MTE_TAG_MAX, max_tag), 0);

> Otherwise it looks fine.
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
Catalin Marinas Oct. 9, 2020, 8:11 a.m. UTC | #3 
On Thu, Oct 08, 2020 at 07:24:12PM +0100, Vincenzo Frascino wrote:
> On 10/2/20 3:06 PM, Catalin Marinas wrote:
> > On Fri, Oct 02, 2020 at 01:10:30AM +0200, Andrey Konovalov wrote:
> >> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> >> index 7c67ac6f08df..d1847f29f59b 100644
> >> --- a/arch/arm64/kernel/mte.c
> >> +++ b/arch/arm64/kernel/mte.c
> >> @@ -23,6 +23,8 @@
> >>  #include <asm/ptrace.h>
> >>  #include <asm/sysreg.h>
> >>  
> >> +u64 gcr_kernel_excl __ro_after_init;
> >> +
> >>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
> >>  {
> >>  	pte_t old_pte = READ_ONCE(*ptep);
> >> @@ -120,6 +122,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> >>  	return ptr;
> >>  }
> >>  
> >> +void mte_init_tags(u64 max_tag)
> >> +{
> >> +	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);
> > 
> > Nitpick: it's not obvious that MTE_TAG_MAX is a mask, so better write
> > this as GENMASK(min(max_tag, MTE_TAG_MAX), 0).
> 
> The two things do not seem equivalent because the format of the tags in KASAN is
> 0xFF and in MTE is 0xF, hence if extract the minimum whatever is the tag passed
> by KASAN it will always be MTE_TAG_MAX.
> 
> To make it cleaner I propose: GENMASK(FIELD_GET(MTE_TAG_MAX, max_tag), 0);

I don't think that's any clearer since FIELD_GET still assumes that
MTE_TAG_MAX is a mask. I think it's better to add a comment on why this
is needed, as you explained above that the KASAN tags go to 0xff.

If you want to get rid of MTE_TAG_MAX altogether, just do a

	max_tag &= (1 << MAX_TAG_SIZE) - 1;

before setting incl (a comment is still useful).
Vincenzo Frascino Oct. 9, 2020, 9:56 a.m. UTC | #4 
On 10/9/20 9:11 AM, Catalin Marinas wrote:
> On Thu, Oct 08, 2020 at 07:24:12PM +0100, Vincenzo Frascino wrote:
>> On 10/2/20 3:06 PM, Catalin Marinas wrote:
>>> On Fri, Oct 02, 2020 at 01:10:30AM +0200, Andrey Konovalov wrote:
>>>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>>>> index 7c67ac6f08df..d1847f29f59b 100644
>>>> --- a/arch/arm64/kernel/mte.c
>>>> +++ b/arch/arm64/kernel/mte.c
>>>> @@ -23,6 +23,8 @@
>>>>  #include <asm/ptrace.h>
>>>>  #include <asm/sysreg.h>
>>>>  
>>>> +u64 gcr_kernel_excl __ro_after_init;
>>>> +
>>>>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>>>>  {
>>>>  	pte_t old_pte = READ_ONCE(*ptep);
>>>> @@ -120,6 +122,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>>>>  	return ptr;
>>>>  }
>>>>  
>>>> +void mte_init_tags(u64 max_tag)
>>>> +{
>>>> +	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);
>>>
>>> Nitpick: it's not obvious that MTE_TAG_MAX is a mask, so better write
>>> this as GENMASK(min(max_tag, MTE_TAG_MAX), 0).
>>
>> The two things do not seem equivalent because the format of the tags in KASAN is
>> 0xFF and in MTE is 0xF, hence if extract the minimum whatever is the tag passed
>> by KASAN it will always be MTE_TAG_MAX.
>>
>> To make it cleaner I propose: GENMASK(FIELD_GET(MTE_TAG_MAX, max_tag), 0);
> 
> I don't think that's any clearer since FIELD_GET still assumes that
> MTE_TAG_MAX is a mask. I think it's better to add a comment on why this
> is needed, as you explained above that the KASAN tags go to 0xff.
> 
> If you want to get rid of MTE_TAG_MAX altogether, just do a
> 
> 	max_tag &= (1 << MAX_TAG_SIZE) - 1;
> 
> before setting incl (a comment is still useful).
> 

Agree, but still think we should use FIELD_GET here since it is common language
in the kernel.

How about we get rid of MTE_TAG_MAX and we do something like:

GENMASK(FIELD_GET(MTE_TAG_MASK >> MTE_TAG_SHIFT, max_tag), 0);

Obviously with a comment ;)
Catalin Marinas Oct. 9, 2020, 10:16 a.m. UTC | #5 
On Fri, Oct 09, 2020 at 10:56:02AM +0100, Vincenzo Frascino wrote:
> On 10/9/20 9:11 AM, Catalin Marinas wrote:
> > On Thu, Oct 08, 2020 at 07:24:12PM +0100, Vincenzo Frascino wrote:
> >> On 10/2/20 3:06 PM, Catalin Marinas wrote:
> >>> On Fri, Oct 02, 2020 at 01:10:30AM +0200, Andrey Konovalov wrote:
> >>>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> >>>> index 7c67ac6f08df..d1847f29f59b 100644
> >>>> --- a/arch/arm64/kernel/mte.c
> >>>> +++ b/arch/arm64/kernel/mte.c
> >>>> @@ -23,6 +23,8 @@
> >>>>  #include <asm/ptrace.h>
> >>>>  #include <asm/sysreg.h>
> >>>>  
> >>>> +u64 gcr_kernel_excl __ro_after_init;
> >>>> +
> >>>>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
> >>>>  {
> >>>>  	pte_t old_pte = READ_ONCE(*ptep);
> >>>> @@ -120,6 +122,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> >>>>  	return ptr;
> >>>>  }
> >>>>  
> >>>> +void mte_init_tags(u64 max_tag)
> >>>> +{
> >>>> +	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);
> >>>
> >>> Nitpick: it's not obvious that MTE_TAG_MAX is a mask, so better write
> >>> this as GENMASK(min(max_tag, MTE_TAG_MAX), 0).
> >>
> >> The two things do not seem equivalent because the format of the tags in KASAN is
> >> 0xFF and in MTE is 0xF, hence if extract the minimum whatever is the tag passed
> >> by KASAN it will always be MTE_TAG_MAX.
> >>
> >> To make it cleaner I propose: GENMASK(FIELD_GET(MTE_TAG_MAX, max_tag), 0);
> > 
> > I don't think that's any clearer since FIELD_GET still assumes that
> > MTE_TAG_MAX is a mask. I think it's better to add a comment on why this
> > is needed, as you explained above that the KASAN tags go to 0xff.
> > 
> > If you want to get rid of MTE_TAG_MAX altogether, just do a
> > 
> > 	max_tag &= (1 << MAX_TAG_SIZE) - 1;
> > 
> > before setting incl (a comment is still useful).
> > 
> 
> Agree, but still think we should use FIELD_GET here since it is common language
> in the kernel.
> 
> How about we get rid of MTE_TAG_MAX and we do something like:
> 
> GENMASK(FIELD_GET(MTE_TAG_MASK >> MTE_TAG_SHIFT, max_tag), 0);

It works for me and you can drop the MTE_TAG_MAX definition (I think
it's only used here).
Vincenzo Frascino Oct. 9, 2020, 10:21 a.m. UTC | #6 
On 10/9/20 11:16 AM, Catalin Marinas wrote:
> On Fri, Oct 09, 2020 at 10:56:02AM +0100, Vincenzo Frascino wrote:
>> On 10/9/20 9:11 AM, Catalin Marinas wrote:
>>> On Thu, Oct 08, 2020 at 07:24:12PM +0100, Vincenzo Frascino wrote:
>>>> On 10/2/20 3:06 PM, Catalin Marinas wrote:
>>>>> On Fri, Oct 02, 2020 at 01:10:30AM +0200, Andrey Konovalov wrote:
>>>>>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>>>>>> index 7c67ac6f08df..d1847f29f59b 100644
>>>>>> --- a/arch/arm64/kernel/mte.c
>>>>>> +++ b/arch/arm64/kernel/mte.c
>>>>>> @@ -23,6 +23,8 @@
>>>>>>  #include <asm/ptrace.h>
>>>>>>  #include <asm/sysreg.h>
>>>>>>  
>>>>>> +u64 gcr_kernel_excl __ro_after_init;
>>>>>> +
>>>>>>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>>>>>>  {
>>>>>>  	pte_t old_pte = READ_ONCE(*ptep);
>>>>>> @@ -120,6 +122,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>>>>>>  	return ptr;
>>>>>>  }
>>>>>>  
>>>>>> +void mte_init_tags(u64 max_tag)
>>>>>> +{
>>>>>> +	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);
>>>>>
>>>>> Nitpick: it's not obvious that MTE_TAG_MAX is a mask, so better write
>>>>> this as GENMASK(min(max_tag, MTE_TAG_MAX), 0).
>>>>
>>>> The two things do not seem equivalent because the format of the tags in KASAN is
>>>> 0xFF and in MTE is 0xF, hence if extract the minimum whatever is the tag passed
>>>> by KASAN it will always be MTE_TAG_MAX.
>>>>
>>>> To make it cleaner I propose: GENMASK(FIELD_GET(MTE_TAG_MAX, max_tag), 0);
>>>
>>> I don't think that's any clearer since FIELD_GET still assumes that
>>> MTE_TAG_MAX is a mask. I think it's better to add a comment on why this
>>> is needed, as you explained above that the KASAN tags go to 0xff.
>>>
>>> If you want to get rid of MTE_TAG_MAX altogether, just do a
>>>
>>> 	max_tag &= (1 << MAX_TAG_SIZE) - 1;
>>>
>>> before setting incl (a comment is still useful).
>>>
>>
>> Agree, but still think we should use FIELD_GET here since it is common language
>> in the kernel.
>>
>> How about we get rid of MTE_TAG_MAX and we do something like:
>>
>> GENMASK(FIELD_GET(MTE_TAG_MASK >> MTE_TAG_SHIFT, max_tag), 0);
> 
> It works for me and you can drop the MTE_TAG_MAX definition (I think
> it's only used here).
> 

Yes indeed, I will drop it.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
index 3a70fb1807fd..a4c61b926d4a 100644
--- a/arch/arm64/include/asm/mte-kasan.h
+++ b/arch/arm64/include/asm/mte-kasan.h
@@ -29,6 +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_init_tags(u64 max_tag);
+
 #else /* CONFIG_ARM64_MTE */
 
 static inline u8 mte_get_ptr_tag(void *ptr)
@@ -49,6 +51,10 @@  static inline void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
 	return addr;
 }
 
+static inline void mte_init_tags(u64 max_tag)
+{
+}
+
 #endif /* CONFIG_ARM64_MTE */
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index cf1cd181dcb2..d02aff9f493d 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -18,6 +18,8 @@ 
 
 #include <asm/pgtable-types.h>
 
+extern u64 gcr_kernel_excl;
+
 void mte_clear_page_tags(void *addr);
 unsigned long mte_copy_tags_from_user(void *to, const void __user *from,
 				      unsigned long n);
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 7d32fc959b1a..dfe6ed8446ac 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -47,6 +47,9 @@  int main(void)
 #ifdef CONFIG_ARM64_PTR_AUTH
   DEFINE(THREAD_KEYS_USER,	offsetof(struct task_struct, thread.keys_user));
   DEFINE(THREAD_KEYS_KERNEL,	offsetof(struct task_struct, thread.keys_kernel));
+#endif
+#ifdef CONFIG_ARM64_MTE
+  DEFINE(THREAD_GCR_EL1_USER,	offsetof(struct task_struct, thread.gcr_user_excl));
 #endif
   BLANK();
   DEFINE(S_X0,			offsetof(struct pt_regs, regs[0]));
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index eca06b8c74db..e76634ad5bc7 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1721,6 +1721,9 @@  static void cpu_enable_mte(struct arm64_cpu_capabilities const *cap)
 
 	/* Enable in-kernel MTE only if KASAN_HW_TAGS is enabled */
 	if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) {
+		/* Enable the kernel exclude mask for random tags generation */
+		write_sysreg_s(SYS_GCR_EL1_RRND | gcr_kernel_excl, SYS_GCR_EL1);
+
 		/* Enable MTE Sync Mode for EL1 */
 		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
 		isb();
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ff34461524d4..eeaac91021bf 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -175,6 +175,43 @@  alternative_else_nop_endif
 #endif
 	.endm
 
+	.macro mte_set_gcr, tmp, tmp2
+#ifdef CONFIG_ARM64_MTE
+	/*
+	 * Calculate and set the exclude mask preserving
+	 * the RRND (bit[16]) setting.
+	 */
+	mrs_s	\tmp2, SYS_GCR_EL1
+	bfi	\tmp2, \tmp, #0, #16
+	msr_s	SYS_GCR_EL1, \tmp2
+	isb
+#endif
+	.endm
+
+	.macro mte_set_kernel_gcr, tmp, tmp2
+#ifdef CONFIG_KASAN_HW_TAGS
+alternative_if_not ARM64_MTE
+	b	1f
+alternative_else_nop_endif
+	ldr_l	\tmp, gcr_kernel_excl
+
+	mte_set_gcr \tmp, \tmp2
+1:
+#endif
+	.endm
+
+	.macro mte_set_user_gcr, tsk, tmp, tmp2
+#ifdef CONFIG_ARM64_MTE
+alternative_if_not ARM64_MTE
+	b	1f
+alternative_else_nop_endif
+	ldr	\tmp, [\tsk, #THREAD_GCR_EL1_USER]
+
+	mte_set_gcr \tmp, \tmp2
+1:
+#endif
+	.endm
+
 	.macro	kernel_entry, el, regsize = 64
 	.if	\regsize == 32
 	mov	w0, w0				// zero upper 32 bits of x0
@@ -214,6 +251,8 @@  alternative_else_nop_endif
 
 	ptrauth_keys_install_kernel tsk, x20, x22, x23
 
+	mte_set_kernel_gcr x22, x23
+
 	scs_load tsk, x20
 	.else
 	add	x21, sp, #S_FRAME_SIZE
@@ -332,6 +371,8 @@  alternative_else_nop_endif
 	/* No kernel C function calls after this as user keys are set. */
 	ptrauth_keys_install_user tsk, x0, x1, x2
 
+	mte_set_user_gcr tsk, x0, x1
+
 	apply_ssbd 0, x0, x1
 	.endif
 
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 7c67ac6f08df..d1847f29f59b 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -23,6 +23,8 @@ 
 #include <asm/ptrace.h>
 #include <asm/sysreg.h>
 
+u64 gcr_kernel_excl __ro_after_init;
+
 static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
 {
 	pte_t old_pte = READ_ONCE(*ptep);
@@ -120,6 +122,13 @@  void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
 	return ptr;
 }
 
+void mte_init_tags(u64 max_tag)
+{
+	u64 incl = GENMASK(max_tag & MTE_TAG_MAX, 0);
+
+	gcr_kernel_excl = ~incl & SYS_GCR_EL1_EXCL_MASK;
+}
+
 static void update_sctlr_el1_tcf0(u64 tcf0)
 {
 	/* ISB required for the kernel uaccess routines */
@@ -155,7 +164,11 @@  static void update_gcr_el1_excl(u64 excl)
 static void set_gcr_el1_excl(u64 excl)
 {
 	current->thread.gcr_user_excl = excl;
-	update_gcr_el1_excl(excl);
+
+	/*
+	 * SYS_GCR_EL1 will be set to current->thread.gcr_user_excl value
+	 * by mte_set_user_gcr() in kernel_exit,
+	 */
 }
 
 void flush_mte_state(void)
@@ -181,7 +194,6 @@  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);
-	update_gcr_el1_excl(next->thread.gcr_user_excl);
 }
 
 void mte_suspend_exit(void)
@@ -189,7 +201,7 @@  void mte_suspend_exit(void)
 	if (!system_supports_mte())
 		return;
 
-	update_gcr_el1_excl(current->thread.gcr_user_excl);
+	update_gcr_el1_excl(gcr_kernel_excl);
 }
 
 long set_mte_ctrl(struct task_struct *task, unsigned long arg)