| Message ID | 151727419015.33451.13762926723789552997.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 29, 2018 at 5:03 PM, Dan Williams <dan.j.williams@intel.com> wrote: > Commit 75f139aaf896 "KVM: x86: Add memory barrier on vmcs field lookup" > added a raw 'asm("lfence");' to prevent a bounds check bypass of > 'vmcs_field_to_offset_table'. We can save an lfence in this path and > just use the common array_index_nospec() helper designed for these types > of fixes. > > Cc: Andrew Honig <ahonig@google.com> > Cc: Jim Mattson <jmattson@google.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Hi Thomas, I notice this one missing from -tip, I suspect because of the collision with the raw 'lfence' that is in current mainline? No worries, I'll send a rebased fixup to Paolo directly once array_idx_nospec() goes upstream. > --- > arch/x86/kvm/vmx.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 924589c53422..8514186241d2 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -34,6 +34,7 @@ > #include <linux/tboot.h> > #include <linux/hrtimer.h> > #include <linux/frame.h> > +#include <linux/nospec.h> > #include "kvm_cache_regs.h" > #include "x86.h" > > @@ -883,13 +884,18 @@ static const unsigned short vmcs_field_to_offset_table[] = { > > static inline short vmcs_field_to_offset(unsigned long field) > { > - BUILD_BUG_ON(ARRAY_SIZE(vmcs_field_to_offset_table) > SHRT_MAX); > + const size_t size = ARRAY_SIZE(vmcs_field_to_offset_table); > + unsigned short offset; > > - if (field >= ARRAY_SIZE(vmcs_field_to_offset_table) || > - vmcs_field_to_offset_table[field] == 0) > + BUILD_BUG_ON(size > SHRT_MAX); > + if (field >= size) > return -ENOENT; > > - return vmcs_field_to_offset_table[field]; > + field = array_index_nospec(field, size); > + offset = vmcs_field_to_offset_table[field]; > + if (offset == 0) > + return -ENOENT; > + return offset; > } > > static inline struct vmcs12 *get_vmcs12(struct kvm_vcpu *vcpu) >
On Tue, 30 Jan 2018, Dan Williams wrote: > On Mon, Jan 29, 2018 at 5:03 PM, Dan Williams <dan.j.williams@intel.com> wrote: > > Commit 75f139aaf896 "KVM: x86: Add memory barrier on vmcs field lookup" > > added a raw 'asm("lfence");' to prevent a bounds check bypass of > > 'vmcs_field_to_offset_table'. We can save an lfence in this path and > > just use the common array_index_nospec() helper designed for these types > > of fixes. > > > > Cc: Andrew Honig <ahonig@google.com> > > Cc: Jim Mattson <jmattson@google.com> > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > Hi Thomas, > > I notice this one missing from -tip, I suspect because of the > collision with the raw 'lfence' that is in current mainline? No > worries, I'll send a rebased fixup to Paolo directly once > array_idx_nospec() goes upstream. It did not apply for that reason and I'm still trying to keep x86/pti as clean as it goes to keep GregKHs backporting hell somehow under control. Sorry, I wanted to reply on that, but then my brain shut down after pushing it out. Thanks, tglx
On 31/01/2018 03:07, Thomas Gleixner wrote: >> Hi Thomas, >> >> I notice this one missing from -tip, I suspect because of the >> collision with the raw 'lfence' that is in current mainline? No >> worries, I'll send a rebased fixup to Paolo directly once >> array_idx_nospec() goes upstream. Fine by me. Also, now that x86/pti has been rebased, a version that replaces the lfence with array_idx_nospec() would apply to both tip and 4.14. Paolo > It did not apply for that reason and I'm still trying to keep x86/pti as > clean as it goes to keep GregKHs backporting hell somehow under control.
On Wed, 31 Jan 2018, Paolo Bonzini wrote: > On 31/01/2018 03:07, Thomas Gleixner wrote: > >> Hi Thomas, > >> > >> I notice this one missing from -tip, I suspect because of the > >> collision with the raw 'lfence' that is in current mainline? No > >> worries, I'll send a rebased fixup to Paolo directly once > >> array_idx_nospec() goes upstream. > > Fine by me. Also, now that x86/pti has been rebased, a version that > replaces the lfence with array_idx_nospec() would apply to both tip and > 4.14. I'm happy to take it. Thanks, tglx
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 924589c53422..8514186241d2 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -34,6 +34,7 @@ #include <linux/tboot.h> #include <linux/hrtimer.h> #include <linux/frame.h> +#include <linux/nospec.h> #include "kvm_cache_regs.h" #include "x86.h" @@ -883,13 +884,18 @@ static const unsigned short vmcs_field_to_offset_table[] = { static inline short vmcs_field_to_offset(unsigned long field) { - BUILD_BUG_ON(ARRAY_SIZE(vmcs_field_to_offset_table) > SHRT_MAX); + const size_t size = ARRAY_SIZE(vmcs_field_to_offset_table); + unsigned short offset; - if (field >= ARRAY_SIZE(vmcs_field_to_offset_table) || - vmcs_field_to_offset_table[field] == 0) + BUILD_BUG_ON(size > SHRT_MAX); + if (field >= size) return -ENOENT; - return vmcs_field_to_offset_table[field]; + field = array_index_nospec(field, size); + offset = vmcs_field_to_offset_table[field]; + if (offset == 0) + return -ENOENT; + return offset; } static inline struct vmcs12 *get_vmcs12(struct kvm_vcpu *vcpu)
Commit 75f139aaf896 "KVM: x86: Add memory barrier on vmcs field lookup" added a raw 'asm("lfence");' to prevent a bounds check bypass of 'vmcs_field_to_offset_table'. We can save an lfence in this path and just use the common array_index_nospec() helper designed for these types of fixes. Cc: Andrew Honig <ahonig@google.com> Cc: Jim Mattson <jmattson@google.com> Acked-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- arch/x86/kvm/vmx.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)