Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Vincenzo Frascino]

Hi Andrey,

On 11/4/20 11:18 PM, Andrey Konovalov wrote:
> Hardware tag-based KASAN relies on Memory Tagging Extension (MTE)
> feature and requires it to be enabled. MTE supports
> 
> This patch adds a new mte_init_tags() helper, that enables MTE in
> Synchronous mode in EL1 and is intended to be called from KASAN runtime
> during initialization.
> 
> The Tag Checking operation causes a synchronous data abort as
> a consequence of a tag check fault when MTE is configured in
> synchronous mode.
> 
> As part of this change enable match-all tag for EL1 to allow the
> kernel to access user pages without faulting. This is required because
> the kernel does not have knowledge of the tags set by the user in a
> page.
> 
> Note: For MTE, the TCF bit field in SCTLR_EL1 affects only EL1 in a
> similar way as TCF0 affects EL0.
> 
> MTE that is built on top of the Top Byte Ignore (TBI) feature hence we
> enable it as part of this patch as well.
> 

seems that in this patch you dropped me as author. Would you mind to clarify the
reason?

> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
> Change-Id: I4d67497268bb7f0c2fc5dcacefa1e273df4af71d
> ---
>  arch/arm64/include/asm/mte-kasan.h |  6 ++++++
>  arch/arm64/kernel/mte.c            |  7 +++++++
>  arch/arm64/mm/proc.S               | 23 ++++++++++++++++++++---
>  3 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
> index 3a70fb1807fd..ae75feaea2d4 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 __init 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/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 06ba6c923ab7..fcfbefcc3174 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -121,6 +121,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>  	return ptr;
>  }
>  
> +void __init mte_init_tags(u64 max_tag)
> +{
> +	/* Enable MTE Sync Mode for EL1. */
> +	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> +	isb();

I am fine with the approach of letting cpu_enable_mte() call directly
kasan_init_tags(), but how does it work of the other 2 implementation of KASAN?
Is it still called in arch_setup()?

I would prefer to keep the code that initializes the sync mode in
cpu_enable_mte() (calling kasan_init_tags() before then that) or in a separate
function since setting the mode has nothing to do with initializing the tags.
The second approach probably would come handy when we introduce async mode.

> +}
> +
>  static void update_sctlr_el1_tcf0(u64 tcf0)
>  {
>  	/* ISB required for the kernel uaccess routines */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 23c326a06b2d..7c3304fb15d9 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -40,9 +40,15 @@
>  #define TCR_CACHE_FLAGS	TCR_IRGN_WBWA | TCR_ORGN_WBWA
>  
>  #ifdef CONFIG_KASAN_SW_TAGS
> -#define TCR_KASAN_FLAGS TCR_TBI1
> +#define TCR_KASAN_SW_FLAGS TCR_TBI1
>  #else
> -#define TCR_KASAN_FLAGS 0
> +#define TCR_KASAN_SW_FLAGS 0
> +#endif
> +
> +#ifdef CONFIG_KASAN_HW_TAGS
> +#define TCR_KASAN_HW_FLAGS SYS_TCR_EL1_TCMA1 | TCR_TBI1
> +#else
> +#define TCR_KASAN_HW_FLAGS 0
>  #endif
>  
>  /*
> @@ -427,6 +433,10 @@ SYM_FUNC_START(__cpu_setup)
>  	 */
>  	mov_q	x5, MAIR_EL1_SET
>  #ifdef CONFIG_ARM64_MTE
> +	mte_tcr	.req	x20
> +
> +	mov	mte_tcr, #0
> +
>  	/*
>  	 * Update MAIR_EL1, GCR_EL1 and TFSR*_EL1 if MTE is supported
>  	 * (ID_AA64PFR1_EL1[11:8] > 1).
> @@ -447,6 +457,9 @@ SYM_FUNC_START(__cpu_setup)
>  	/* clear any pending tag check faults in TFSR*_EL1 */
>  	msr_s	SYS_TFSR_EL1, xzr
>  	msr_s	SYS_TFSRE0_EL1, xzr
> +
> +	/* set the TCR_EL1 bits */
> +	mov_q	mte_tcr, TCR_KASAN_HW_FLAGS
>  1:
>  #endif
>  	msr	mair_el1, x5
> @@ -456,7 +469,11 @@ SYM_FUNC_START(__cpu_setup)
>  	 */
>  	mov_q	x10, TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | \
>  			TCR_TG_FLAGS | TCR_KASLR_FLAGS | TCR_ASID16 | \
> -			TCR_TBI0 | TCR_A1 | TCR_KASAN_FLAGS
> +			TCR_TBI0 | TCR_A1 | TCR_KASAN_SW_FLAGS
> +#ifdef CONFIG_ARM64_MTE
> +	orr	x10, x10, mte_tcr
> +	.unreq	mte_tcr
> +#endif
>  	tcr_clear_errata_bits x10, x9, x5
>  
>  #ifdef CONFIG_ARM64_VA_BITS_52
> 

-- 
Regards,
Vincenzo

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Andrey Konovalov]

