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

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

Hi Will,

> On Wed, Sep 03, 2025 at 04:00:19PM +0100, Yeoreum Yun wrote:
> > Since Armv8.9, FEATURE_MTE_STORE_ONLY feature is introduced to restrict
> > raise of tag check fault on store operation only.
> > Introcude KASAN write only mode based on this feature.
>
> Typo ^^

Thanks.

>
> >
> > KASAN write only mode restricts KASAN checks operation for write only and
> > omits the checks for fetch/read operations when accessing memory.
> > So it might be used not only debugging enviroment but also normal
> > enviroment to check memory safty.
> >
> > This features can be controlled with "kasan.write_only" arguments.
> > When "kasan.write_only=on", KASAN checks write operation only otherwise
> > KASAN checks all operations.
> >
> > This changes the MTE_STORE_ONLY feature as BOOT_CPU_FEATURE like
> > ARM64_MTE_ASYMM so that makes it initialise in kasan_init_hw_tags()
> > with other function together.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> >  Documentation/dev-tools/kasan.rst  |  3 ++
> >  arch/arm64/include/asm/memory.h    |  1 +
> >  arch/arm64/include/asm/mte-kasan.h |  6 +++
> >  arch/arm64/kernel/cpufeature.c     |  2 +-
> >  arch/arm64/kernel/mte.c            | 18 ++++++++
> >  mm/kasan/hw_tags.c                 | 70 +++++++++++++++++++++++++++++-
> >  mm/kasan/kasan.h                   |  7 +++
> >  7 files changed, 104 insertions(+), 3 deletions(-)
>
> [...]
>
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index 9a6927394b54..d5b5fb47d52b 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,9 @@ DEFINE_STATIC_KEY_FALSE(kasan_flag_vmalloc);
> >  #endif
> >  EXPORT_SYMBOL_GPL(kasan_flag_vmalloc);
> >
> > +/* Whether to check write accesses only. */
> > +static bool kasan_flag_write_only = false;
> > +
> >  #define PAGE_ALLOC_SAMPLE_DEFAULT	1
> >  #define PAGE_ALLOC_SAMPLE_ORDER_DEFAULT	3
> >
> > @@ -141,6 +151,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 +284,28 @@ void __init kasan_init_hw_tags(void)
> >  		break;
> >  	}
> >
> > +	switch (kasan_arg_write_only) {
> > +	case KASAN_ARG_WRITE_ONLY_DEFAULT:
> > +		/* Default is specified by kasan_flag_write_only definition. */
> > +		break;
> > +	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();
>
> I'm probably missing something here, but why have 'enum
> kasan_arg_write_only' at all? What stops you from setting
> 'kasan_flag_write_only' directly from early_kasan_flag_write_only()?
>
> This all looks weirdly over-engineered, as though 'kasan_flag_write_only'
> is expected to be statically initialised to something other than 'false'.

For the conherent pattern for other options.
Since other options manage arg value and internal state separately,
I just followed former ancestor.

>
> >  	/* 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));
>
> It's also confusing, because now you appear to be passing the funny new
> 'enum' type to str_on_off(), which expects a bool.

Oops. This is my mistake from v3 :(
Thanks to point out ///

--
Sincerely,
Yeoreum Yun