Re: [Xen-devel] [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.07.15 at 17:51, <rcojocaru@xxxxxxxxxxxxxxx> 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.

> @@ -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.

> +            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.

> @@ -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.

> @@ -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).

> @@ -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().

> @@ -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).

> +    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.

> @@ -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...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel