| Message ID | 20160616060531.30028-3-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/06/2016 08:05, Haozhong Zhang wrote: > + /* > + * Only bits masked by msr_ia32_feature_control_valid_bits can be set in > + * msr_ia32_feature_control. > + * > + * msr_ia32_feature_control_valid_bits should be modified by > + * feature_control_valid_bits_add/del(), and only bits masked by > + * FEATURE_CONTROL_MAX_VALID_BITS can be modified. > + */ > u64 msr_ia32_feature_control; > + u64 msr_ia32_feature_control_valid_bits; I noticed that the fw_cfg patch used an uint32_t. It probably should use uint64_t; what you did here is correct. Paolo > }; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/06/2016 08:05, Haozhong Zhang wrote: > +/* > + * FEATURE_CONTROL_LOCKED is added/removed automatically by > + * feature_control_valid_bits_add/del(), so it's not included here. > + */ > +#define FEATURE_CONTROL_MAX_VALID_BITS \ > + FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX > + > +static void feature_control_valid_bits_add(struct kvm_vcpu *vcpu, uint64_t bits) > +{ > + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); The KVM-specific ASSERT macro should go. You can use WARN_ON. However, I think FEATURE_CONTROL_LOCKED should always be writable. If you change that, it's simpler to just do |= and &= in the caller. > + to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |= > + bits | FEATURE_CONTROL_LOCKED; > +} > + > +static void feature_control_valid_bits_del(struct kvm_vcpu *vcpu, uint64_t bits) > +{ > + uint64_t *valid_bits = > + &to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; > + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); > + *valid_bits &= ~bits; > + if (!(*valid_bits & ~FEATURE_CONTROL_LOCKED)) > + *valid_bits = 0; > +} > + > #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x) > #define FIELD(number, name) [number] = VMCS12_OFFSET(name) > #define FIELD64(number, name) [number] = VMCS12_OFFSET(name), \ > @@ -2864,6 +2897,14 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) > return 0; > } > > +static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, > + uint64_t val) > +{ > + uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; > + > + return valid_bits && !(val & ~valid_bits); > +} > /* > * Reads an msr value (of 'msr_index') into 'pdata'. > * Returns 0 on success, non-0 otherwise. > @@ -2906,7 +2947,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > msr_info->data = vmcs_read64(GUEST_BNDCFGS); > break; > case MSR_IA32_FEATURE_CONTROL: > - if (!nested_vmx_allowed(vcpu)) > + if (!vmx_feature_control_msr_valid(vcpu, 0)) You can remove this if completely in patch 1. It's okay to make the MSR always available. Thanks, Paolo > return 1; > msr_info->data = to_vmx(vcpu)->msr_ia32_feature_control; > break; > @@ -2999,7 +3040,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > ret = kvm_set_msr_common(vcpu, msr_info); > break; > case MSR_IA32_FEATURE_CONTROL: > - if (!nested_vmx_allowed(vcpu) || > + if (!vmx_feature_control_msr_valid(vcpu, data) || > (to_vmx(vcpu)->msr_ia32_feature_control & > FEATURE_CONTROL_LOCKED && !msr_info->host_initiated)) > return 1; > @@ -9095,6 +9136,13 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) > vmx->nested.nested_vmx_secondary_ctls_high &= > ~SECONDARY_EXEC_PCOMMIT; > } > + > + if (nested_vmx_allowed(vcpu)) > + feature_control_valid_bits_add > + (vcpu, FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX); > + else > + feature_control_valid_bits_del > + (vcpu, FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX); > } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/16/16 12:01, Paolo Bonzini wrote: > > > On 16/06/2016 08:05, Haozhong Zhang wrote: > > +/* > > + * FEATURE_CONTROL_LOCKED is added/removed automatically by > > + * feature_control_valid_bits_add/del(), so it's not included here. > > + */ > > +#define FEATURE_CONTROL_MAX_VALID_BITS \ > > + FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX > > + > > +static void feature_control_valid_bits_add(struct kvm_vcpu *vcpu, uint64_t bits) > > +{ > > + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); > > The KVM-specific ASSERT macro should go. You can use WARN_ON. > will change to WARN_ON. > However, I think FEATURE_CONTROL_LOCKED should always be writable. If > you change that, it's simpler to just do |= and &= in the caller. > These two functions (add/del) are to prevent callers from forgetting setting/clearing FEATURE_CONTROL_LOCKED in msr_ia32_feature_control_valid_bits: it should be set if any feature bit is set, and be cleared if all feature bits are cleared. The second rule could relaxed as we can always present MSR_IA32_FEATURE_CONTROL to guest. I'm okey to let callers take care for the locked bit. > > + to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |= > > + bits | FEATURE_CONTROL_LOCKED; > > +} > > + > > +static void feature_control_valid_bits_del(struct kvm_vcpu *vcpu, uint64_t bits) > > +{ > > + uint64_t *valid_bits = > > + &to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; > > + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); > > + *valid_bits &= ~bits; > > + if (!(*valid_bits & ~FEATURE_CONTROL_LOCKED)) > > + *valid_bits = 0; > > +} > > + > > #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x) > > #define FIELD(number, name) [number] = VMCS12_OFFSET(name) > > #define FIELD64(number, name) [number] = VMCS12_OFFSET(name), \ > > @@ -2864,6 +2897,14 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) > > return 0; > > } > > > > +static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, > > + uint64_t val) > > +{ > > + uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; > > + > > + return valid_bits && !(val & ~valid_bits); > > +} > > /* > > * Reads an msr value (of 'msr_index') into 'pdata'. > > * Returns 0 on success, non-0 otherwise. > > @@ -2906,7 +2947,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > msr_info->data = vmcs_read64(GUEST_BNDCFGS); > > break; > > case MSR_IA32_FEATURE_CONTROL: > > - if (!nested_vmx_allowed(vcpu)) > > + if (!vmx_feature_control_msr_valid(vcpu, 0)) > > You can remove this if completely in patch 1. It's okay to make the MSR > always available. > But then it also allows all bits to be set by guests, even though some features are not available. Though this problem already exists before Patch 1, I prefer to leave it as was in Patch 1 and fix it here. Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/06/2016 13:16, Haozhong Zhang wrote: >> However, I think FEATURE_CONTROL_LOCKED should always be writable. If >> you change that, it's simpler to just do |= and &= in the caller. > > These two functions (add/del) are to prevent callers from forgetting > setting/clearing FEATURE_CONTROL_LOCKED in > msr_ia32_feature_control_valid_bits: it should be set if any feature > bit is set, and be cleared if all feature bits are cleared. The second > rule could relaxed as we can always present MSR_IA32_FEATURE_CONTROL > to guest. Yes, this means that FEATURE_CONTROL_LOCKED effectively is always valid. So you end up with just &= to clear and |= to set. > I'm okey to let callers take care for the locked bit. > >>> + to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |= >>> + bits | FEATURE_CONTROL_LOCKED; >>> +} >>> + >>> +static void feature_control_valid_bits_del(struct kvm_vcpu *vcpu, uint64_t bits) >>> +{ >>> + uint64_t *valid_bits = >>> + &to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; >>> + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); >>> + *valid_bits &= ~bits; >>> + if (!(*valid_bits & ~FEATURE_CONTROL_LOCKED)) >>> + *valid_bits = 0; >>> +} >>> + >>> #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x) >>> #define FIELD(number, name) [number] = VMCS12_OFFSET(name) >>> #define FIELD64(number, name) [number] = VMCS12_OFFSET(name), \ >>> @@ -2864,6 +2897,14 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) >>> return 0; >>> } >>> >>> +static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, >>> + uint64_t val) >>> +{ >>> + uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; >>> + >>> + return valid_bits && !(val & ~valid_bits); >>> +} >>> /* >>> * Reads an msr value (of 'msr_index') into 'pdata'. >>> * Returns 0 on success, non-0 otherwise. >>> @@ -2906,7 +2947,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>> msr_info->data = vmcs_read64(GUEST_BNDCFGS); >>> break; >>> case MSR_IA32_FEATURE_CONTROL: >>> - if (!nested_vmx_allowed(vcpu)) >>> + if (!vmx_feature_control_msr_valid(vcpu, 0)) >> >> You can remove this if completely in patch 1. It's okay to make the MSR >> always available. >> > > But then it also allows all bits to be set by guests, even though some > features are not available. Note that this is "get". Of course the "if" must stay in vmx_set_msr. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/16/16 11:55, Paolo Bonzini wrote: > > > On 16/06/2016 08:05, Haozhong Zhang wrote: > > + /* > > + * Only bits masked by msr_ia32_feature_control_valid_bits can be set in > > + * msr_ia32_feature_control. > > + * > > + * msr_ia32_feature_control_valid_bits should be modified by > > + * feature_control_valid_bits_add/del(), and only bits masked by > > + * FEATURE_CONTROL_MAX_VALID_BITS can be modified. > > + */ > > u64 msr_ia32_feature_control; > > + u64 msr_ia32_feature_control_valid_bits; > > I noticed that the fw_cfg patch used an uint32_t. It probably should > use uint64_t; what you did here is correct. > I'll fix there. Thanks, Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/16/16 13:19, Paolo Bonzini wrote: > > > On 16/06/2016 13:16, Haozhong Zhang wrote: > >> However, I think FEATURE_CONTROL_LOCKED should always be writable. If > >> you change that, it's simpler to just do |= and &= in the caller. > > > > These two functions (add/del) are to prevent callers from forgetting > > setting/clearing FEATURE_CONTROL_LOCKED in > > msr_ia32_feature_control_valid_bits: it should be set if any feature > > bit is set, and be cleared if all feature bits are cleared. The second > > rule could relaxed as we can always present MSR_IA32_FEATURE_CONTROL > > to guest. > > Yes, this means that FEATURE_CONTROL_LOCKED effectively is always valid. > So you end up with just &= to clear and |= to set. > > > I'm okey to let callers take care for the locked bit. > > > >>> + to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |= > >>> + bits | FEATURE_CONTROL_LOCKED; > >>> +} > >>> + > >>> +static void feature_control_valid_bits_del(struct kvm_vcpu *vcpu, uint64_t bits) > >>> +{ > >>> + uint64_t *valid_bits = > >>> + &to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; > >>> + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); > >>> + *valid_bits &= ~bits; > >>> + if (!(*valid_bits & ~FEATURE_CONTROL_LOCKED)) > >>> + *valid_bits = 0; > >>> +} > >>> + > >>> #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x) > >>> #define FIELD(number, name) [number] = VMCS12_OFFSET(name) > >>> #define FIELD64(number, name) [number] = VMCS12_OFFSET(name), \ > >>> @@ -2864,6 +2897,14 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) > >>> return 0; > >>> } > >>> > >>> +static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, > >>> + uint64_t val) > >>> +{ > >>> + uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; > >>> + > >>> + return valid_bits && !(val & ~valid_bits); > >>> +} > >>> /* > >>> * Reads an msr value (of 'msr_index') into 'pdata'. > >>> * Returns 0 on success, non-0 otherwise. > >>> @@ -2906,7 +2947,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > >>> msr_info->data = vmcs_read64(GUEST_BNDCFGS); > >>> break; > >>> case MSR_IA32_FEATURE_CONTROL: > >>> - if (!nested_vmx_allowed(vcpu)) > >>> + if (!vmx_feature_control_msr_valid(vcpu, 0)) > >> > >> You can remove this if completely in patch 1. It's okay to make the MSR > >> always available. > >> > > > > But then it also allows all bits to be set by guests, even though some > > features are not available. > > Note that this is "get". Of course the "if" must stay in vmx_set_msr. > My mistake. I'll remove it in patch 1. Thanks, Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6b63f2d..1dc89c5 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -602,7 +602,16 @@ struct vcpu_vmx { u32 guest_pkru; u32 host_pkru; + /* + * Only bits masked by msr_ia32_feature_control_valid_bits can be set in + * msr_ia32_feature_control. + * + * msr_ia32_feature_control_valid_bits should be modified by + * feature_control_valid_bits_add/del(), and only bits masked by + * FEATURE_CONTROL_MAX_VALID_BITS can be modified. + */ u64 msr_ia32_feature_control; + u64 msr_ia32_feature_control_valid_bits; }; enum segment_cache_field { @@ -624,6 +633,30 @@ static struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) return &(to_vmx(vcpu)->pi_desc); } +/* + * FEATURE_CONTROL_LOCKED is added/removed automatically by + * feature_control_valid_bits_add/del(), so it's not included here. + */ +#define FEATURE_CONTROL_MAX_VALID_BITS \ + FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX + +static void feature_control_valid_bits_add(struct kvm_vcpu *vcpu, uint64_t bits) +{ + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); + to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |= + bits | FEATURE_CONTROL_LOCKED; +} + +static void feature_control_valid_bits_del(struct kvm_vcpu *vcpu, uint64_t bits) +{ + uint64_t *valid_bits = + &to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; + ASSERT(!(bits & ~FEATURE_CONTROL_MAX_VALID_BITS)); + *valid_bits &= ~bits; + if (!(*valid_bits & ~FEATURE_CONTROL_LOCKED)) + *valid_bits = 0; +} + #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x) #define FIELD(number, name) [number] = VMCS12_OFFSET(name) #define FIELD64(number, name) [number] = VMCS12_OFFSET(name), \ @@ -2864,6 +2897,14 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) return 0; } +static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu, + uint64_t val) +{ + uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits; + + return valid_bits && !(val & ~valid_bits); +} + /* * Reads an msr value (of 'msr_index') into 'pdata'. * Returns 0 on success, non-0 otherwise. @@ -2906,7 +2947,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) msr_info->data = vmcs_read64(GUEST_BNDCFGS); break; case MSR_IA32_FEATURE_CONTROL: - if (!nested_vmx_allowed(vcpu)) + if (!vmx_feature_control_msr_valid(vcpu, 0)) return 1; msr_info->data = to_vmx(vcpu)->msr_ia32_feature_control; break; @@ -2999,7 +3040,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) ret = kvm_set_msr_common(vcpu, msr_info); break; case MSR_IA32_FEATURE_CONTROL: - if (!nested_vmx_allowed(vcpu) || + if (!vmx_feature_control_msr_valid(vcpu, data) || (to_vmx(vcpu)->msr_ia32_feature_control & FEATURE_CONTROL_LOCKED && !msr_info->host_initiated)) return 1; @@ -9095,6 +9136,13 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu) vmx->nested.nested_vmx_secondary_ctls_high &= ~SECONDARY_EXEC_PCOMMIT; } + + if (nested_vmx_allowed(vcpu)) + feature_control_valid_bits_add + (vcpu, FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX); + else + feature_control_valid_bits_del + (vcpu, FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX); } static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
KVM currently does not check the value written to guest MSR_IA32_FEATURE_CONTROL, though bits corresponding to disabled features may be set. This patch makes KVM to validate individual bits written to guest MSR_IA32_FEATURE_CONTROL according to enabled features. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- arch/x86/kvm/vmx.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-)