On Thu, Nov 5, 2020 at 12:13 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> Hi Andrey,
>
> On 11/4/20 11:18 PM, Andrey Konovalov wrote:
> > Hardware tag-based KASAN relies on Memory Tagging Extension (MTE)
> > feature and requires it to be enabled. MTE supports
> >
> > This patch adds a new mte_init_tags() helper, that enables MTE in
> > Synchronous mode in EL1 and is intended to be called from KASAN runtime
> > during initialization.
> >
> > The Tag Checking operation causes a synchronous data abort as
> > a consequence of a tag check fault when MTE is configured in
> > synchronous mode.
> >
> > As part of this change enable match-all tag for EL1 to allow the
> > kernel to access user pages without faulting. This is required because
> > the kernel does not have knowledge of the tags set by the user in a
> > page.
> >
> > Note: For MTE, the TCF bit field in SCTLR_EL1 affects only EL1 in a
> > similar way as TCF0 affects EL0.
> >
> > MTE that is built on top of the Top Byte Ignore (TBI) feature hence we
> > enable it as part of this patch as well.
> >
>
> seems that in this patch you dropped me as author. Would you mind to clarify the
> reason?

Sorry, a mistake while squashing/rebasing, will fix in the next version.

>
> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Co-developed-by: Andrey Konovalov <andreyknvl@google.com>
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > ---
> > Change-Id: I4d67497268bb7f0c2fc5dcacefa1e273df4af71d
> > ---
> >  arch/arm64/include/asm/mte-kasan.h |  6 ++++++
> >  arch/arm64/kernel/mte.c            |  7 +++++++
> >  arch/arm64/mm/proc.S               | 23 ++++++++++++++++++++---
> >  3 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
> > index 3a70fb1807fd..ae75feaea2d4 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 __init 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/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index 06ba6c923ab7..fcfbefcc3174 100644
> > --- a/arch/arm64/kernel/mte.c
> > +++ b/arch/arm64/kernel/mte.c
> > @@ -121,6 +121,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> >       return ptr;
> >  }
> >
> > +void __init mte_init_tags(u64 max_tag)
> > +{
> > +     /* Enable MTE Sync Mode for EL1. */
> > +     sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> > +     isb();
>
> I am fine with the approach of letting cpu_enable_mte() call directly
> kasan_init_tags(), but how does it work of the other 2 implementation of KASAN?
> Is it still called in arch_setup()?

Yes, the other 2 modes are initialized in setup_arch().

> I would prefer to keep the code that initializes the sync mode in
> cpu_enable_mte() (calling kasan_init_tags() before then that)

This won't work, we'll later need to make the decision about whether
to turn on MTE at all in KASAN runtime based on KASAN boot flags.

> or in a separate
> function since setting the mode has nothing to do with initializing the tags.

This will work. Any preference on the name of this function?

Alternatively we can rename mte_init_tags() to something else and let
it handle both RRND and sync/async.

Thanks!

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Vincenzo Frascino]

On 11/5/20 11:35 AM, Andrey Konovalov wrote:
> This will work. Any preference on the name of this function?
>

I called it in my current iteration mte_enable(), and calling it from
cpu_enable_mte().

> Alternatively we can rename mte_init_tags() to something else and let
> it handle both RRND and sync/async.

This is an option but then you need to change the name of kasan_init_tags and
the init_tags indirection name as well. I would go for the simpler and just
splitting the function as per above.

What do you think?

-- 
Regards,
Vincenzo

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Andrey Konovalov]

On Thu, Nov 5, 2020 at 12:39 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> On 11/5/20 11:35 AM, Andrey Konovalov wrote:
> > This will work. Any preference on the name of this function?
> >
>
> I called it in my current iteration mte_enable(), and calling it from
> cpu_enable_mte().
>
> > Alternatively we can rename mte_init_tags() to something else and let
> > it handle both RRND and sync/async.
>
> This is an option but then you need to change the name of kasan_init_tags and
> the init_tags indirection name as well. I would go for the simpler and just
> splitting the function as per above.
>
> What do you think?

