Re: [PATCH v2] KVM: arm64: vgic: Avoid double-deactivate of IRQs in the nested context.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch skips `vgic_v3_deactivate_phys()` unconditionally when `vgic_state_is_nested(vcpu)` is true, which leaves physical interrupts permanently active if the deactivation was triggered via a sysreg trap rather than an LR hardware sync.

Pre-existing issues:
- [High] KVM natively processes trapped L2 `SYS_ICC_DIR_EL1` accesses using L1's virtual interrupt state, resulting in corruption of L1's VGIC state.
- [High] A malicious nested guest hypervisor (L1) can trigger a host `WARN_ONCE` by configuring mismatched virtual IMO and FMO bits.
--

--- Patch [1]: [PATCH v2] KVM: arm64: vgic: Avoid double-deactivate of IRQs in the nested context. ---
commit 4b2bb201a4176459b35d819c32daab08bb5354bf
Author: D Scott Phillips <scott@os.amperecomputing.com>

KVM: arm64: vgic: Avoid double-deactivate of IRQs in the nested context.

In the nested state, the physical interrupt has already been deactivated
through the HW bit in the LR. The extra deactivation would be harmless but
can hit an errata case on AmpereOne, so avoid it here.

Link: https://lore.kernel.org/linux-arm-kernel/20260710222128.416581-1-scott@os.amperecomputing.com/

> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 9e841e7afd4a7..7aa417440f6a2 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -275,7 +275,13 @@ void vgic_v3_deactivate(struct kvm_vcpu *vcpu, u64 val)
>  		lr = vgic_v3_compute_lr(vcpu, irq) & ~ICH_LR_ACTIVE_BIT;
>  	}
>  
> -	if (lr & ICH_LR_HW)
> +	/*
> +	 * In the nested state, the irq has already been deactivated via the HW
> +	 * bit in the LR. Deactivating again would be harmless except AmpereOne
> +	 * errata AC03_CPU_57, AC04_CPU_29 could cause irq delivery to break if
> +	 * the deactivation hits the highest priority pending irq.
> +	 */
> +	if ((lr & ICH_LR_HW) && !vgic_state_is_nested(vcpu))
>  		vgic_v3_deactivate_phys(FIELD_GET(ICH_LR_PHYS_ID_MASK, lr));

[Severity: High]
Does skipping the physical deactivation here leave the physical interrupt
permanently active if the deactivation was triggered via a sysreg trap
rather than an LR hardware sync?

If an untrusted L2 nested guest executes SYS_ICC_DIR_EL1 for a hardware-backed
virtual interrupt, and this traps to L0 (for instance, due to an L0-level
erratum workaround like vgic_v3_broken_seis() that sets TDIR=1), L0's native
sysreg handler access_gic_dir() will call vgic_v3_deactivate().

Since the access was trapped, the hardware did not execute it, so the physical
interrupt was never deactivated. However, vgic_state_is_nested(vcpu) evaluates
to true, causing vgic_v3_deactivate_phys() to be unconditionally skipped.

Could this prevent future interrupts of equal or lower priority from being
delivered, potentially hanging the associated physical device or host timer?


[Severity: High]
This isn't a bug introduced by this patch, but does KVM natively process
trapped L2 SYS_ICC_DIR_EL1 accesses using L1's virtual interrupt state,
resulting in corruption of L1's VGIC state?

If L0 forces DIR to trap, but L1 did not set ICH_HCR_EL2.TDIR, L2's DIR access
traps to L0. sysreg_emulate_nested() checks L1's TDIR and returns false:

arch/arm64/kvm/emulate-nested.c:sysreg_emulate_nested() {
    ...
    case CGT_ICH_HCR_TC_TDIR:
        if (!__forward_traps(vcpu, ICH_HCR_EL2, tc.mask))
            goto emul;
    ...
}

This causes KVM to fall back to native emulation via access_gic_dir(), which
calls vgic_v3_deactivate() using L2's virtual INTID:

arch/arm64/kvm/sys_regs.c:access_gic_dir() {
    ...
    vgic_v3_deactivate(vcpu, p->regval);
    ...
}

Since KVM mistakenly looks up this INTID in vcpu->arch.vgic_cpu (representing
L1's virtual interrupt state), could a malicious L2 guest arbitrarily execute
SYS_ICC_DIR_EL1 to modify the active state of L1's virtual interrupts and
disrupt the L1 hypervisor?


[Severity: High]
This is a pre-existing issue, but can a malicious nested guest hypervisor (L1)
trigger a host kernel panic by configuring mismatched virtual IMO and FMO
bits?

When vgic_state_is_nested() is called, it evaluates the guest's virtual
HCR_EL2 and triggers a WARN_ONCE if HCR_IMO is set without HCR_FMO (or vice
versa):

arch/arm64/kvm/vgic/vgic-v3-nested.c:vgic_state_is_nested() {
    ...
    if (is_nested_ctxt(vcpu)) {
        xmo = __vcpu_sys_reg(vcpu, HCR_EL2) & (HCR_IMO | HCR_FMO);
        WARN_ONCE(xmo && xmo != (HCR_IMO | HCR_FMO),
                  "Separate virtual IRQ/FIQ settings not supported\n");
    ...
}

A malicious L1 hypervisor can trivially reach this by writing to its virtual
HCR_EL2 sysreg and executing an ERET to L2. When KVM L0 processes any exit
from L2, it calls kvm_vgic_sync_hwstate(), which evaluates
vgic_state_is_nested().

If panic_on_warn is enabled, would this allow an unprivileged guest to crash
the host?

>  
>  	vgic_v3_fold_lr(vcpu, lr);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714231158.496808-1-scott@os.amperecomputing.com?part=1