| Message ID | 20230602233250.1014316-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Out-of-bounds access in kvm_recalculate_phys_map() | expand |
On Fri, Jun 02, 2023, Sean Christopherson wrote: > Bail from kvm_recalculate_phys_map() and disable the optimized map if the > target vCPU's x2APIC ID is out-of-bounds, i.e. if the vCPU was added > and/or enabled its local APIC after the map was allocated. This fixes an > out-of-bounds access bug in the !x2apic_format path where KVM would write > beyond the end of phys_map. > > Check the x2APIC ID regardless of whether or not x2APIC is enabled, > as KVM's hardcodes x2APIC ID to be the vCPU ID, i.e. it can't change, and > the map allocation in kvm_recalculate_apic_map() doesn't check for x2APIC > being enabled, i.e. the check won't get false postivies. > > Note, this also affects the x2apic_format path, which previously just > ignored the "x2apic_id > new->max_apic_id" case. That too is arguably a > bug fix, as ignoring the vCPU meant that KVM would not send interrupts to > the vCPU until the next map recalculation. In practice, that "bug" is > likely benign as a newly present vCPU/APIC would immediately trigger a > recalc. But, there's no functional downside to disabling the map, and > a future patch will gracefully handle the -E2BIG case by retrying instead > of simply disabling the optimized map. > > Opportunistically add a sanity check on the xAPIC ID size, along with a > comment explaining why the xAPIC ID is guaranteed to be "good". > > Reported-by: Michal Luczaj <mhal@rbox.co> Fixes: 5b84b0291702 ("KVM: x86: Honor architectural behavior for aliased 8-bit APIC IDs") Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <seanjc@google.com> > ---
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index e542cf285b51..3c300a196bdf 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -228,6 +228,23 @@ static int kvm_recalculate_phys_map(struct kvm_apic_map *new, u32 xapic_id = kvm_xapic_id(apic); u32 physical_id; + /* + * For simplicity, KVM always allocates enough space for all possible + * xAPIC IDs. Yell, but don't kill the VM, as KVM can continue on + * without the optimized map. + */ + if (WARN_ON_ONCE(xapic_id > new->max_apic_id)) + return -EINVAL; + + /* + * Bail if a vCPU was added and/or enabled its APIC between allocating + * the map and doing the actual calculations for the map. Note, KVM + * hardcodes the x2APIC ID to vcpu_id, i.e. there's no TOCTOU bug if + * the compiler decides to reload x2apic_id after this check. + */ + if (x2apic_id > new->max_apic_id) + return -E2BIG; + /* * Deliberately truncate the vCPU ID when detecting a mismatched APIC * ID to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a @@ -253,8 +270,7 @@ static int kvm_recalculate_phys_map(struct kvm_apic_map *new, */ if (vcpu->kvm->arch.x2apic_format) { /* See also kvm_apic_match_physical_addr(). */ - if ((apic_x2apic_mode(apic) || x2apic_id > 0xff) && - x2apic_id <= new->max_apic_id) + if (apic_x2apic_mode(apic) || x2apic_id > 0xff) new->phys_map[x2apic_id] = apic; if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
Bail from kvm_recalculate_phys_map() and disable the optimized map if the target vCPU's x2APIC ID is out-of-bounds, i.e. if the vCPU was added and/or enabled its local APIC after the map was allocated. This fixes an out-of-bounds access bug in the !x2apic_format path where KVM would write beyond the end of phys_map. Check the x2APIC ID regardless of whether or not x2APIC is enabled, as KVM's hardcodes x2APIC ID to be the vCPU ID, i.e. it can't change, and the map allocation in kvm_recalculate_apic_map() doesn't check for x2APIC being enabled, i.e. the check won't get false postivies. Note, this also affects the x2apic_format path, which previously just ignored the "x2apic_id > new->max_apic_id" case. That too is arguably a bug fix, as ignoring the vCPU meant that KVM would not send interrupts to the vCPU until the next map recalculation. In practice, that "bug" is likely benign as a newly present vCPU/APIC would immediately trigger a recalc. But, there's no functional downside to disabling the map, and a future patch will gracefully handle the -E2BIG case by retrying instead of simply disabling the optimized map. Opportunistically add a sanity check on the xAPIC ID size, along with a comment explaining why the xAPIC ID is guaranteed to be "good". Reported-by: Michal Luczaj <mhal@rbox.co> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/lapic.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)