From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V2 1/3] xen/vm_access: Support for memory-content hiding Date: Fri, 26 Jun 2015 11:22:27 +0300 Message-ID: <558D0BC3.7060809@bitdefender.com> References: <1434359007-9302-1-git-send-email-rcojocaru@bitdefender.com> <1434359007-9302-2-git-send-email-rcojocaru@bitdefender.com> <558C41230200007800089C55@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <558C41230200007800089C55@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, jun.nakajima@intel.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, eddie.dong@intel.com, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com, keir@xen.org, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org On 06/25/2015 06:57 PM, Jan Beulich wrote: >>>> On 15.06.15 at 11:03, wrote: >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -578,6 +578,28 @@ static int hvmemul_read( >> container_of(ctxt, struct hvm_emulate_ctxt, ctxt)); >> } >> >> +static int hvmemul_read_set_context( >> + enum x86_segment seg, >> + unsigned long offset, >> + void *p_data, >> + unsigned int bytes, >> + struct x86_emulate_ctxt *ctxt) >> +{ >> + struct vcpu *curr = current; >> + unsigned int len; >> + >> + if ( !curr->arch.vm_event.emul_read_data ) >> + return X86EMUL_UNHANDLEABLE; >> + >> + len = min_t(unsigned int, >> + bytes, curr->arch.vm_event.emul_read_data->size); >> + >> + if ( len ) >> + memcpy(p_data, curr->arch.vm_event.emul_read_data->data, len); > > And the rest of the destination simply remains unmodified / > uninitialized? It does, yes. Should I memset() it to 0 or something else before the memcpy()? >> @@ -891,14 +934,37 @@ static int hvmemul_rep_outs( >> !!(ctxt->regs->eflags & X86_EFLAGS_DF), NULL); >> } >> >> -static int hvmemul_rep_movs( >> +static int hvmemul_rep_outs_set_context( >> + enum x86_segment src_seg, >> + unsigned long src_offset, >> + uint16_t dst_port, >> + unsigned int bytes_per_rep, >> + unsigned long *reps, >> + struct x86_emulate_ctxt *ctxt) >> +{ >> + struct vcpu *curr = current; >> + unsigned int safe_bytes; >> + >> + if ( !curr->arch.vm_event.emul_read_data ) >> + return X86EMUL_UNHANDLEABLE; >> + >> + safe_bytes = min_t(unsigned int, bytes_per_rep, >> + curr->arch.vm_event.emul_read_data->size); >> + >> + return hvmemul_do_pio(dst_port, reps, safe_bytes, 0, IOREQ_WRITE, >> + !!(ctxt->regs->eflags & X86_EFLAGS_DF), >> + curr->arch.vm_event.emul_read_data->data); > > This isn't consistent with e.g. rep_movs below - you shouldn't > reduce the width of the operation. I agree, but to be honest I wasn't sure of how to better go about this, it seem a bit more involved that the rest of the cases. I could perhaps allocate a bytes_per_rep-sized buffer, memset() it to zero and then copy safe_bytes from curr->arch.vm_event.emul_read_data->data to the beginning of it? > Also - did I overlook where *reps gets forced to 1? If it's being > done elsewhere, perhaps worth an ASSERT()? *reps got forced to 1 in hvmemul_rep_stos_set_context() in V1, I took that out as per your comments, please see: http://lists.xen.org/archives/html/xen-devel/2015-05/msg01054.html >> @@ -981,7 +1047,19 @@ static int hvmemul_rep_movs( >> */ >> rc = hvm_copy_from_guest_phys(buf, sgpa, bytes); >> if ( rc == HVMCOPY_okay ) >> + { >> + struct vcpu *curr = current; >> + >> + if ( unlikely(set_context) && curr->arch.vm_event.emul_read_data ) >> + { >> + unsigned long safe_bytes = min_t(unsigned long, bytes, >> + curr->arch.vm_event.emul_read_data->size); > > The variable doesn't need to be unsigned long. bytes is unsigned long in the function above: unsigned long saddr, daddr, bytes; I think that's the only place where that is the case, but I had assumed that whoever wrote the code knew better and followed suit. I'm happy to change both places though. >> @@ -1000,13 +1078,40 @@ static int hvmemul_rep_movs( >> return X86EMUL_OKAY; >> } >> >> -static int hvmemul_rep_stos( >> +static int hvmemul_rep_movs( >> + enum x86_segment src_seg, >> + unsigned long src_offset, >> + enum x86_segment dst_seg, >> + unsigned long dst_offset, >> + unsigned int bytes_per_rep, >> + unsigned long *reps, >> + struct x86_emulate_ctxt *ctxt) >> +{ >> + return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset, >> + bytes_per_rep, reps, ctxt, 0); >> +} >> + >> +static int hvmemul_rep_movs_set_context( >> + enum x86_segment src_seg, >> + unsigned long src_offset, >> + enum x86_segment dst_seg, >> + unsigned long dst_offset, >> + unsigned int bytes_per_rep, >> + unsigned long *reps, >> + struct x86_emulate_ctxt *ctxt) >> +{ >> + return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset, >> + bytes_per_rep, reps, ctxt, 1); >> +} > > Perhaps putting a flag hvmemul_ctxt would be better? Sorry, I don't understand the comment. Can you, please, clarify? >> @@ -1408,6 +1569,32 @@ static const struct x86_emulate_ops hvm_emulate_ops_no_write = { >> .invlpg = hvmemul_invlpg >> }; >> >> +static const struct x86_emulate_ops hvm_emulate_ops_set_context = { >> + .read = hvmemul_read_set_context, >> + .insn_fetch = hvmemul_insn_fetch, >> + .write = hvmemul_write, >> + .cmpxchg = hvmemul_cmpxchg_set_context, >> + .rep_ins = hvmemul_rep_ins, >> + .rep_outs = hvmemul_rep_outs_set_context, >> + .rep_movs = hvmemul_rep_movs_set_context, >> + .rep_stos = hvmemul_rep_stos_set_context, > > If you don't override .write, why would you override .rep_stos? I shouldn't, I got carried away with it (twice). I apologize. I'll remove the rep_stos customization in V3. >> @@ -1528,18 +1715,31 @@ int hvm_emulate_one_no_write( >> return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_no_write); >> } >> >> -void hvm_mem_access_emulate_one(bool_t nowrite, unsigned int trapnr, >> +int hvm_emulate_one_set_context( > > static? Ack. >> +void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr, >> unsigned int errcode) >> { >> struct hvm_emulate_ctxt ctx = {{ 0 }}; >> - int rc; >> + int rc = X86EMUL_UNHANDLEABLE; >> >> hvm_emulate_prepare(&ctx, guest_cpu_user_regs()); >> >> - if ( nowrite ) >> - rc = hvm_emulate_one_no_write(&ctx); >> - else >> + switch ( kind ) { >> + case EMUL_KIND_NORMAL: >> rc = hvm_emulate_one(&ctx); > > Perhaps this should be the default case? If not, the initialization > of rc would better be put in the default case instead of at the top > of the function. Ack, will make that the default case. >> @@ -63,6 +64,21 @@ static int vm_event_enable( >> vm_event_ring_lock_init(ved); >> vm_event_ring_lock(ved); >> >> + for_each_vcpu( d, v ) >> + { >> + if ( v->arch.vm_event.emul_read_data ) >> + break; > > continue? Of course, thanks. In addition to all the comments, Tamas has kindly pointed out that the correct name is mem_access, not vm_event (in the patch title), I'll be fixing that too in V3. Sorry for the typo. Thanks, Razvan