From: Sean Christopherson seanjc@google.com
[ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
Ignore the userspace provided x2APIC ID when fixing up APIC state for KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID register"), which added the fixup, didn't intend to allow userspace to modify the x2APIC ID. In fact, that commit is when KVM first started treating the x2APIC ID as readonly, apparently to fix some race:
static inline u32 kvm_apic_id(struct kvm_lapic *apic) { - return (kvm_lapic_get_reg(apic, APIC_ID) >> 24) & 0xff; + /* To avoid a race between apic_base and following APIC_ID update when + * switching to x2apic_mode, the x2apic mode returns initial x2apic id. + */ + if (apic_x2apic_mode(apic)) + return apic->vcpu->vcpu_id; + + return kvm_lapic_get_reg(apic, APIC_ID) >> 24; }
Furthermore, KVM doesn't support delivering interrupts to vCPUs with a modified x2APIC ID, but KVM *does* return the modified value on a guest RDMSR and for KVM_GET_LAPIC. I.e. no remotely sane setup can actually work with a modified x2APIC ID.
Making the x2APIC ID fully readonly fixes a WARN in KVM's optimized map calculation, which expects the LDR to align with the x2APIC ID.
WARNING: CPU: 2 PID: 958 at arch/x86/kvm/lapic.c:331 kvm_recalculate_apic_map+0x609/0xa00 [kvm] CPU: 2 PID: 958 Comm: recalc_apic_map Not tainted 6.4.0-rc3-vanilla+ #35 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-1-1 04/01/2014 RIP: 0010:kvm_recalculate_apic_map+0x609/0xa00 [kvm] Call Trace: <TASK> kvm_apic_set_state+0x1cf/0x5b0 [kvm] kvm_arch_vcpu_ioctl+0x1806/0x2100 [kvm] kvm_vcpu_ioctl+0x663/0x8a0 [kvm] __x64_sys_ioctl+0xb8/0xf0 do_syscall_64+0x56/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7fade8b9dd6f
Unfortunately, the WARN can still trigger for other CPUs than the current one by racing against KVM_SET_LAPIC, so remove it completely.
Reported-by: Michal Luczaj mhal@rbox.co Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@rbox.co Reported-by: Haoyu Wu haoyuwu254@gmail.com Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@gmail.com Reported-by: syzbot+545f1326f405db4e1c3e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@google.com Signed-off-by: Sean Christopherson seanjc@google.com Message-ID: 20240802202941.344889-2-seanjc@google.com Signed-off-by: Paolo Bonzini pbonzini@redhat.com Signed-off-by: Gavin Guo gavinguo@igalia.com --- arch/x86/kvm/lapic.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 34766abbabd8..cd9c1e1f6fd3 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -338,10 +338,8 @@ static void kvm_recalculate_logical_map(struct kvm_apic_map *new, * reversing the LDR calculation to get cluster of APICs, i.e. no * additional work is required. */ - if (apic_x2apic_mode(apic)) { - WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic))); + if (apic_x2apic_mode(apic)) return; - }
if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask))) { @@ -2964,18 +2962,28 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s, bool set) { if (apic_x2apic_mode(vcpu->arch.apic)) { + u32 x2apic_id = kvm_x2apic_id(vcpu->arch.apic); u32 *id = (u32 *)(s->regs + APIC_ID); u32 *ldr = (u32 *)(s->regs + APIC_LDR); u64 icr;
if (vcpu->kvm->arch.x2apic_format) { - if (*id != vcpu->vcpu_id) + if (*id != x2apic_id) return -EINVAL; } else { + /* + * Ignore the userspace value when setting APIC state. + * KVM's model is that the x2APIC ID is readonly, e.g. + * KVM only supports delivering interrupts to KVM's + * version of the x2APIC ID. However, for backwards + * compatibility, don't reject attempts to set a + * mismatched ID for userspace that hasn't opted into + * x2apic_format. + */ if (set) - *id >>= 24; + *id = x2apic_id; else - *id <<= 24; + *id = x2apic_id << 24; }
/* @@ -2984,7 +2992,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, * split to ICR+ICR2 in userspace for backwards compatibility. */ if (set) { - *ldr = kvm_apic_calc_x2apic_ldr(*id); + *ldr = kvm_apic_calc_x2apic_ldr(x2apic_id);
icr = __kvm_lapic_get_reg(s->regs, APIC_ICR) | (u64)__kvm_lapic_get_reg(s->regs, APIC_ICR2) << 32;
[ Sasha's backport helper bot ]
Hi,
The upstream commit SHA1 provided is correct: 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071
WARNING: Author mismatch between patch and upstream commit: Backport author: Gavin Guo gavinguo@igalia.com Commit author: Sean Christopherson seanjc@google.com
Status in newer kernel trees: 6.12.y | Present (exact SHA1) 6.6.y | Not found
Note: The patch differs from the upstream commit: --- 1: 4b7c3f6d04bd ! 1: f0c195ebb384 KVM: x86: Make x2APIC ID 100% readonly @@ Metadata ## Commit message ## KVM: x86: Make x2APIC ID 100% readonly
+ [ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ] + Ignore the userspace provided x2APIC ID when fixing up APIC state for KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID @@ Commit message Signed-off-by: Sean Christopherson seanjc@google.com Message-ID: 20240802202941.344889-2-seanjc@google.com Signed-off-by: Paolo Bonzini pbonzini@redhat.com + Signed-off-by: Gavin Guo gavinguo@igalia.com
## arch/x86/kvm/lapic.c ## @@ arch/x86/kvm/lapic.c: static void kvm_recalculate_logical_map(struct kvm_apic_map *new, ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.6.y | Success | Success |
On Thu, Dec 26, 2024 at 11:38:47AM +0800, Gavin Guo wrote:
From: Sean Christopherson seanjc@google.com
[ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
Ignore the userspace provided x2APIC ID when fixing up APIC state for KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID register"), which added the fixup, didn't intend to allow userspace to modify the x2APIC ID. In fact, that commit is when KVM first started treating the x2APIC ID as readonly, apparently to fix some race:
static inline u32 kvm_apic_id(struct kvm_lapic *apic) {
return (kvm_lapic_get_reg(apic, APIC_ID) >> 24) & 0xff;
/* To avoid a race between apic_base and following APIC_ID update when* switching to x2apic_mode, the x2apic mode returns initial x2apic id.*/if (apic_x2apic_mode(apic))return apic->vcpu->vcpu_id;return kvm_lapic_get_reg(apic, APIC_ID) >> 24;}
Furthermore, KVM doesn't support delivering interrupts to vCPUs with a modified x2APIC ID, but KVM *does* return the modified value on a guest RDMSR and for KVM_GET_LAPIC. I.e. no remotely sane setup can actually work with a modified x2APIC ID.
Making the x2APIC ID fully readonly fixes a WARN in KVM's optimized map calculation, which expects the LDR to align with the x2APIC ID.
WARNING: CPU: 2 PID: 958 at arch/x86/kvm/lapic.c:331 kvm_recalculate_apic_map+0x609/0xa00 [kvm] CPU: 2 PID: 958 Comm: recalc_apic_map Not tainted 6.4.0-rc3-vanilla+ #35 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-1-1 04/01/2014 RIP: 0010:kvm_recalculate_apic_map+0x609/0xa00 [kvm] Call Trace:
<TASK> kvm_apic_set_state+0x1cf/0x5b0 [kvm] kvm_arch_vcpu_ioctl+0x1806/0x2100 [kvm] kvm_vcpu_ioctl+0x663/0x8a0 [kvm] __x64_sys_ioctl+0xb8/0xf0 do_syscall_64+0x56/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7fade8b9dd6f
Unfortunately, the WARN can still trigger for other CPUs than the current one by racing against KVM_SET_LAPIC, so remove it completely.
Reported-by: Michal Luczaj mhal@rbox.co Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@rbox.co Reported-by: Haoyu Wu haoyuwu254@gmail.com Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@gmail.com Reported-by: syzbot+545f1326f405db4e1c3e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@google.com Signed-off-by: Sean Christopherson seanjc@google.com Message-ID: 20240802202941.344889-2-seanjc@google.com Signed-off-by: Paolo Bonzini pbonzini@redhat.com Signed-off-by: Gavin Guo gavinguo@igalia.com
As this one isn't tagged for stable, the KVM maintainers should ack the backport before we take it.
On Sat, Dec 28, 2024, Sasha Levin wrote:
On Thu, Dec 26, 2024 at 11:38:47AM +0800, Gavin Guo wrote:
From: Sean Christopherson seanjc@google.com
[ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
Ignore the userspace provided x2APIC ID when fixing up APIC state for KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID register"), which added the fixup, didn't intend to allow userspace to modify the x2APIC ID. In fact, that commit is when KVM first started treating the x2APIC ID as readonly, apparently to fix some race:
static inline u32 kvm_apic_id(struct kvm_lapic *apic) {
return (kvm_lapic_get_reg(apic, APIC_ID) >> 24) & 0xff;
/* To avoid a race between apic_base and following APIC_ID update when* switching to x2apic_mode, the x2apic mode returns initial x2apic id.*/if (apic_x2apic_mode(apic))return apic->vcpu->vcpu_id;return kvm_lapic_get_reg(apic, APIC_ID) >> 24;}
Furthermore, KVM doesn't support delivering interrupts to vCPUs with a modified x2APIC ID, but KVM *does* return the modified value on a guest RDMSR and for KVM_GET_LAPIC. I.e. no remotely sane setup can actually work with a modified x2APIC ID.
Making the x2APIC ID fully readonly fixes a WARN in KVM's optimized map calculation, which expects the LDR to align with the x2APIC ID.
WARNING: CPU: 2 PID: 958 at arch/x86/kvm/lapic.c:331 kvm_recalculate_apic_map+0x609/0xa00 [kvm] CPU: 2 PID: 958 Comm: recalc_apic_map Not tainted 6.4.0-rc3-vanilla+ #35 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-1-1 04/01/2014 RIP: 0010:kvm_recalculate_apic_map+0x609/0xa00 [kvm] Call Trace:
<TASK> kvm_apic_set_state+0x1cf/0x5b0 [kvm] kvm_arch_vcpu_ioctl+0x1806/0x2100 [kvm] kvm_vcpu_ioctl+0x663/0x8a0 [kvm] __x64_sys_ioctl+0xb8/0xf0 do_syscall_64+0x56/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7fade8b9dd6f
Unfortunately, the WARN can still trigger for other CPUs than the current one by racing against KVM_SET_LAPIC, so remove it completely.
Reported-by: Michal Luczaj mhal@rbox.co Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@rbox.co Reported-by: Haoyu Wu haoyuwu254@gmail.com Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@gmail.com Reported-by: syzbot+545f1326f405db4e1c3e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@google.com Signed-off-by: Sean Christopherson seanjc@google.com Message-ID: 20240802202941.344889-2-seanjc@google.com Signed-off-by: Paolo Bonzini pbonzini@redhat.com Signed-off-by: Gavin Guo gavinguo@igalia.com
As this one isn't tagged for stable, the KVM maintainers should ack the backport before we take it.
What's the motivation for applying this to 6.6? AFAIK, there's no real world use case that benefits from the patch, the fix is purely to plug a hole where fuzzers, e.g. syzkaller, can trip a WARN.
That said, this is essentially a prerequisite for "KVM: x86: Re-split x2APIC ICR into ICR+ICR2 for AMD (x2AVIC)"[*], and it's relatively low risk, so I'm not opposed to landing it in 6.6.
[*] https://lore.kernel.org/all/2024100123-unreached-enrage-2cb1@gregkh
On 1/7/25 04:27, Sean Christopherson wrote:
On Sat, Dec 28, 2024, Sasha Levin wrote:
On Thu, Dec 26, 2024 at 11:38:47AM +0800, Gavin Guo wrote:
From: Sean Christopherson seanjc@google.com
[ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
Ignore the userspace provided x2APIC ID when fixing up APIC state for KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID register"), which added the fixup, didn't intend to allow userspace to modify the x2APIC ID. In fact, that commit is when KVM first started treating the x2APIC ID as readonly, apparently to fix some race:
static inline u32 kvm_apic_id(struct kvm_lapic *apic) {
return (kvm_lapic_get_reg(apic, APIC_ID) >> 24) & 0xff;
/* To avoid a race between apic_base and following APIC_ID update when* switching to x2apic_mode, the x2apic mode returns initial x2apic id.*/if (apic_x2apic_mode(apic))return apic->vcpu->vcpu_id;return kvm_lapic_get_reg(apic, APIC_ID) >> 24;}
Furthermore, KVM doesn't support delivering interrupts to vCPUs with a modified x2APIC ID, but KVM *does* return the modified value on a guest RDMSR and for KVM_GET_LAPIC. I.e. no remotely sane setup can actually work with a modified x2APIC ID.
Making the x2APIC ID fully readonly fixes a WARN in KVM's optimized map calculation, which expects the LDR to align with the x2APIC ID.
WARNING: CPU: 2 PID: 958 at arch/x86/kvm/lapic.c:331 kvm_recalculate_apic_map+0x609/0xa00 [kvm] CPU: 2 PID: 958 Comm: recalc_apic_map Not tainted 6.4.0-rc3-vanilla+ #35 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-1-1 04/01/2014 RIP: 0010:kvm_recalculate_apic_map+0x609/0xa00 [kvm] Call Trace:
<TASK> kvm_apic_set_state+0x1cf/0x5b0 [kvm] kvm_arch_vcpu_ioctl+0x1806/0x2100 [kvm] kvm_vcpu_ioctl+0x663/0x8a0 [kvm] __x64_sys_ioctl+0xb8/0xf0 do_syscall_64+0x56/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7fade8b9dd6f
Unfortunately, the WARN can still trigger for other CPUs than the current one by racing against KVM_SET_LAPIC, so remove it completely.
Reported-by: Michal Luczaj mhal@rbox.co Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@rbox.co Reported-by: Haoyu Wu haoyuwu254@gmail.com Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@gmail.com Reported-by: syzbot+545f1326f405db4e1c3e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@google.com Signed-off-by: Sean Christopherson seanjc@google.com Message-ID: 20240802202941.344889-2-seanjc@google.com Signed-off-by: Paolo Bonzini pbonzini@redhat.com Signed-off-by: Gavin Guo gavinguo@igalia.com
As this one isn't tagged for stable, the KVM maintainers should ack the backport before we take it.
What's the motivation for applying this to 6.6? AFAIK, there's no real world use case that benefits from the patch, the fix is purely to plug a hole where fuzzers, e.g. syzkaller, can trip a WARN.
That said, this is essentially a prerequisite for "KVM: x86: Re-split x2APIC ICR into ICR+ICR2 for AMD (x2AVIC)"[*], and it's relatively low risk, so I'm not opposed to landing it in 6.6.
[*] https://lore.kernel.org/all/2024100123-unreached-enrage-2cb1@gregkh
Thank you for reviewing the backporting. This backporting aims to address the syzkaller warning message and ensure that the stable kernel is consistent with the upstream version.
On Thu, Jan 09, 2025, Gavin Guo wrote:
On 1/7/25 04:27, Sean Christopherson wrote:
On Sat, Dec 28, 2024, Sasha Levin wrote:
On Thu, Dec 26, 2024 at 11:38:47AM +0800, Gavin Guo wrote:
From: Sean Christopherson seanjc@google.com
[ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
Ignore the userspace provided x2APIC ID when fixing up APIC state for KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID register"), which added the fixup, didn't intend to allow userspace to modify the x2APIC ID. In fact, that commit is when KVM first started treating the x2APIC ID as readonly, apparently to fix some race:
...
Reported-by: Michal Luczaj mhal@rbox.co Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@rbox.co Reported-by: Haoyu Wu haoyuwu254@gmail.com Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@gmail.com Reported-by: syzbot+545f1326f405db4e1c3e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@google.com Signed-off-by: Sean Christopherson seanjc@google.com Message-ID: 20240802202941.344889-2-seanjc@google.com Signed-off-by: Paolo Bonzini pbonzini@redhat.com Signed-off-by: Gavin Guo gavinguo@igalia.com
As this one isn't tagged for stable, the KVM maintainers should ack the backport before we take it.
What's the motivation for applying this to 6.6? AFAIK, there's no real world use case that benefits from the patch, the fix is purely to plug a hole where fuzzers, e.g. syzkaller, can trip a WARN.
That said, this is essentially a prerequisite for "KVM: x86: Re-split x2APIC ICR into ICR+ICR2 for AMD (x2AVIC)"[*], and it's relatively low risk, so I'm not opposed to landing it in 6.6.
[*] https://lore.kernel.org/all/2024100123-unreached-enrage-2cb1@gregkh
Thank you for reviewing the backporting. This backporting aims to address the syzkaller warning message and ensure that the stable kernel is consistent with the upstream version.
IMO, that's not sufficient justification for backporting to stable kernels. This does not fix a problem that any real VMM will ever encounter, there's no security implications, and outside of kernels that are running with panic_on_warn=1, which seems insane outside of explicit test environments, there is zero risk to the host.
This is a bit of a bad example because I do expect this commit to land in 6.6 at some point because it's a preqreq. But I don't want to set the precedent that every commit that addresses a fuzzer-induced WARN is stable material.
Sanity check WARNs in KVM are tricky because userspace can shove arbitrary guest state into KVM, i.e. avoiding what are effectively false positives requires quite a bit of thought, and practically speaking such false positives will happen from time to time. While I definitely want to keep KVM warning-free, I also don't want to discourage sanity checks in KVM, i.e. I view intermittent false positives in KVM as a cost of doing business.
If we start treating what are effectively false positives as stable material, then it changes the math, i.e. each false positive becomes more costly because there are backporting implications, and we'll naturally be more conservative in adding sanity checks, which I don't want to happen.
linux-stable-mirror@lists.linaro.org
-
Gavin Guo -
Sasha Levin -
Sean Christopherson