OK, let's split. mte_enable() as a name sounds good to me. Both
functions will still be called one right after another from
kasan_init_hw_tags (as it's now called) though. I think the name
works, as it means initializing the hw_tags mode, not just the tags.

Will do in v9.

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Vincenzo Frascino]

On 11/5/20 12:14 PM, Andrey Konovalov wrote:
> On Thu, Nov 5, 2020 at 12:39 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
>>
>> On 11/5/20 11:35 AM, Andrey Konovalov wrote:
>>> This will work. Any preference on the name of this function?
>>>
>>
>> I called it in my current iteration mte_enable(), and calling it from
>> cpu_enable_mte().
>>
>>> Alternatively we can rename mte_init_tags() to something else and let
>>> it handle both RRND and sync/async.
>>
>> This is an option but then you need to change the name of kasan_init_tags and
>> the init_tags indirection name as well. I would go for the simpler and just
>> splitting the function as per above.
>>
>> What do you think?
> 
> OK, let's split. mte_enable() as a name sounds good to me. Both
> functions will still be called one right after another from
> kasan_init_hw_tags (as it's now called) though. I think the name
> works, as it means initializing the hw_tags mode, not just the tags.
> 

I agree. When you finish with v9, could you please provide a tree with both the
sets on top similar to [1]? I would like to repeat the tests (ltp + kselftests)
and even to rebase my async code on top of it since we are aligning with the
development.

[1] https://github.com/xairy/linux/tree/up-boot-mte-v1

> Will do in v9.
> 

-- 
Regards,
Vincenzo

Re: [PATCH v8 28/43] arm64: mte: Reset the page tag in page->flags

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:43AM +0100, Andrey Konovalov wrote:
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 8f99c65837fd..06ba6c923ab7 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -34,6 +34,7 @@ static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>  			return;
>  	}
>  
> +	page_kasan_tag_reset(page);
>  	mte_clear_page_tags(page_address(page));

I think we need an smp_wmb() between setting the flags and clearing the
actual tags. If another threads reads page->flags and builds a tagged
address out of it (see page_to_virt) there's an address dependency to
the actual memory access. However, on the current thread, we don't
guarantee that the new page->flags are visible before the tags were
updated.

>  }
>  
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index 70a71f38b6a9..348f4627da08 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -22,6 +22,7 @@ void copy_highpage(struct page *to, struct page *from)
>  	copy_page(kto, kfrom);
>  
>  	if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) {
> +		page_kasan_tag_reset(to);
>  		set_bit(PG_mte_tagged, &to->flags);
>  		mte_copy_page_tags(kto, kfrom);

Nitpick: move page_kasan_tag_reset() just above mte_copy_page_tags() for
consistency with the other places where PG_mte_tagged is set before or
after the actual tag setting.

>  	}
> diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
> index c52c1847079c..0e7eccbe598a 100644
> --- a/arch/arm64/mm/mteswap.c
> +++ b/arch/arm64/mm/mteswap.c
> @@ -53,6 +53,7 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page)
>  	if (!tags)
>  		return false;
>  
> +	page_kasan_tag_reset(page);
>  	mte_restore_page_tags(page_address(page), tags);

There is another mte_restore_page_tags() caller in hibernate.c. That one
doesn't need page_kasan_tag_reset() since the page->flags would have
been already restored but please add a comment in that file why its not
needed.

-- 
Catalin

Re: [PATCH v8 17/43] kasan, arm64: move initialization message

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:32AM +0100, Andrey Konovalov wrote:
> Software tag-based KASAN mode is fully initialized with kasan_init_tags(),
> while the generic mode only requires kasan_init(). Move the
> initialization message for tag-based mode into kasan_init_tags().
> 
> Also fix pr_fmt() usage for KASAN code: generic.c doesn't need it as it
> doesn't use any printing functions; tag-based mode should use "kasan:"
> instead of KBUILD_MODNAME (which stands for file name).
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

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

Re: [PATCH v8 18/43] kasan, arm64: rename kasan_init_tags and mark as __init

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:33AM +0100, Andrey Konovalov wrote:
> Rename kasan_init_tags() to kasan_init_sw_tags() as the upcoming hardware
> tag-based KASAN mode will have its own initialization routine.
> Also similarly to kasan_init() mark kasan_init_tags() as __init.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

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

