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

[sashiko-bot@kernel.org]

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.

> +	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