Re: [PATCH v4 1/2] kasan/hw-tags: introduce kasan.write_only option

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Ben,

[..]

> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2920,7 +2920,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> >   	{
> >   		.desc = "Store Only MTE Tag Check",
> >   		.capability = ARM64_MTE_STORE_ONLY,
> > -		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > +		.type = ARM64_CPUCAP_BOOT_CPU_FEATURE,
> >   		.matches = has_cpuid_feature,
> >   		ARM64_CPUID_FIELDS(ID_AA64PFR2_EL1, MTESTOREONLY, IMP)
> >   	},
> > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index e5e773844889..cd5452eb7486 100644
> > --- a/arch/arm64/kernel/mte.c
> > +++ b/arch/arm64/kernel/mte.c
> > @@ -157,6 +157,24 @@ void mte_enable_kernel_asymm(void)
> >   		mte_enable_kernel_sync();
> >   	}
> >   }
> > +
> > +int mte_enable_kernel_store_only(void)
> > +{
> > +	/*
> > +	 * If the CPU does not support MTE store only,
> > +	 * the kernel checks all operations.
> > +	 */
> > +	if (!cpus_have_cap(ARM64_MTE_STORE_ONLY))
> > +		return -EINVAL;

> Would it be better to make this function return void

This is the same point Catalin points out from patch v2.
But for usage of kunit test, it need to keep return as int.

> and add a static key in
> the manner of mte_async_or_asymm_mode, perhaps mte_store_only_mode? This
> information could then be used to help determine whether it is required to
> enable and disable tco in __get_kernel_nofault() and
> load_unaligned_zeropad().

Yes. Since the mte_store_only enabled, it doesn't need to enable tco
since load/fetch doesn't increase the TSFR.
However This sounds like an over optimisation.
I think it would be enough to check when mte_async_or_asymm_mode()
otherwise in for __get_kernel_nofault() or load_unaligned_zeropad(),
it should use like:
  - __mte_enable_tco_async_and_store_only()
  or
  - __mte_enable_tco_async(op) // whether op is load or store (boolean or enum)

But this seems ugly too.

So I think it woule be better to remain as it is --
without static_key for store only since there is no usage.

>
> > +
> > +	sysreg_clear_set(sctlr_el1, SCTLR_EL1_TCSO_MASK,
> > +			 SYS_FIELD_PREP(SCTLR_EL1, TCSO, 1));
> > +	isb();
> > +
> > +	pr_info_once("MTE: enabled stonly mode at EL1\n");
> nit: stonly can be expanded to store only

Thanks. I'll change it.


> > +
> > +	return 0;
> > +}
> >   #endif
> >   #ifdef CONFIG_KASAN_HW_TAGS
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index 9a6927394b54..df67b48739b4 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -41,9 +41,16 @@ enum kasan_arg_vmalloc {
> >   	KASAN_ARG_VMALLOC_ON,
> >   };
> > +enum kasan_arg_write_only {
> > +	KASAN_ARG_WRITE_ONLY_DEFAULT,
> > +	KASAN_ARG_WRITE_ONLY_OFF,
> > +	KASAN_ARG_WRITE_ONLY_ON,
> > +};
> > +
> >   static enum kasan_arg kasan_arg __ro_after_init;
> >   static enum kasan_arg_mode kasan_arg_mode __ro_after_init;
> >   static enum kasan_arg_vmalloc kasan_arg_vmalloc __initdata;
> > +static enum kasan_arg_write_only kasan_arg_write_only __ro_after_init;
> >   /*
> >    * Whether KASAN is enabled at all.
> > @@ -67,6 +74,8 @@ DEFINE_STATIC_KEY_FALSE(kasan_flag_vmalloc);
> >   #endif
> >   EXPORT_SYMBOL_GPL(kasan_flag_vmalloc);
> > +static bool kasan_flag_write_only;
> > +
> >   #define PAGE_ALLOC_SAMPLE_DEFAULT	1
> >   #define PAGE_ALLOC_SAMPLE_ORDER_DEFAULT	3
> > @@ -141,6 +150,23 @@ static int __init early_kasan_flag_vmalloc(char *arg)
> >   }
> >   early_param("kasan.vmalloc", early_kasan_flag_vmalloc);
> > +/* kasan.write_only=off/on */
> > +static int __init early_kasan_flag_write_only(char *arg)
> > +{
> > +	if (!arg)
> > +		return -EINVAL;
> > +
> > +	if (!strcmp(arg, "off"))
> > +		kasan_arg_write_only = KASAN_ARG_WRITE_ONLY_OFF;
> > +	else if (!strcmp(arg, "on"))
> > +		kasan_arg_write_only = KASAN_ARG_WRITE_ONLY_ON;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +early_param("kasan.write_only", early_kasan_flag_write_only);
> > +
> >   static inline const char *kasan_mode_info(void)
> >   {
> >   	if (kasan_mode == KASAN_MODE_ASYNC)
> > @@ -257,15 +283,26 @@ void __init kasan_init_hw_tags(void)
> >   		break;
> >   	}
> > +	switch (kasan_arg_write_only) {
> > +	case KASAN_ARG_WRITE_ONLY_DEFAULT:
> > +	case KASAN_ARG_WRITE_ONLY_OFF:
> > +		kasan_flag_write_only = false;
> > +		break;
> > +	case KASAN_ARG_WRITE_ONLY_ON:
> > +		kasan_flag_write_only = true;
> > +		break;
> > +	}
> > +
> >   	kasan_init_tags();
> >   	/* KASAN is now initialized, enable it. */
> >   	static_branch_enable(&kasan_flag_enabled);
> > -	pr_info("KernelAddressSanitizer initialized (hw-tags, mode=%s, vmalloc=%s, stacktrace=%s)\n",
> > +	pr_info("KernelAddressSanitizer initialized (hw-tags, mode=%s, vmalloc=%s, stacktrace=%s, write_only=%s\n",
> >   		kasan_mode_info(),
> >   		str_on_off(kasan_vmalloc_enabled()),
> > -		str_on_off(kasan_stack_collection_enabled()));
> > +		str_on_off(kasan_stack_collection_enabled()),
> > +		str_on_off(kasan_arg_write_only));
> >   }
> >   #ifdef CONFIG_KASAN_VMALLOC
> > @@ -392,6 +429,13 @@ void kasan_enable_hw_tags(void)
> >   		hw_enable_tag_checks_asymm();
> >   	else
> >   		hw_enable_tag_checks_sync();
> > +
> > +	if (kasan_arg_write_only == KASAN_ARG_WRITE_ONLY_ON &&
> > +	    hw_enable_tag_checks_write_only()) {
> > +		kasan_arg_write_only == KASAN_ARG_WRITE_ONLY_OFF;
> > +		kasan_flag_write_only = false;
> > +		pr_warn_once("System doesn't support write-only option. Disable it\n");
> > +	}
> >   }
> >   #if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST)
> > @@ -404,4 +448,10 @@ VISIBLE_IF_KUNIT void kasan_force_async_fault(void)
> >   }
> >   EXPORT_SYMBOL_IF_KUNIT(kasan_force_async_fault);
> > +VISIBLE_IF_KUNIT bool kasan_write_only_enabled(void)
> > +{
> > +	return kasan_flag_write_only;
> > +}
> > +EXPORT_SYMBOL_IF_KUNIT(kasan_write_only_enabled);
> > +
> >   #endif
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index 129178be5e64..c1490136c96b 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -428,6 +428,7 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
> >   #define hw_enable_tag_checks_sync()		arch_enable_tag_checks_sync()
> >   #define hw_enable_tag_checks_async()		arch_enable_tag_checks_async()
> >   #define hw_enable_tag_checks_asymm()		arch_enable_tag_checks_asymm()
> > +#define hw_enable_tag_checks_write_only()	arch_enable_tag_checks_write_only()
> >   #define hw_suppress_tag_checks_start()		arch_suppress_tag_checks_start()
> >   #define hw_suppress_tag_checks_stop()		arch_suppress_tag_checks_stop()
> >   #define hw_force_async_tag_fault()		arch_force_async_tag_fault()
> > @@ -437,11 +438,17 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
> >   			arch_set_mem_tag_range((addr), (size), (tag), (init))
> >   void kasan_enable_hw_tags(void);
> > +bool kasan_write_only_enabled(void);
> >   #else /* CONFIG_KASAN_HW_TAGS */
> >   static inline void kasan_enable_hw_tags(void) { }
> > +static inline bool kasan_write_only_enabled(void)
> > +{
> > +	return false;
> > +}
> > +
> >   #endif /* CONFIG_KASAN_HW_TAGS */
> >   #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
>
> Thanks,
>
> Ben
>

--
Sincerely,
Yeoreum Yun