Re: [PATCH v4 4/4] KVM: arm64: Add hyp_printk event to nVHE/pKVM hyp

[Vincent Donnefort <vdonnefort@google.com>]

On Thu, Oct 01, 2026 at 09:07:09AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] kvm: trace_hyp_printk lacks format string validation leading to host panic/leak
> - [Medium] arm64: unaligned _hyp_string_fmts section start leads to pointer corruption
> - [Medium] kvm: out-of-bounds read in __hyp_trace_printk when trace sequence is empty
> - [Medium] kvm: uninitialized padding in remote_event_format_hyp_printk leaks hyp memory
> --
> 
> commit bc7c47888c5fbd307a50212081f15f7498660a9b
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> KVM: arm64: Add hyp_printk event to nVHE/pKVM hyp
> 
> Create an event to allow developers to log pretty much anything into the
> hypervisor tracing buffer with trace_hyp_printk(), just like the kernel
> tracing has the function trace_printk().
> 
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index af1d720209764..7f9b6c7dfe70f 100644
> > --- a/arch/arm64/kernel/vmlinux.lds.S
> > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > @@ -324,6 +324,10 @@ SECTIONS
> >  		__hyp_events_start = .;
> >  		*(SORT(_hyp_events.*))
> >  		__hyp_events_end = .;
> > +
> > +		__hyp_string_fmts_start = .;
> > +		*(_hyp_string_fmts)
> > +		__hyp_string_fmts_end = .;
> >  	}
> >  #endif
> 
> [Severity: Medium]
> Could __hyp_string_fmts_start capture an unaligned address before section
> padding is applied? The compiler might align struct hyp_string_fmt
> elements, but without an ALIGN() directive, __hyp_string_fmts_start
> captures the location counter before any alignment padding. When calculating
> fmt_id in the hypervisor (fmt - __hyp_string_fmts_start), this unaligned
> offset might yield incorrect values and cause memory access errors.

arg, that is correct, it depends on the number of events...

I'll move into its own .rodata.hyp_string_fmts output section.

And make the hyp_string_fmts struct const at the same time...

> 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/trace.h b/arch/arm64/kvm/hyp/include/nvhe/trace.h
> > index 8813ff250f8e0..fa50720e2cffd 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/trace.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/trace.h
> [ ... ]
> > @@ -46,6 +46,70 @@ static inline pid_t __tracing_get_vcpu_pid(struct kvm_cpu_context *host_ctxt)
> >  void *tracing_reserve_entry(unsigned long length);
> >  void tracing_commit_entry(void);
> >  
> > +/*
> > + * The trace_hyp_printk boilerplate is too fiddly to be declared with
> > + * HYP_EVENT():
> > + *
> > + * The string format is stored into a kernel-accessible ELF section. The
> > + * hypervisor only writes the format ID.
> > + *
> > + * The function has a variadic prototype. We have no easy way to know each
> > + * argument width so they must all cast to u64.
> > + */
> > +#define REMOTE_EVENT_CUSTOM_PRINTK(...)
> > +
> > +#define __TO_U64_0()
> > +#define __TO_U64_1(x)			, (u64)(x)
> > +#define __TO_U64_2(x, ...)		, (u64)(x) __TO_U64_1(__VA_ARGS__)
> > +#define __TO_U64_3(x, ...)		, (u64)(x) __TO_U64_2(__VA_ARGS__)
> > +#define __TO_U64_4(x, ...)		, (u64)(x) __TO_U64_3(__VA_ARGS__)
> > +#define __TO_U64_5(x, ...)		, (u64)(x) __TO_U64_4(__VA_ARGS__)
> > +#define __TO_U64_6(x, ...)		, (u64)(x) __TO_U64_5(__VA_ARGS__)
> > +#define __TO_U64_7(x, ...)		, (u64)(x) __TO_U64_6(__VA_ARGS__)
> > +#define __TO_U64_8(x, ...)		, (u64)(x) __TO_U64_7(__VA_ARGS__)
> > +
> > +#define __TO_U64_X(N, ...)		CONCATENATE(__TO_U64_, N)(__VA_ARGS__)
> > +#define __TO_U64(...)			__TO_U64_X(COUNT_ARGS(__VA_ARGS__), ##__VA_ARGS__)
> > +
> > +REMOTE_EVENT_FORMAT(hyp_printk, HE_STRUCT(he_field(u16, fmt_id) he_field(u64, args[])));
> 
> [Severity: Medium]
> Can this leak hypervisor memory to the host or userspace through uninitialized
> struct padding? The REMOTE_EVENT_FORMAT definition creates a structure with
> u16 fmt_id followed by a u64 args[] array, which introduces padding bytes
> before the array to satisfy the 8-byte alignment requirement of u64. Because
> tracing_reserve_entry() does not zero the allocated space, these padding bytes
> are never initialized before tracing_commit_entry() exposes them.