Re: [PATCH v8 29/43] arm64: mte: Add in-kernel tag fault handler

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:44AM +0100, Andrey Konovalov wrote:
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
> 
> Add the implementation of the in-kernel fault handler.
> 
> When a tag fault happens on a kernel address:
> * MTE is disabled on the current CPU,
> * the execution continues.
> 
> When a tag fault happens on a user address:
> * the kernel executes do_bad_area() and panics.
> 
> The tag fault handler for kernel addresses is currently empty and will be
> filled in by a future commit.
> 
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

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

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:45AM +0100, Andrey Konovalov wrote:
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 06ba6c923ab7..fcfbefcc3174 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -121,6 +121,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>  	return ptr;
>  }
>  
> +void __init mte_init_tags(u64 max_tag)
> +{
> +	/* Enable MTE Sync Mode for EL1. */
> +	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> +	isb();
> +}

Is this going to be called on each CPU? I quickly went through the rest
of the patches and couldn't see how.

-- 
Catalin

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Andrey Konovalov]

On Thu, Nov 5, 2020 at 3:14 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
>
>
> On 11/5/20 12:14 PM, Andrey Konovalov wrote:
> > On Thu, Nov 5, 2020 at 12:39 PM Vincenzo Frascino
> > <vincenzo.frascino@arm.com> wrote:
> >>
> >> On 11/5/20 11:35 AM, Andrey Konovalov wrote:
> >>> This will work. Any preference on the name of this function?
> >>>
> >>
> >> I called it in my current iteration mte_enable(), and calling it from
> >> cpu_enable_mte().
> >>
> >>> Alternatively we can rename mte_init_tags() to something else and let
> >>> it handle both RRND and sync/async.
> >>
> >> This is an option but then you need to change the name of kasan_init_tags and
> >> the init_tags indirection name as well. I would go for the simpler and just
> >> splitting the function as per above.
> >>
> >> What do you think?
> >
> > OK, let's split. mte_enable() as a name sounds good to me. Both
> > functions will still be called one right after another from
> > kasan_init_hw_tags (as it's now called) though. I think the name
> > works, as it means initializing the hw_tags mode, not just the tags.
> >
>
> I agree. When you finish with v9, could you please provide a tree with both the
> sets on top similar to [1]? I would like to repeat the tests (ltp + kselftests)
> and even to rebase my async code on top of it since we are aligning with the
> development.
>
> [1] https://github.com/xairy/linux/tree/up-boot-mte-v1

Sure, will share the trees, probably later today.

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Andrey Konovalov]

On Thu, Nov 5, 2020 at 6:26 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Nov 05, 2020 at 12:18:45AM +0100, Andrey Konovalov wrote:
> > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index 06ba6c923ab7..fcfbefcc3174 100644
> > --- a/arch/arm64/kernel/mte.c
> > +++ b/arch/arm64/kernel/mte.c
> > @@ -121,6 +121,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> >       return ptr;
> >  }
> >
> > +void __init mte_init_tags(u64 max_tag)
> > +{
> > +     /* Enable MTE Sync Mode for EL1. */
> > +     sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> > +     isb();
> > +}
>
> Is this going to be called on each CPU? I quickly went through the rest
> of the patches and couldn't see how.

Yes, on each CPU. This is done via kasan_init_hw_tags() that is called
from cpu_enable_mte(). This change is added in the "kasan, arm64:
implement HW_TAGS runtime". Would it make sense to put it into a
separate patch?

