linux-kernel - Re: [PATCH v7 1/2] kasan/hw-tags: introduce kasan.write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aMnGUr9zeutyPpAg@e129823.arm.com>
Date: Tue, 16 Sep 2025 21:19:30 +0100
From: Yeoreum Yun <yeoreum.yun@....com>
To: Will Deacon <will@...nel.org>
Cc: ryabinin.a.a@...il.com, glider@...gle.com, andreyknvl@...il.com,
	dvyukov@...gle.com, vincenzo.frascino@....com, corbet@....net,
	catalin.marinas@....com, akpm@...ux-foundation.org,
	scott@...amperecomputing.com, jhubbard@...dia.com,
	pankaj.gupta@....com, leitao@...ian.org, kaleshsingh@...gle.com,
	maz@...nel.org, broonie@...nel.org, oliver.upton@...ux.dev,
	james.morse@....com, ardb@...nel.org,
	hardevsinh.palaniya@...iconsignals.io, david@...hat.com,
	yang@...amperecomputing.com, kasan-dev@...glegroups.com,
	workflows@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mm@...ck.org
Subject: Re: [PATCH v7 1/2] kasan/hw-tags: introduce kasan.write_only option

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@....com>
> > Reviewed-by: Andrey Konovalov <andreyknvl@...il.com>
> > Reviewed-by: Catalin Marinas <catalin.marinas@....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