Re: [Xen-devel] [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 15.09.16 at 18:51, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
>> *hvmemul_ctxt,
>>          pfec |= PFEC_user_mode;
>>
>>      hvmemul_ctxt->insn_buf_eip = regs->eip;
>> -    if ( !vio->mmio_insn_bytes )
>> +
>> +    if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>> +    {
>> +        BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>> +                     sizeof(curr->arch.vm_event->emul.insn));
>
> This should quite clearly be !=, and I think it builds only because you
> use the wrong operand in the first sizeof().
>
>> +        hvmemul_ctxt->insn_buf_bytes = 
>> sizeof(curr->arch.vm_event->emul.insn);
>> +        memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>> +               hvmemul_ctxt->insn_buf_bytes);
>
> This memcpy()s between dissimilar types. Please omit the & and
> properly add .data on the second argument (and this .data
> addition should then also be mirrored in the BUILD_BUG_ON()).
>
>> +    }
>> +    else if ( !vio->mmio_insn_bytes )
>
> And then - I'm sorry for not having thought of this before - I think
> this would better not live here, or have an effect more explicitly
> only when coming here through hvm_emulate_one_vm_event().
> Since the former seems impractical, I think giving _hvm_emulate_one()
> one or two extra parameters would be the most straightforward
> approach.

So this is the spot where the mmio insn buffer is getting copied as
well instead of fetching the instructions from the guest memory. So
having the vm_event buffer getting copied here too makes the most
sense. Having the vm_event insn buffer getting copied in somewhere
else, while the mmio insn buffer getting copied here, IMHO just
fragments the flow even more making it harder to see what is actually
happening. How about adjusting the if-else here to be:

if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn  )
...
else if ( vio->mmio_insn_bytes )
...
else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )

?

Tamas

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