----- ahonig@xxxxxxxxxx wrote: > The attack scenario that I had in mind is a variant 1 side channel > attack (CVE-2017-5753, > https://urldefense.proofpoint.com/v2/url?u=https-3A__googleprojectzero.blogspot.com_2018_01_reading-2Dprivileged-2Dmemory-2Dwith-2Dside.html&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=OQMUnJ3O0bO12V1XxtDVw8RWakWdnzaaBCPM0L_vLBo&s=16u_dpgrPExpXVeTQzGe0Gzop4ZOKcCHc-vlkiPM_ZA&e=). > This particularly scenario would involve an L1 hypervisor using > vmread/vmwrite to try execute a variant 1 side channel leak on the > host. > > In general variant 1 relies on a bounds check that gets bypassed > speculatively. However it requires a fairly specific code pattern to > actually be useful for an exploit, which is why most bounds check do > not require speculation barrier. It requires two memory references > close to each other. One that is out of bounds and attacker > controlled and one where the memory address is based on the memory > read in the first access. The first memory reference is a read of > the > memory that the attacker wants to leak and the second references > creates side channel in the cache where the line accessed represents > the data to be leaked. > > This code has that pattern because a potentially very large value for > field could be used in the vmcs_to_offset_table lookup which will be > put into f. Then very shortly thereafter and potentially still in > the > speculation window f will be dereferenced in the vmcs_to_field_offset > function. Agreed. Thanks for the explanation. I do however think that some short summary of this should have been put in the commit message. Maybe also in documentation in code. -Liran > > > > On Fri, Jan 12, 2018 at 10:50 AM, Liran Alon <liran.alon@xxxxxxxxxx> > wrote: > > > > ----- jmattson@xxxxxxxxxx wrote: > > > >> From: Andrew Honig <ahonig@xxxxxxxxxx> > >> > >> This adds a memory barrier when performing a lookup into > >> the vmcs_field_to_offset_table. This is related to > >> CVE-2017-5753. > >> > >> Signed-off-by: Andrew Honig <ahonig@xxxxxxxxxx> > >> Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx> > >> --- > >> arch/x86/kvm/vmx.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > >> index cef1882e5d57..f304ebb20229 100644 > >> --- a/arch/x86/kvm/vmx.c > >> +++ b/arch/x86/kvm/vmx.c > >> @@ -1084,6 +1084,7 @@ static inline struct vmcs_field_info > >> *to_vmcs_field_info(unsigned long field) > >> if (field >= ARRAY_SIZE(vmcs_field_to_offset_table)) > >> return NULL; > >> > >> + asm("lfence"); // Change to gmb() when it's available. > >> f = &vmcs_field_to_offset_table[field]; > >> if (f->offset == 0) > >> return NULL; > >> -- > >> 2.16.0.rc1.238.g530d649a79-goog > > > > Can you elaborate on why this specific place was chosen to have a > "speculation barrier"? > > What is the attack scenario that you had in mind? > > > > Is it because when L1 executes VMREAD/VMWRITE with a > general-purpose-register, > > it will be read by handle_vmread()/handle_vmwrite() from VMCS and > used as an index to vmcs_field_to_offset_table[] which could be used > speculatively out-of-bound? > > > > Regards, > > -Liran