That memory is kernel-shared memory anyway. I do not believe this is an issue.

> 
> > +extern struct hyp_event_id hyp_event_id_hyp_printk;
> > +
> > +static __always_inline void __trace_hyp_printk(struct hyp_string_fmt *fmt, int nr_args, ...)
> > +{
> > +	struct remote_event_format_hyp_printk *entry;
> > +	va_list va;
> > +	int i;
> > +
> > +	if (!atomic_read(&hyp_event_id_hyp_printk.enabled))
> > +		return;
> > +
> > +	entry = tracing_reserve_entry(struct_size(entry, args, nr_args));
> > +	if (!entry)
> > +		return;
> > +
> > +	entry->hdr.id = hyp_event_id_hyp_printk.id;
> > +	entry->fmt_id = fmt - __hyp_string_fmts_start;
> > +
> > +	va_start(va, nr_args);
> > +	for (i = 0; i < nr_args; i++)
> > +		entry->args[i] = va_arg(va, u64);
> > +	va_end(va);
> > +
> > +	tracing_commit_entry();
> > +}
> > +
> > +#define trace_hyp_printk(__fmt, __args...)						\
> > +do {											\
> > +	static struct hyp_string_fmt __used __section("_hyp_string_fmts")		\
> > +	__hyp_str_fmt  = {								\
> > +		.fmt		= __fmt,						\
> > +		.nr_args	= COUNT_ARGS(__args),					\
> > +	};										\
> > +	BUILD_BUG_ON(sizeof(__fmt) > HYP_STRING_FMT_MAX_SIZE);				\
> > +	/* __TO_U64 prepends a comma if there are arguments */				\
> > +	__trace_hyp_printk(&__hyp_str_fmt, COUNT_ARGS(__args) __TO_U64(__args));	\
> > +} while (0)
> 
> [Severity: High]
> Does this macro validate that the provided format string matches the given
> arguments? Without checking the format string (e.g. using __printf attributes
> or if (0) printk(...) type-checking), a mismatch like
> trace_hyp_printk("val=%d") will set nr_args to 0.

It is impossible to use __printf here due to the u64 cast.

> 
> The host kernel's __hyp_trace_printk() will then unconditionally call
> trace_seq_printf(seq, fmt); with no arguments, potentially causing
> out-of-bounds reads or invalid dereferences when it parses the %d or %s
> specifiers.
> 
> > diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> > index 8faaec899f92c..ebb882d0152b6 100644
> > --- a/arch/arm64/kvm/hyp_trace.c
> > +++ b/arch/arm64/kvm/hyp_trace.c
> [ ... ]
> > @@ -421,6 +424,58 @@ static const char *__hyp_enter_exit_reason_str(u8 reason)
> >  	return strs[min(reason, HYP_REASON_UNKNOWN)];
> >  }
> >  
> > +static void __hyp_trace_printk(struct remote_event_format_hyp_printk *entry, struct trace_seq *seq)
> > +{
> [ ... ]
> > +	switch (hyp_str_fmt->nr_args) {
> > +	case 0:
> > +		trace_seq_printf(seq, fmt);
> > +		break;
> > +	case 1:
> > +		trace_seq_printf(seq, fmt, entry->args[0]);
> > +		break;
> [ ... ]
> > +	}
> > +
> > +	if (seq->buffer[trace_seq_used(seq) - 1] != '\n')
> > +		trace_seq_putc(seq, '\n');
> 
> [Severity: Medium]
> Is it possible for trace_seq_used(seq) to be 0 here? If the sequence buffer
> was empty or in an overflowed state when passed to this function, subtracting
> 1 would result in an out-of-bounds array read (accessing seq->buffer[-1]).
> Would adding a check for trace_seq_used(seq) > 0 prevent this?

I will fix that too.

> 
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261001084909.2937943-1-vdonnefort@google.com?part=4

I will also change the "\"Unknown\"" even though we do not support yet external
tooling.

-- 
Vincent