Re: [PATCH v3 06/18] KVM: arm64: Skip fixed-feature state flush for protected vCPUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Early return in `flush_debug_state` leaves the protected guest's `debug_owner` as `VCPU_DEBUG_FREE`, completely bypassing hardware debug register context switching and exposing host physical debug registers to the guest.
--

commit 3ff31a206fa3951489ae8f183dab7b2241b1018e
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: Skip fixed-feature state flush for protected vCPUs

This commit skips the fixed-feature state flush for protected vCPUs by
returning early in flush_debug_state() and sync_debug_state(), and moving the
mdcr_el2 copy into the non-protected branch in flush_hyp_vcpu().

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index ef65c3cd6f810..229a4877d14f5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -200,6 +200,9 @@ static void flush_debug_state(struct pkvm_hyp_vcpu *hyp_vcpu)
>  {
>  	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
>  
> +	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu))
> +		return;
> +

[Severity: Critical]
Does this early return leave the protected guest's debug_owner as
VCPU_DEBUG_FREE (its zero-initialized value) instead of synchronizing it?
This could bypass hardware debug register context switching, potentially
exposing host physical debug registers to the guest.

>  	hyp_vcpu->vcpu.arch.debug_owner = host_vcpu->arch.debug_owner;
>  
>  	if (kvm_guest_owns_debug_regs(&hyp_vcpu->vcpu)) {

[ ... ]

> @@ -218,6 +221,9 @@ static void sync_debug_state(struct pkvm_hyp_vcpu *hyp_vcpu)
>  {
>  	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
>  
> +	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu))
> +		return;
> +

[Severity: Critical]
Could this early return ignore debug state updates on exit, further
contributing to the debug isolation bypass?

If KVM's world switch sees !kvm_debug_regs_in_use(), it might skip restoring
the host's debug registers.

>  	if (kvm_guest_owns_debug_regs(&hyp_vcpu->vcpu))
>  		host_vcpu->arch.vcpu_debug_state = hyp_vcpu->vcpu.arch.vcpu_debug_state;
>  	else if (kvm_host_owns_debug_regs(&hyp_vcpu->vcpu))

[ ... ]

> @@ -241,6 +247,12 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  	if (!pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
>  		if (vcpu_get_flag(host_vcpu, PKVM_HOST_STATE_DIRTY))
>  			flush_hyp_vcpu_state(hyp_vcpu);
> +
> +		hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWI | HCR_TWE);
> +		hyp_vcpu->vcpu.arch.hcr_el2 |= READ_ONCE(host_vcpu->arch.hcr_el2) &
> +							 (HCR_TWI | HCR_TWE);
> +
> +		hyp_vcpu->vcpu.arch.mdcr_el2 = host_vcpu->arch.mdcr_el2;

[Severity: Critical]
By moving the mdcr_el2 copy into the non-protected branch, does the protected
guest retain the EL2-computed mdcr_el2 where TDA is deliberately cleared?

This seems to prevent debug accesses from trapping. If a protected guest
directly executes MSR DBGBVR0_EL1, <addr> without trapping to EL2, and KVM
skips restoring the host's debug registers on exit due to the early returns
above, the guest's malicious values could remain active when the host resumes.

If the host has breakpoints enabled (MDSCR_EL1.MDE = 1), these guest-injected
breakpoints could fire in the host kernel.

>  		hyp_vcpu->vcpu.arch.iflags = host_vcpu->arch.iflags;
>  	} else {
>  		hyp_vcpu->vcpu.arch.ctxt = host_vcpu->arch.ctxt;
> @@ -249,17 +261,13 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  	/* __hyp_running_vcpu must be NULL in a guest context. */
>  	hyp_vcpu->vcpu.arch.ctxt.__hyp_running_vcpu = NULL;
>  
> -	hyp_vcpu->vcpu.arch.mdcr_el2	= host_vcpu->arch.mdcr_el2;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914113338.159227-1-fuad.tabba@linux.dev?part=6