KVM: arm64: Pin the host vCPU before adjusting its PC under pKVM
Fuad Tabba
reviewerPATCH 1/1 KVM: arm64: Pin the host vCPU before adjusting its PC under pKVM
Under pKVM, a page the host shares with EL2 is mapped at EL2 only while it's pinned, and the host vCPU and its VM are pinned at the vCPU's first KVM_RUN. KVM_SET_VCPU_EVENTS with ext_dabt_pending reaches __kvm_adjust_pc() at EL2 before that, and dereferencing the unmapped host vCPU panics the hypervisor. Any process holding /dev/kvm on a pKVM host can trigger it.
Pin the host vCPU around the adjustment when no hyp vCPU is loaded for it, and its VM too, for the MTE flag the exception entry reads. A loaded hyp vCPU implies both are pinned. A pin fails only for memory the host never shared, and the request is then dropped like any other bad host pointer.
--- arch/arm64/kvm/hyp/nvhe/hyp-main.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
arch/arm64/kvm/hyp/nvhe/hyp-main.c +29 -1
@@ -539,11 +539,39 @@ static void handle___pkvm_host_mkyoung_guest(struct kvm_cpu_context *host_ctxt)cpu_reg (host_ctxt,1 ) = ret; } +/* +* With no hyp vCPU loaded for it, the host vCPU and its VM may be unpinned, +* and so unmapped at EL2: both are pinned from the vCPU's first run, and +* KVM_SET_VCPU_EVENTS can reach here before that. +*/ +static void adjust_pc_unloaded (struct kvm_vcpu *host_vcpu) +{ +struct kvm *host_kvm; + +if (hyp_pin_shared_mem (host_vcpu, host_vcpu +1 )) +return ; + +/* The exception entry reads the VM's MTE flag. */ + host_kvm =kern_hyp_va (host_vcpu->kvm); +if (!hyp_pin_shared_mem (host_kvm, host_kvm +1 )) { +__kvm_adjust_pc (host_vcpu); +hyp_unpin_shared_mem (host_kvm, host_kvm +1 ); + } + +hyp_unpin_shared_mem (host_vcpu, host_vcpu +1 ); +} +static void handle___kvm_adjust_pc (struct kvm_cpu_context *host_ctxt) { DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1); +struct pkvm_hyp_vcpu *hyp_vcpu; +struct kvm_vcpu *host_vcpu; -__kvm_adjust_pc (kern_hyp_va (vcpu)); + host_vcpu =__get_host_hyp_vcpus (vcpu, &hyp_vcpu); +if (host_vcpu) +__kvm_adjust_pc (host_vcpu); +else +adjust_pc_unloaded (kern_hyp_va (vcpu)); }static void handle___kvm_flush_vm_context (struct kvm_cpu_context *host_ctxt)
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
Signature · 1 line
2.39.5
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Double-fetch (TOCTOU) and unvalidated dereference of the host-controlled vcpu->kvm pointer creates an arbitrary EL2 memory leak. --
commit 6841c04b3ad40b222469871cbf4d11537656c507
Author: Fuad Tabba <[email protected]>
KVM: arm64: Pin the host vCPU before adjusting its PC under pKVM
This commit pins the host vCPU and its VM around __kvm_adjust_pc() when no hyp vCPU is loaded, to prevent the hypervisor from panicking by dereferencing an unmapped host vCPU. A loaded hyp vCPU implies both are already pinned.
Quoted 4 lines
arch/arm64/kvm/hyp/nvhe/hyp-main.c +0
-0
[ ... ]
arch/arm64/kvm/hyp/nvhe/hyp-main.c static void handle___pkvm_host_mkyoung_guest(struct kvm_cpu_context *host_ctxt) · line 557
Quoted 11 lines
arch/arm64/kvm/hyp/nvhe/hyp-main.c +11
-0+static void adjust_pc_unloaded (struct kvm_vcpu *host_vcpu) +{ +struct kvm *host_kvm; + +if (hyp_pin_shared_mem (host_vcpu, host_vcpu +1 )) +return ; + +/* The exception entry reads the VM's MTE flag. */ + host_kvm =kern_hyp_va (host_vcpu->kvm); +if (!hyp_pin_shared_mem (host_kvm, host_kvm +1 )) { +__kvm_adjust_pc (host_vcpu);
[Severity: Critical]
This is a pre-existing issue, but does this create a double-fetch (TOCTOU) vulnerability that allows the host kernel to leak arbitrary EL2 memory?
When a vCPU is unloaded, adjust_pc_unloaded() reads host_vcpu->kvm from host-shared memory (the first fetch) and pins it. When a vCPU is loaded, handle___kvm_adjust_pc() directly calls __kvm_adjust_pc(host_vcpu) on the host_vcpu without pinning it here.
In either case, deep inside the __kvm_adjust_pc() call path, enter_exception64() in arch/arm64/kvm/hyp/exception.c re-reads vcpu->kvm directly from the host-shared vcpu structure (the second fetch):
if (kvm_has_mte(kern_hyp_va(vcpu->kvm)))
Because the host kernel can concurrently modify host_vcpu->kvm between the pin (if any) and this second fetch, it could redirect the pointer to an arbitrary EL2 address. kvm_has_mte() would then dereference this unvalidated pointer to test the KVM_ARCH_FLAG_MTE_ENABLED bit (bit 1).
If that bit is set, it updates the PSR_TCO_BIT in the vCPU's CPSR, which gets written back to host-accessible memory in host_vcpu->arch.ctxt.regs.pstate. By shifting the base address and observing the result, the host could leak EL2-private or protected guest memory bit by bit.
Quoted 5 lines
arch/arm64/kvm/hyp/nvhe/hyp-main.c +5
-0+hyp_unpin_shared_mem (host_kvm, host_kvm +1 ); + } + +hyp_unpin_shared_mem (host_vcpu, host_vcpu +1 ); +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Fuad Tabba
reviewerSashiko, this one pierced.
[...]
Quoted 3 lines
[Severity: Critical]
This is a pre-existing issue, but does this create a double-fetch (TOCTOU) vulnerability that allows the host kernel to leak arbitrary EL2 memory?
[...]
Quoted 4 lines
If that bit is set, it updates the
PSR_TCO_BITin the vCPU's CPSR, which gets written back to host-accessible memory inhost_vcpu->arch.ctxt.regs.pstate. By shifting the base address and observing the result, the host could leak EL2-private or protected guest memory bit by bit.
The read in enter_exception64() is unvalidated on both paths. With a hyp vCPU loaded, the handler adjusts the host copy and nothing checks host_vcpu->kvm at all, so no race is needed. On the unloaded path the pin in this patch narrows the window rather than opening it. What leaks is one bit per probe (bit 1 of the word at a host-chosen hyp linear address, through PSR_TCO) on MTE-capable hardware, or a hyp panic on an unmapped address.
I'll send a v2 with a separate patch ahead of this one that gets the VM through a get/put pair at the read site, and drop the VM pin from this patch, since it then has nothing to protect.
Cheers,
/fuad