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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 06/26/2015 11:44 AM, Jan Beulich wrote:
>>>> @@ -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?
> See above - you first need to define the model you want to follow,
> and then you need to handle this uniformly everywhere.
> 
>>> >> 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 
> That's fine, but then you can't simply ignore it in the safe_bytes
> calculation.

Sorry, I don't follow - I did truncate the data if bytes_per_rep >
curr->arch.vm_event.emul_read_data->size. But I'm passing reps on
untouched to hvmemul_do_pio(). Did I misunderstand the hvmemul_do_pio()
code? Should I pass a (*reps * bytes_per_rep) sized buffer to
hvmemul_do_pio() when its last parameter is != NULL?


Thanks,
Razvan