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

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Sep 30, 2026 at 03:56:55PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] trace: trace_hyp_printk calculates invalid ring_buffer_event pointer causing memory corruption
> - [Medium] trace: trace_hyp_printk lacks compile-time format string validation
> - [Medium] trace: trace_hyp_printk macro shadows caller variables named fmt
> --
> 
> commit 26cdec70ff2d60c665c546592ecab1e31f40820e
> 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/kvm/hyp/include/nvhe/trace.h b/arch/arm64/kvm/hyp/include/nvhe/trace.h
> > index 8813ff250f8e0..3d0b5c634bb3f 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/trace.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/trace.h
> [ ... ]
> > +
> > +#define trace_hyp_printk(__fmt, __args...)						\
> > +do {											\
> > +	static struct hyp_string_fmt __used __section("_hyp_string_fmts") fmt = {	\
> 
> [Severity: Medium]
> Will this shadow caller variables named fmt? If a caller passes a local
> variable named fmt as one of the format arguments, it seems it would resolve
> to this static struct instead of the intended variable, logging the struct
> address or causing a compilation error.
> 
> [Severity: Medium]
> Does this macro validate the format string at compile time?
> 
> Since __trace_hyp_printk() does not use a __printf compiler attribute, passing
> unsupported specifiers like %s or %p, or providing mismatched arguments, won't
> trigger a compiler warning. This could cause a host panic when
> trace_seq_printf() attempts to dereference an EL2 pointer at runtime.
> 
> > +		.fmt = __fmt								\
> > +	};										\
> > +	BUILD_BUG_ON(sizeof(__fmt) > HYP_STRING_FMT_MAX_SIZE);				\
> > +	/* __TO_U64 prepends a comma if there are arguments */				\
> > +	__trace_hyp_printk(&fmt, COUNT_ARGS(__args) __TO_U64(__args));			\
> > +} while (0)
> >  
> >  int __tracing_load(unsigned long desc_va, size_t desc_size);
> [ ... ]
> > diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> > index 8faaec899f92c..9d4378bcad74b 100644
> > --- a/arch/arm64/kvm/hyp_trace.c
> > +++ b/arch/arm64/kvm/hyp_trace.c
> [ ... ]
> > @@ -421,6 +424,61 @@ 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)
> > +{
> > +	const char *fmt = (const char *)(&__hyp_string_fmts_start[entry->fmt_id]);
> > +	struct ring_buffer_event *evt = (void *)entry - RB_EVNT_HDR_SIZE;
> 
> [Severity: High]
> Does this pointer arithmetic correctly locate the event header?
> 
> Since this event is allocated by simple_ring_buffer_reserve(), the payload
> starts at array[1] (offset 8) rather than immediately after a standard ring
> buffer header. Subtracting RB_EVNT_HDR_SIZE (4 bytes) appears to result in a
> misaligned pointer pointing to array[0] instead of the event header.

Lol, fun I missed that in the testing because printf behaves quite well with the
format string.

Anyway, it's even worse than Sahiko thinks because I am adding support for
compressed length here [1]. As a consequence it is a bit dodgy to know the event
size. I'll store the number of arguments directly into struct hyp_string_fmt
instead.

[1] https://lore.kernel.org/all/20260924145754.1285146-8-vdonnefort@google.com/

> 
> > +	int nr_args;
> > +
> > +	trace_seq_putc(seq, ' ');
> > +
> > +	if ((void *)fmt >= (void *)__hyp_string_fmts_end) {
> > +		trace_seq_printf(seq, "Unknown hyp_string_fmt ID %d\n", entry->fmt_id);
> > +		return;
> > +	}
> > +
> > +	nr_args = (ring_buffer_event_length(evt) -
> > +		   offsetof(struct remote_event_format_hyp_printk, args)) / sizeof(entry->args[0]);
> 
> [Severity: High]
> Does ring_buffer_event_length() read garbage data here?
> 
> If the evt pointer is misaligned, interpreting the payload data as the event
> header could yield a garbage nr_args value. This could lead to out-of-bounds
> memory reads or information leaks of uninitialized host stack/registers when
> trace_seq_printf() is called below.
> 
> > +	switch (nr_args) {
> > +	case 0:
> > +		trace_seq_printf(seq, fmt);
> > +		break;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260930153505.2144559-1-vdonnefort@google.com?part=4

-- 
Vincent