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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/07/2015 04:27 PM, Jan Beulich wrote:
>>>> On 06.07.15 at 17:51, <rcojocaru@bitdefender.com> 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