Re: [PATCH V2 1/3] xen/vm_access: Support for memory-content hiding

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 06/25/2015 06:57 PM, Jan Beulich wrote:
>>>> On 15.06.15 at 11:03, <rcojocaru@bitdefender.com> 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