Re: [PATCH v8 32/43] arm64: mte: Switch GCR_EL1 in kernel entry and exit

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:47AM +0100, Andrey Konovalov wrote:
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 14b0c19a33e3..cc7e0f8707f7 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);
> @@ -123,6 +125,23 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>  
>  void __init mte_init_tags(u64 max_tag)
>  {
> +	static bool gcr_kernel_excl_initialized = false;
> +
> +	if (!gcr_kernel_excl_initialized) {
> +		/*
> +		 * The format of the tags in KASAN is 0xFF and in MTE is 0xF.
> +		 * This conversion extracts an MTE tag from a KASAN tag.
> +		 */
> +		u64 incl = GENMASK(FIELD_GET(MTE_TAG_MASK >> MTE_TAG_SHIFT,
> +					     max_tag), 0);
> +
> +		gcr_kernel_excl = ~incl & SYS_GCR_EL1_EXCL_MASK;
> +		gcr_kernel_excl_initialized = true;
> +	}
> +
> +	/* Enable the kernel exclude mask for random tags generation. */
> +	write_sysreg_s(SYS_GCR_EL1_RRND | gcr_kernel_excl, SYS_GCR_EL1);

Same question as on a previous patch. Is SYS_GCR_EL1 written on the
other registers via cpu_enable_mte()?

-- 
Catalin

Re: [PATCH v8 37/43] kasan, arm64: expand CONFIG_KASAN checks

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:52AM +0100, Andrey Konovalov wrote:
> Some #ifdef CONFIG_KASAN checks are only relevant for software KASAN
> modes (either related to shadow memory or compiler instrumentation).
> Expand those into CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

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

Re: [PATCH v8 32/43] arm64: mte: Switch GCR_EL1 in kernel entry and exit

[Andrey Konovalov]

On Thu, Nov 5, 2020 at 6:30 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Nov 05, 2020 at 12:18:47AM +0100, Andrey Konovalov wrote:
> > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index 14b0c19a33e3..cc7e0f8707f7 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);
> > @@ -123,6 +125,23 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> >
> >  void __init mte_init_tags(u64 max_tag)
> >  {
> > +     static bool gcr_kernel_excl_initialized = false;
> > +
> > +     if (!gcr_kernel_excl_initialized) {
> > +             /*
> > +              * The format of the tags in KASAN is 0xFF and in MTE is 0xF.
> > +              * This conversion extracts an MTE tag from a KASAN tag.
> > +              */
> > +             u64 incl = GENMASK(FIELD_GET(MTE_TAG_MASK >> MTE_TAG_SHIFT,
> > +                                          max_tag), 0);
> > +
> > +             gcr_kernel_excl = ~incl & SYS_GCR_EL1_EXCL_MASK;
> > +             gcr_kernel_excl_initialized = true;
> > +     }
> > +
> > +     /* Enable the kernel exclude mask for random tags generation. */
> > +     write_sysreg_s(SYS_GCR_EL1_RRND | gcr_kernel_excl, SYS_GCR_EL1);
>
> Same question as on a previous patch. Is SYS_GCR_EL1 written on the
> other registers via cpu_enable_mte()?

You mean for other CPUs? mte_init_tags() is called for each CPU if
that's what you mean. Otherwise, please clarify the question.

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Catalin Marinas]

On Thu, Nov 05, 2020 at 06:29:17PM +0100, Andrey Konovalov wrote:
> On Thu, Nov 5, 2020 at 6:26 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> >
> > On Thu, Nov 05, 2020 at 12:18:45AM +0100, Andrey Konovalov wrote:
> > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > > index 06ba6c923ab7..fcfbefcc3174 100644
> > > --- a/arch/arm64/kernel/mte.c
> > > +++ b/arch/arm64/kernel/mte.c
> > > @@ -121,6 +121,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> > >       return ptr;
> > >  }
> > >
> > > +void __init mte_init_tags(u64 max_tag)
> > > +{
> > > +     /* Enable MTE Sync Mode for EL1. */
> > > +     sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> > > +     isb();
> > > +}
> >
> > Is this going to be called on each CPU? I quickly went through the rest
> > of the patches and couldn't see how.
> 
> Yes, on each CPU. This is done via kasan_init_hw_tags() that is called
> from cpu_enable_mte(). This change is added in the "kasan, arm64:
> implement HW_TAGS runtime".

Ah, I got there eventually in patch 38. Too many indirections ;) (I'm
sure we could have trimmed them down a bit, hw_init_tags ==
arch_init_tags == mte_init_tags).

> Would it make sense to put it into a separate patch?

I think that's fine. I had the impression that kasan_init_hw_tags()
should only be called once.

-- 
Catalin

Re: [PATCH v8 32/43] arm64: mte: Switch GCR_EL1 in kernel entry and exit

[Catalin Marinas]

On Thu, Nov 05, 2020 at 12:18:47AM +0100, Andrey Konovalov wrote:
> 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>
> Co-developed-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

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

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Andrey Konovalov]

