| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <lnfdbq433rp5d66tlgpllmdzftwzwkt4jdmhpqf354g7lz7wnv@yxofvwgcoycp>
Date: Tue, 9 Dec 2025 18:37:28 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Jim Mattson <jmattson@...gle.com>,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 12/13] KVM: nSVM: Sanitize control fields copied from
VMCB12
On Tue, Dec 09, 2025 at 08:19:02AM -0800, Sean Christopherson wrote:
> On Mon, Nov 10, 2025, Yosry Ahmed wrote:
> > Make sure all fields used from VMCB12 in creating the VMCB02 are
> > sanitized, such no unhandled or reserved bits end up in the VMCB02.
> >
> > The following control fields are read from VMCB12 and have bits that are
> > either reserved or not handled/advertised by KVM: tlb_ctl, int_ctl,
> > int_state, int_vector, event_inj, misc_ctl, and misc_ctl2.
> >
> > The following fields do not require any extra sanitizing:
> > - int_ctl: bits from VMCB12 are copied bit-by-bit as needed.
> > - misc_ctl: only used in consistency checks (particularly NP_ENABLE).
> > - misc_ctl2: bits from VMCB12 are copied bit-by-bit as needed.
> >
> > For the remaining fields, make sure only defined bits are copied from
> > VMCB12 by defining appropriate masks where needed. The only exception is
> > tlb_ctl, which is unused, so remove it.
> >
> > Opportunisitcally move some existing definitions in svm.h around such
>
> Opportunistically. But moot point, because please put such cleanups in a separate
> patch. There are so many opportunistic cleanups in this patch that I genuinely
> can't see what's changing, and I don't have the patience right now to stare hard.
Fair enough.
>
> Cleanups will making *related* changes are totally fine, e.g. bundling the use
> of PAGE_MASK in conjuction with changing the code to do "from->iopm_base_pa & ..."
> instead of "to->msrpm_base_pa &= ..." is fine, but those changes have nothing to
> do with the rest of the patch.
>
> > that they are ordered by bit position, and cleanup ignoring the lower
> > bits of {io/msr}pm_base_pa in __nested_copy_vmcb_control_to_cache() by
> > using PAGE_MASK. Also, expand the comment about the ASID being copied
> > only for consistency checks.
> >
> > Suggested-by: Jim Mattson <jmattson@...gle.com>
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> > ---
> > arch/x86/include/asm/svm.h | 11 ++++++++---
> > arch/x86/kvm/svm/nested.c | 26 ++++++++++++++------------
> > arch/x86/kvm/svm/svm.h | 1 -
> > 3 files changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> > index a842018952d2c..44f2cfcd8d4ff 100644
> > --- a/arch/x86/include/asm/svm.h
> > +++ b/arch/x86/include/asm/svm.h
> > @@ -213,11 +213,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
> > #define V_NMI_ENABLE_SHIFT 26
> > #define V_NMI_ENABLE_MASK (1 << V_NMI_ENABLE_SHIFT)
> >
> > +#define X2APIC_MODE_SHIFT 30
> > +#define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
> > +
> > #define AVIC_ENABLE_SHIFT 31
> > #define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_SHIFT)
> >
> > -#define X2APIC_MODE_SHIFT 30
> > -#define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
> > +#define SVM_INT_VECTOR_MASK (0xff)
> >
> > #define SVM_INTERRUPT_SHADOW_MASK BIT_ULL(0)
> > #define SVM_GUEST_INTERRUPT_MASK BIT_ULL(1)
> > @@ -626,8 +628,11 @@ static inline void __unused_size_checks(void)
> > #define SVM_EVTINJ_TYPE_EXEPT (3 << SVM_EVTINJ_TYPE_SHIFT)
> > #define SVM_EVTINJ_TYPE_SOFT (4 << SVM_EVTINJ_TYPE_SHIFT)
> >
> > -#define SVM_EVTINJ_VALID (1 << 31)
> > #define SVM_EVTINJ_VALID_ERR (1 << 11)
> > +#define SVM_EVTINJ_VALID (1 << 31)
>
> If you want to do cleanup, these should all use BIT()...
I can do that.
>
> > +
> > +#define SVM_EVTINJ_RESERVED_BITS ~(SVM_EVTINJ_VEC_MASK | SVM_EVTINJ_TYPE_MASK | \
> > + SVM_EVTINJ_VALID_ERR | SVM_EVTINJ_VALID)
>
> Because then I don't have to think hard about what exactly this will generate.
Ack.
>
> > #define SVM_EXITINTINFO_VEC_MASK SVM_EVTINJ_VEC_MASK
> > #define SVM_EXITINTINFO_TYPE_MASK SVM_EVTINJ_TYPE_MASK
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 89830380cebc5..503cb7f5a4c5f 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -479,10 +479,11 @@ void __nested_copy_vmcb_control_to_cache(struct kvm_vcpu *vcpu,
> > for (i = 0; i < MAX_INTERCEPT; i++)
> > to->intercepts[i] = from->intercepts[i];
> >
> > - to->iopm_base_pa = from->iopm_base_pa;
> > - to->msrpm_base_pa = from->msrpm_base_pa;
> > + /* Lower bits of IOPM_BASE_PA and MSRPM_BASE_PA are ignored */
> > + to->iopm_base_pa = from->iopm_base_pa & PAGE_MASK;
> > + to->msrpm_base_pa = from->msrpm_base_pa & PAGE_MASK;
> > +
> > to->tsc_offset = from->tsc_offset;
> > - to->tlb_ctl = from->tlb_ctl;
> > to->int_ctl = from->int_ctl;
> > to->int_vector = from->int_vector;
> > to->int_state = from->int_state;
> > @@ -492,19 +493,21 @@ void __nested_copy_vmcb_control_to_cache(struct kvm_vcpu *vcpu,
> > to->exit_info_2 = from->exit_info_2;
> > to->exit_int_info = from->exit_int_info;
> > to->exit_int_info_err = from->exit_int_info_err;
> > - to->misc_ctl = from->misc_ctl;
> > + to->misc_ctl = from->misc_ctl;
> > to->event_inj = from->event_inj;
> > to->event_inj_err = from->event_inj_err;
> > to->next_rip = from->next_rip;
> > to->nested_cr3 = from->nested_cr3;
> > - to->misc_ctl2 = from->misc_ctl2;
> > + to->misc_ctl2 = from->misc_ctl2;
> > to->pause_filter_count = from->pause_filter_count;
> > to->pause_filter_thresh = from->pause_filter_thresh;
> >
> > - /* Copy asid here because nested_vmcb_check_controls will check it. */
> > + /*
> > + * Copy asid here because nested_vmcb_check_controls() will check it.
> > + * The ASID could be invalid, or conflict with another VM's ASID , so it
> > + * should never be used directly to run L2.
> > + */
> > to->asid = from->asid;
> > - to->msrpm_base_pa &= ~0x0fffULL;
> > - to->iopm_base_pa &= ~0x0fffULL;
> >
> > #ifdef CONFIG_KVM_HYPERV
> > /* Hyper-V extensions (Enlightened VMCB) */
> > @@ -890,9 +893,9 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
> > (svm->nested.ctl.int_ctl & int_ctl_vmcb12_bits) |
> > (vmcb01->control.int_ctl & int_ctl_vmcb01_bits);
> >
> > - vmcb02->control.int_vector = svm->nested.ctl.int_vector;
> > - vmcb02->control.int_state = svm->nested.ctl.int_state;
> > - vmcb02->control.event_inj = svm->nested.ctl.event_inj;
> > + vmcb02->control.int_vector = svm->nested.ctl.int_vector & SVM_INT_VECTOR_MASK;
> > + vmcb02->control.int_state = svm->nested.ctl.int_state & SVM_INTERRUPT_SHADOW_MASK;
> > + vmcb02->control.event_inj = svm->nested.ctl.event_inj & ~SVM_EVTINJ_RESERVED_BITS;
> > vmcb02->control.event_inj_err = svm->nested.ctl.event_inj_err;
> >
> > /*
> > @@ -1774,7 +1777,6 @@ static void nested_copy_vmcb_cache_to_control(struct vmcb_control_area *dst,
> > dst->msrpm_base_pa = from->msrpm_base_pa;
> > dst->tsc_offset = from->tsc_offset;
> > dst->asid = from->asid;
> > - dst->tlb_ctl = from->tlb_ctl;
> > dst->int_ctl = from->int_ctl;
> > dst->int_vector = from->int_vector;
> > dst->int_state = from->int_state;
> > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > index ef6bdce630dc0..c8d43793aa9d6 100644
> > --- a/arch/x86/kvm/svm/svm.h
> > +++ b/arch/x86/kvm/svm/svm.h
> > @@ -178,7 +178,6 @@ struct vmcb_ctrl_area_cached {
> > u64 msrpm_base_pa;
> > u64 tsc_offset;
> > u32 asid;
> > - u8 tlb_ctl;
> > u32 int_ctl;
> > u32 int_vector;
> > u32 int_state;
> > --
> > 2.51.2.1041.gc1ab5b90ca-goog
> >
Powered by blists - more mailing lists