Re: [PATCH v3 11/14] x86: Handle int3 for inline KASAN reports

[Dave Hansen <dave.hansen@intel.com>]

On 4/4/25 06:14, Maciej Wieczor-Retman wrote:
> When a tag mismatch happens in inline software tag-based KASAN on x86 an
> int3 instruction is executed and needs proper handling.

Does this mean "inline software"? Or "inline" functions? I'm not quite
parsing that. I think it needs some more background.

> Call kasan_report() from the int3 handler and pass down the proper
> information from registers - RDI should contain the problematic address
> and RAX other metadata.
> 
> Also early return from the int3 selftest if inline KASAN is enabled
> since it will cause a kernel panic otherwise.
...
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index bf82c6f7d690..ba277a25b57f 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1979,6 +1979,9 @@ static noinline void __init int3_selftest(void)
>  	};
>  	unsigned int val = 0;
>  
> +	if (IS_ENABLED(CONFIG_KASAN_INLINE))
> +		return;

Comments, please. This is a total non sequitur otherwise.

>  	BUG_ON(register_die_notifier(&int3_exception_nb));
>  
>  	/*
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 9f88b8a78e50..32c81fc2d439 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
...
> @@ -849,6 +850,51 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  	cond_local_irq_disable(regs);
>  }
>  
> +#ifdef CONFIG_KASAN_SW_TAGS
> +
> +#define KASAN_RAX_RECOVER	0x20
> +#define KASAN_RAX_WRITE	0x10
> +#define KASAN_RAX_SIZE_MASK	0x0f
> +#define KASAN_RAX_SIZE(rax)	(1 << ((rax) & KASAN_RAX_SIZE_MASK))

This ABI _looks_ like it was conjured out out of thin air. I assume it's
coming from the compiler. Any pointers to that ABI definition in or out
of the kernel would be appreciated.

> +static bool kasan_handler(struct pt_regs *regs)
> +{
> +	int metadata = regs->ax;
> +	u64 addr = regs->di;
> +	u64 pc = regs->ip;
> +	bool recover = metadata & KASAN_RAX_RECOVER;
> +	bool write = metadata & KASAN_RAX_WRITE;
> +	size_t size = KASAN_RAX_SIZE(metadata);

"metadata" is exactly the same length as "regs->ax", so it seems a
little silly. Also, please use vertical alignment as a tool to make code
more readable. Isn't this much more readable?

	bool recover = regs->ax & KASAN_RAX_RECOVER;
	bool write   = regs->ax & KASAN_RAX_WRITE;
	size_t size  = KASAN_RAX_SIZE(regs->ax);
	u64 addr     = regs->di;
	u64 pc       = regs->ip;

> +	if (!IS_ENABLED(CONFIG_KASAN_INLINE))
> +		return false;
> +
> +	if (user_mode(regs))
> +		return false;
> +
> +	kasan_report((void *)addr, size, write, pc);
> +
> +	/*
> +	 * The instrumentation allows to control whether we can proceed after
> +	 * a crash was detected. This is done by passing the -recover flag to
> +	 * the compiler. Disabling recovery allows to generate more compact
> +	 * code.
> +	 *
> +	 * Unfortunately disabling recovery doesn't work for the kernel right
> +	 * now. KASAN reporting is disabled in some contexts (for example when
> +	 * the allocator accesses slab object metadata; this is controlled by
> +	 * current->kasan_depth). All these accesses are detected by the tool,
> +	 * even though the reports for them are not printed.
> +	 *
> +	 * This is something that might be fixed at some point in the future.
> +	 */

Can we please find a way to do this that doesn't copy and paste a rather
verbose comment?

What if we passed 'recover' into kasan_report() and had it do the die()?

> +	if (!recover)
> +		die("Oops - KASAN", regs, 0);
> +	return true;
> +}
> +
> +#endif
> +
>  static bool do_int3(struct pt_regs *regs)
>  {
>  	int res;
> @@ -863,6 +909,12 @@ static bool do_int3(struct pt_regs *regs)
>  	if (kprobe_int3_handler(regs))
>  		return true;
>  #endif
> +
> +#ifdef CONFIG_KASAN_SW_TAGS
> +	if (kasan_handler(regs))
> +		return true;
> +#endif
I won't get _too_ grumbly about ti since there's another culprit right
above, but the "no #fidefs in .c files" rule still applies. The right
way to do this is with a stub kasan_handler() in a header with the
#ifdef in the header.

Actually, ditto on the kasan_handler() #ifdef. I suspect it can go away
too and be replaced with a IS_ENABLED(CONFIG_KASAN_SW_TAGS) check.