From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding Date: Tue, 7 Jul 2015 18:32:35 +0300 Message-ID: <559BF113.3090905@bitdefender.com> References: <1436197873-4559-1-git-send-email-rcojocaru@bitdefender.com> <1436197873-4559-2-git-send-email-rcojocaru@bitdefender.com> <559BEFD9020000780008D794@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559BEFD9020000780008D794@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: jun.nakajima@intel.com, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.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, tlengyel@novetta.com, keir@xen.org, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org On 07/07/2015 04:27 PM, Jan Beulich wrote: >>>> On 06.07.15 at 17:51, wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -269,6 +269,7 @@ struct vcpu *alloc_vcpu_struct(void) >> >> void free_vcpu_struct(struct vcpu *v) >> { >> + xfree(v->arch.vm_event.emul_read_data); >> free_xenheap_page(v); >> } > > Please note the function's name - nothing else should be freed here imo. Ack, moved it to alloc_vcpu() and complete_domain_destroy() (the callers of free_vcpu_struct(), in xen/common/domain.c). >> @@ -893,6 +915,24 @@ static int hvmemul_cmpxchg( >> unsigned int bytes, >> struct x86_emulate_ctxt *ctxt) >> { >> + struct hvm_emulate_ctxt *hvmemul_ctxt = >> + container_of(ctxt, struct hvm_emulate_ctxt, ctxt); >> + >> + if ( unlikely(hvmemul_ctxt->set_context) ) >> + { >> + struct vcpu *curr = current; >> + >> + if ( curr->arch.vm_event.emul_read_data ) >> + { > > hvmemul_read() returns X86EMUL_UNHANDLEABLE when > !curr->arch.vm_event.emul_read_data. I think this ought to > be consistent. Ack. >> + unsigned int safe_bytes = min_t(unsigned int, bytes, >> + curr->arch.vm_event.emul_read_data->size); >> + >> + memset(p_new, 0, bytes); >> + memcpy(p_new, curr->arch.vm_event.emul_read_data->data, >> + safe_bytes); > > Here as well as in elsewhere - pretty inefficient to zero the > whole array in order to then possibly overwrite all of it. I'd really > like to see only the excess bytes to be zeroed. Ack, now zeroing only the remainder. >> @@ -935,6 +975,41 @@ static int hvmemul_rep_ins( >> !!(ctxt->regs->eflags & X86_EFLAGS_DF), gpa); >> } >> >> +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) >> +{ >> + unsigned int bytes = *reps * bytes_per_rep; >> + struct vcpu *curr = current; >> + unsigned int safe_bytes; >> + char *buf = NULL; >> + int rc; >> + >> + if ( !curr->arch.vm_event.emul_read_data ) >> + return X86EMUL_UNHANDLEABLE; >> + >> + buf = xmalloc_bytes(bytes); >> + >> + if ( buf == NULL ) >> + return X86EMUL_UNHANDLEABLE; >> + >> + memset(buf, 0, bytes); > > If this were to stay (see above), xzalloc_array() above please. And > xmalloc_array() otherwise. Changed it to xmalloc_array() and zeroing out only the reminder. >> @@ -1063,7 +1142,20 @@ static int hvmemul_rep_movs( >> */ >> rc = hvm_copy_from_guest_phys(buf, sgpa, bytes); >> if ( rc == HVMCOPY_okay ) >> + { >> + struct vcpu *curr = current; >> + >> + if ( unlikely(hvmemul_ctxt->set_context) && >> + curr->arch.vm_event.emul_read_data ) >> + { >> + unsigned int safe_bytes = min_t(unsigned int, bytes, >> + curr->arch.vm_event.emul_read_data->size); >> + >> + memcpy(buf, curr->arch.vm_event.emul_read_data->data, safe_bytes); >> + } > > This again is inconsistent with what you do above: You need to > decide whether excess data (beyond safe_bytes) is safe to read > (like you do here) or should be returned as zeroes (as done above). An omission, now fixed. >> @@ -1599,6 +1711,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, >> int hvm_emulate_one( >> struct hvm_emulate_ctxt *hvmemul_ctxt) >> { >> + hvmemul_ctxt->set_context = 0; > > I think this would better go into hvm_emulate_prepare(). Ack. >> @@ -1616,10 +1736,16 @@ void hvm_mem_access_emulate_one(bool_t nowrite, >> unsigned int trapnr, >> >> hvm_emulate_prepare(&ctx, guest_cpu_user_regs()); >> >> - if ( nowrite ) >> + switch ( kind ) { > > Coding style (also elsewhere in the patch). Sorry about that, fixed. >> + case EMUL_KIND_NOWRITE: >> rc = hvm_emulate_one_no_write(&ctx); >> - else >> + break; >> + case EMUL_KIND_SET_CONTEXT: >> + rc = hvm_emulate_one_set_context(&ctx); > > Unless you see future uses for it, please expand the wrapper here. Now that setting set_context to 0 happens in hvm_emulate_one_no_write() I'm just setting it to 1 and falling through to the default case here. >> @@ -1552,9 +1556,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, >> >> if ( v->arch.vm_event.emulate_flags ) >> { >> - hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags & >> - MEM_ACCESS_EMULATE_NOWRITE) != 0, >> - TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE); >> + enum emul_kind kind = EMUL_KIND_NORMAL; >> + >> + if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA ) >> + kind = EMUL_KIND_SET_CONTEXT; >> + else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_EMULATE_NOWRITE ) > > Is there code in place rejecting both flags being set at once? I don't > recall having seen any... No, there isn't. Both flags can be set at once, but if so only the SET_EMUL_READ_DATA will be honored. Thanks, Razvan