On Thu, Nov 5, 2020 at 6:39 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Nov 05, 2020 at 06:29:17PM +0100, Andrey Konovalov wrote:
> > On Thu, Nov 5, 2020 at 6:26 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > >
> > > On Thu, Nov 05, 2020 at 12:18:45AM +0100, Andrey Konovalov wrote:
> > > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > > > index 06ba6c923ab7..fcfbefcc3174 100644
> > > > --- a/arch/arm64/kernel/mte.c
> > > > +++ b/arch/arm64/kernel/mte.c
> > > > @@ -121,6 +121,13 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
> > > >       return ptr;
> > > >  }
> > > >
> > > > +void __init mte_init_tags(u64 max_tag)
> > > > +{
> > > > +     /* Enable MTE Sync Mode for EL1. */
> > > > +     sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> > > > +     isb();
> > > > +}
> > >
> > > Is this going to be called on each CPU? I quickly went through the rest
> > > of the patches and couldn't see how.
> >
> > Yes, on each CPU. This is done via kasan_init_hw_tags() that is called
> > from cpu_enable_mte(). This change is added in the "kasan, arm64:
> > implement HW_TAGS runtime".
>
> Ah, I got there eventually in patch 38. Too many indirections ;) (I'm
> sure we could have trimmed them down a bit, hw_init_tags ==
> arch_init_tags == mte_init_tags).

The idea with these indirections was to make hw_tags.c to not directly
call MTE stuff and abstract away the underlying memory tagging
implementation. We won't know for sure how fitting these abstractions
are before we add another memory tagging implementation though :)

> > Would it make sense to put it into a separate patch?
>
> I think that's fine. I had the impression that kasan_init_hw_tags()
> should only be called once.

This was the case before, but not anymore. I've also added a comment
before the kasan_init_hw_tags() definition.

Re: [PATCH v8 30/43] arm64: kasan: Allow enabling in-kernel MTE

[Vincenzo Frascino]

Hi Catalin,

On 11/5/20 6:09 PM, Andrey Konovalov wrote:
>> Ah, I got there eventually in patch 38. Too many indirections ;) (I'm
>> sure we could have trimmed them down a bit, hw_init_tags ==
>> arch_init_tags == mte_init_tags).
> The idea with these indirections was to make hw_tags.c to not directly
> call MTE stuff and abstract away the underlying memory tagging
> implementation. We won't know for sure how fitting these abstractions
> are before we add another memory tagging implementation though :)
> 

I agree with Andrey, because in future the sparc architecture might want to
support HW KASAN hence keeping a generic infrastructure adds some value.

-- 
Regards,
Vincenzo

Re: [PATCH v8 28/43] arm64: mte: Reset the page tag in page->flags

[Vincenzo Frascino]

Hi Catalin,

On 11/5/20 3:59 PM, Catalin Marinas wrote:
> On Thu, Nov 05, 2020 at 12:18:43AM +0100, Andrey Konovalov wrote:
>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index 8f99c65837fd..06ba6c923ab7 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -34,6 +34,7 @@ static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>>  			return;
>>  	}
>>  
>> +	page_kasan_tag_reset(page);
>>  	mte_clear_page_tags(page_address(page));
> 
> I think we need an smp_wmb() between setting the flags and clearing the
> actual tags. If another threads reads page->flags and builds a tagged
> address out of it (see page_to_virt) there's an address dependency to
> the actual memory access. However, on the current thread, we don't
> guarantee that the new page->flags are visible before the tags were
> updated.
> 

Indeed, and I will add a comment as well to explain why.

>>  }
>>  
>> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
>> index 70a71f38b6a9..348f4627da08 100644
>> --- a/arch/arm64/mm/copypage.c
>> +++ b/arch/arm64/mm/copypage.c
>> @@ -22,6 +22,7 @@ void copy_highpage(struct page *to, struct page *from)
>>  	copy_page(kto, kfrom);
>>  
>>  	if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) {
>> +		page_kasan_tag_reset(to);
>>  		set_bit(PG_mte_tagged, &to->flags);
>>  		mte_copy_page_tags(kto, kfrom);
> 
> Nitpick: move page_kasan_tag_reset() just above mte_copy_page_tags() for
> consistency with the other places where PG_mte_tagged is set before or
> after the actual tag setting.
> 

Fine, I will add it to the next iteration.

>>  	}
>> diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
>> index c52c1847079c..0e7eccbe598a 100644
>> --- a/arch/arm64/mm/mteswap.c
>> +++ b/arch/arm64/mm/mteswap.c
>> @@ -53,6 +53,7 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page)
>>  	if (!tags)
>>  		return false;
>>  
>> +	page_kasan_tag_reset(page);
>>  	mte_restore_page_tags(page_address(page), tags);

I just realized based on your comment above that we need smp_wmb() here as well.
I will add it with a comment as well.

> 
> There is another mte_restore_page_tags() caller in hibernate.c. That one
> doesn't need page_kasan_tag_reset() since the page->flags would have
> been already restored but please add a comment in that file why its not
> needed.
> 

Yes I will do, I agree on the reasoning, I will report it in the comments.

-- 
Regards,
Vincenzo