From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code. Date: Mon, 25 Jun 2012 18:35:44 +0300 Message-ID: <20120625153544.GC2596@redhat.com> References: <20120624132710.GW6533@redhat.com> <4FE7188A.2080500@redhat.com> <20120624142753.GX6533@redhat.com> <4FE86046.6090702@redhat.com> <20120625131253.GA16583@redhat.com> <4FE86A53.2060108@redhat.com> <20120625141750.GA2596@redhat.com> <4FE8767F.1020108@redhat.com> <20120625145501.GB2596@redhat.com> <4FE87DB7.1000905@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, mtosatti@redhat.com To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44946 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754116Ab2FYPfq (ORCPT ); Mon, 25 Jun 2012 11:35:46 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q5PFZjxT011814 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 25 Jun 2012 11:35:45 -0400 Content-Disposition: inline In-Reply-To: <4FE87DB7.1000905@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jun 25, 2012 at 06:03:19PM +0300, Avi Kivity wrote: > On 06/25/2012 05:55 PM, Gleb Natapov wrote: > > On Mon, Jun 25, 2012 at 05:32:31PM +0300, Avi Kivity wrote: > >> On 06/25/2012 05:17 PM, Gleb Natapov wrote: > >> > On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote: > >> >> On 06/25/2012 04:12 PM, Gleb Natapov wrote: > >> >> > >> >> >> Right. But I think we can have x86_linearize() that doesn't take a > >> >> >> context parameter, only ops. > >> >> >> > >> >> > All ops take context parameter though. > >> >> > > >> >> > >> >> context is meaningful for: > >> >> - saving state between executions (decode/execute/execute) > >> >> - passing state that is not provided via callbacks (regs/mode/flags) > >> >> - returning results > >> >> > >> >> Only the second is relevant, and we're trying to get rid of that too. > >> >> > >> > Callbacks were passed pointer to vcpu, but they were changed to get ctxt > >> > to better encapsulate emulator.c from rest of the KVM. Are you suggesting > >> > this was a mistake and we need to rework callbacks to receive pointer > >> > to vcpu again? I hope not :) > >> > >> Ouch. I guess we have to pass the context, but not initialize any of it > >> except ops. > > That's hacky and error pron. We need to audit that linearize() and all > > callbacks/functions it uses do not rely on un-initialized state, which > > is doable now, but who will remember to check that in the future, while > > changing seemingly unrelated code, which, by a coincidence, called during > > linearize()? Instant security vulnerability. For security (if not > > sanity) sake we should really make sure that ctxt is initialized while > > we are in emulator.c and make as many checks for it as possible. > > Agree. Though the security issue is limited; the structure won't be > uninitialized, it would retain values from the previous call. So it's > limited to intra-guest vulnerabilities. > Yes, that's the kind I mean, not host crash. Intra-guest vulnerabilities should not be taken lightly. From guest POV they are like buggy CPUs that allows privilege escalation. > > > >> Later we can extend x86_decode_insn() and the other > >> functions to follow the same rule. > >> > > What rule? We cannot not initialize a context. You can reduce things > > that should be initialized to minimum (getting GP registers on demand, > > etc), but still some initialization is needed since ctxt holds emulation > > state and it needs to be reset before each emulation. > > An alternative is to use two contexts, the base context only holds ops > and is the parameter to all the callbacks on the non-state APIs, the > derived context holds the state: > > struct x86_emulation_ctxt { > struct x86_ops *ops; > /* state that always needs to be initialized, preferablt none */ > }; > > struct x86_insn_ctxt { > struct x86_emulation_ctxt em; > /* instruction state */ > } > > and so we have a compile-time split between users of the state and > non-users. > I do not understand how you will divide current ctxt structure between those two. Where will you put those for instance: interruptibility, have_exception, perm_ok, only_vendor_specific_insn and how can they not be initialized before each instruction emulation? -- Gleb.