| Message ID | 20200218232953.5724-11-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Allow userspace to disable the emulator | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Shuffle a few operand structs to the end of struct x86_emulate_ctxt and > update the cache creation to whitelist only the region of the emulation > context that is expected to be copied to/from user memory, e.g. the > instruction operands, registers, and fetch/io/mem caches. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/kvm_emulate.h | 8 +++++--- > arch/x86/kvm/x86.c | 12 ++++++------ > 2 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h > index 2f0a600efdff..82f712d5c692 100644 > --- a/arch/x86/kvm/kvm_emulate.h > +++ b/arch/x86/kvm/kvm_emulate.h > @@ -322,9 +322,6 @@ struct x86_emulate_ctxt { > u8 intercept; > u8 op_bytes; > u8 ad_bytes; > - struct operand src; > - struct operand src2; > - struct operand dst; > int (*execute)(struct x86_emulate_ctxt *ctxt); > int (*check_perm)(struct x86_emulate_ctxt *ctxt); > /* > @@ -349,6 +346,11 @@ struct x86_emulate_ctxt { > u8 seg_override; > u64 d; > unsigned long _eip; > + > + /* Here begins the usercopy section. */ > + struct operand src; > + struct operand src2; > + struct operand dst; Out of pure curiosity, how certain are we that this is going to be enough for userspaces? > struct operand memop; > /* Fields above regs are cleared together. */ > unsigned long _regs[NR_VCPU_REGS]; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 370af9fe0f5b..e1eaca65756b 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -235,13 +235,13 @@ static struct kmem_cache *x86_emulator_cache; > > static struct kmem_cache *kvm_alloc_emulator_cache(void) > { > - return kmem_cache_create_usercopy("x86_emulator", > - sizeof(struct x86_emulate_ctxt), > + unsigned int useroffset = offsetof(struct x86_emulate_ctxt, src); > + unsigned int size = sizeof(struct x86_emulate_ctxt); > + > + return kmem_cache_create_usercopy("x86_emulator", size, > __alignof__(struct x86_emulate_ctxt), > - SLAB_ACCOUNT, > - 0, > - sizeof(struct x86_emulate_ctxt), > - NULL); > + SLAB_ACCOUNT, useroffset, > + size - useroffset, NULL); > } > > static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
On 26/02/20 18:51, Vitaly Kuznetsov wrote: >> + >> + /* Here begins the usercopy section. */ >> + struct operand src; >> + struct operand src2; >> + struct operand dst; > Out of pure curiosity, how certain are we that this is going to be > enough for userspaces? > And also, where exactly are the user copies done? Paolo
On Wed, Feb 26, 2020 at 06:51:02PM +0100, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > Shuffle a few operand structs to the end of struct x86_emulate_ctxt and > > update the cache creation to whitelist only the region of the emulation > > context that is expected to be copied to/from user memory, e.g. the > > instruction operands, registers, and fetch/io/mem caches. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/kvm_emulate.h | 8 +++++--- > > arch/x86/kvm/x86.c | 12 ++++++------ > > 2 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h > > index 2f0a600efdff..82f712d5c692 100644 > > --- a/arch/x86/kvm/kvm_emulate.h > > +++ b/arch/x86/kvm/kvm_emulate.h > > @@ -322,9 +322,6 @@ struct x86_emulate_ctxt { > > u8 intercept; > > u8 op_bytes; > > u8 ad_bytes; > > - struct operand src; > > - struct operand src2; > > - struct operand dst; > > int (*execute)(struct x86_emulate_ctxt *ctxt); > > int (*check_perm)(struct x86_emulate_ctxt *ctxt); > > /* > > @@ -349,6 +346,11 @@ struct x86_emulate_ctxt { > > u8 seg_override; > > u64 d; > > unsigned long _eip; > > + > > + /* Here begins the usercopy section. */ > > + struct operand src; > > + struct operand src2; > > + struct operand dst; > > Out of pure curiosity, how certain are we that this is going to be > enough for userspaces? This is purely related to in-kernel hardening, there's no concern about this being "enough" because it's not directly visible to userspace. usercopy refers to the kernel copying data to/from user memory. When running with CONFIG_HARDENED_USERCOPY=y, copy_{to,from}_user performs extra checks to warn if the kernel is doing something potentially dangerous. For this code specifically, it will warn if user data is being copied into a struct and the affected range within the struct isn't explicitly annotated as being safe for usercopy. The idea is that the kernel shouldn't be copying user data into variables that the kernel expects are fully under its control. Currently, KVM marks the entire struct x86_emulate_ctxt as being "safe" for usercopy, which is sub-optimal because HARDENED_USERCOPY wouldn't be able to detect KVM bugs, e.g. if "enum x86emul_mode mode" were overwritten by copy_from_user(). Shuffling the emulator fields so that the all fields that are used for usercopy are clustered together allows KVM to use a more precise declaration for which fields are "safe", e.g. the theoretical @mode bug would now be caught.
On Mon, Mar 02, 2020 at 07:40:27PM +0100, Paolo Bonzini wrote: > On 26/02/20 18:51, Vitaly Kuznetsov wrote: > >> + > >> + /* Here begins the usercopy section. */ > >> + struct operand src; > >> + struct operand src2; > >> + struct operand dst; > > Out of pure curiosity, how certain are we that this is going to be > > enough for userspaces? > > > > And also, where exactly are the user copies done? Anything that funnels into ctxt->ops->read_std() or ctxt->ops->write_std(), e.g. if (ctxt->src2.type == OP_MEM) { rc = segmented_read(ctxt, ctxt->src2.addr.mem, &ctxt->src2.val, ctxt->src2.bytes); if (rc != X86EMUL_CONTINUE) goto done; } segmented_read() : @data = &ctxt->src2.val | |-> read_emulated() | |-> ctxt->ops->read_emulated() / emulator_read_emulated() | |-> emulator_read_write() | |-> emulator_read_write_onepage() | |-> ops->read_write_emulate() / read_emulate() | |-> kvm_vcpu_read_guest() | ... |-> __kvm_read_guest_page() | |-> __copy_from_user(data, ...)
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h index 2f0a600efdff..82f712d5c692 100644 --- a/arch/x86/kvm/kvm_emulate.h +++ b/arch/x86/kvm/kvm_emulate.h @@ -322,9 +322,6 @@ struct x86_emulate_ctxt { u8 intercept; u8 op_bytes; u8 ad_bytes; - struct operand src; - struct operand src2; - struct operand dst; int (*execute)(struct x86_emulate_ctxt *ctxt); int (*check_perm)(struct x86_emulate_ctxt *ctxt); /* @@ -349,6 +346,11 @@ struct x86_emulate_ctxt { u8 seg_override; u64 d; unsigned long _eip; + + /* Here begins the usercopy section. */ + struct operand src; + struct operand src2; + struct operand dst; struct operand memop; /* Fields above regs are cleared together. */ unsigned long _regs[NR_VCPU_REGS]; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 370af9fe0f5b..e1eaca65756b 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -235,13 +235,13 @@ static struct kmem_cache *x86_emulator_cache; static struct kmem_cache *kvm_alloc_emulator_cache(void) { - return kmem_cache_create_usercopy("x86_emulator", - sizeof(struct x86_emulate_ctxt), + unsigned int useroffset = offsetof(struct x86_emulate_ctxt, src); + unsigned int size = sizeof(struct x86_emulate_ctxt); + + return kmem_cache_create_usercopy("x86_emulator", size, __alignof__(struct x86_emulate_ctxt), - SLAB_ACCOUNT, - 0, - sizeof(struct x86_emulate_ctxt), - NULL); + SLAB_ACCOUNT, useroffset, + size - useroffset, NULL); } static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
Shuffle a few operand structs to the end of struct x86_emulate_ctxt and update the cache creation to whitelist only the region of the emulation context that is expected to be copied to/from user memory, e.g. the instruction operands, registers, and fetch/io/mem caches. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/kvm_emulate.h | 8 +++++--- arch/x86/kvm/x86.c | 12 ++++++------ 2 files changed, 11 insertions(+), 9 deletions(-)