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 Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 20.09.16 at 16:56, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>>> 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.
>>>
>>> And I didn't unconditionally ask to move the copying elsewhere.
>>> The alternative - passing the override in as function argument(s),
>>> which would then be NULL/zero for all cases except the VM event
>>> one, would be as suitable. It is in particular ...
>>>
>>>> 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 )
>>>
>>> ... this curr->arch.vm_event reference which I'd like to see gone
>>> from this specific code path. The ordering in your original patch,
>>> otoh, would then be fine (check for the override first with unlikely(),
>>> else do what is being done today). Such a code structure would
>>> then also ease a possible second way of overriding the insn by
>>> some other party, without having to touch the code here again.
>>
>> So that check is one that Razvan asked to be added. I think it is
>> necessary too as there seems to be a race-condition if vm_event gets
>> shutdown after the response flag is set but before this emulation path
>> takes place. Effectively set_context_insn may be set but the
>> arch.vm_event already gotten freed. Razvan, is that correct?
>
> Well, in case you misunderstood: I didn't ask for the check to be
> _removed_, but for it to be _moved elsewhere_.
>

So as Razvan pointed out, there is a check already in hvm_do_resume
for exactly the same effect, so then what you are asking for is
already done.

Tamas

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