Re: [PATCH v4 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 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.

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

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

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?

> +}

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