Re: [PATCH v1 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 2 potential issue(s) to consider:
- [High] Linker script alignment mismatch causes the host to read padding bytes instead of trace format strings.
- [High] Missing format string validation in `trace_hyp_printk` allows host kernel panics via `%s` and information leaks via argument mismatch.
--

commit d7bc5a6433c1c6dd4e023e0c02d6df0881142324
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Add hyp_printk event to nVHE/pKVM hyp

This commit adds a trace_hyp_printk() mechanism to allow logging from
the nVHE/pKVM hypervisor into the host's tracing buffer. The format
strings are stored in a kernel-accessible ELF section, while the
hypervisor writes the string ID and arguments.

> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index e1ac876200a3d..b6d62642b6bdb 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 = .;

[Severity: High]
Does this lack of alignment before __hyp_string_fmts_start cause pointer
arithmetic issues?

The start symbol is defined without an explicit alignment directive like
. = ALIGN(8);. Because struct hyp_string_fmt is a 128-byte array, the
compiler may align it to a larger boundary. The __hyp_string_fmts_start
symbol captures the location counter before the section's alignment
requirements are processed by the linker.

Later in __trace_hyp_printk() within arch/arm64/kvm/hyp/include/nvhe/trace.h,
the pointer arithmetic:
  entry->fmt_id = fmt - __hyp_string_fmts_start;

can yield a corrupted index if the start symbol includes padding bytes. The
host reconstructs the pointer using this corrupted index in
__hyp_trace_printk(), causing it to read padding bytes instead of the actual
trace format strings.

> +		*(_hyp_string_fmts)
> +		__hyp_string_fmts_end = .;
>  	}
>  #endif

[ ... ]
> 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 = {	\
> +		.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)

[Severity: High]
Could this missing format validation lead to host kernel panics or
information leaks?

The trace_hyp_printk macro blindly casts arguments to u64 without verifying
that they match the format string at compile time.

This can cause severe cross-privilege issues:

1. If an EL2 developer uses the %s format specifier, the macro casts the EL2
string pointer to u64 and passes it to the host. The host's trace_seq_printf
would attempt to dereference the EL2 virtual address, resulting in an
immediate EL1 translation fault and host kernel panic.

2. If a developer passes fewer arguments than format specifiers (for example,
trace_hyp_printk("x=%d y=%d", x)), the COUNT_ARGS macro returns 1, and the
host executes the case 1: block in __hyp_trace_printk. The underlying
vsnprintf on the host will parse the second %d and fetch the missing argument
from the host kernel's stack or registers, leaking host kernel memory into
the trace buffer.

Could we add a dummy __attribute__((format(printf))) check to ensure
these mismatches are caught during the build?

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