Re: [Xen-devel] [PATCH RFC v3 2/2] x86/emulate: Send vm_event from emulate

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.19 at 17:42, <aisaila@xxxxxxxxxxxxxxx> wrote:
>>> @@ -1224,7 +1293,35 @@ int hvmemul_insn_fetch(
>>>           container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>       /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
>>>       uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
>>> +    paddr_t gpa;
>>> +    uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
>>> +    unsigned long addr, reps = 1;
>>> +    int rc;
>>> +    struct hvm_emulate_ctxt old;
>>> +
>>> +    rc = hvmemul_virtual_to_linear(seg, offset, bytes, &reps,
>>> +                                   hvm_access_insn_fetch, hvmemul_ctxt, 
>>> &addr);
>> 
>> The double translation is as problematic here, but what's worse:
>> 
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +    {
>>> +        x86_emul_reset_event(ctxt);
>>> +        rc = X86EMUL_OKAY;
>>> +    }
>> 
>> You zap an error here, but ...
> 
> In this case hvmemul_virtual_to_linear() can call
> x86_emul_hw_exception() and then signals the caller to inject the 
> exception. I don;t want to inject a non-user segment here and so I leave 
> the ctxt as it was before.
> 
>> 
>>> +    if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
>>> +        pfec |= PFEC_user_mode;
>>> +
>>> +    old = *hvmemul_ctxt;
>>> +    rc = hvmemul_linear_to_phys(addr, &gpa, bytes, &reps,
>>> +                                pfec, hvmemul_ctxt);
>> 
>> ... you happily use "addr" here anyway.
> 
> The address here is ok to be used or maybe a if ( rc != X86EMUL_OKAY ) 
> check can be put after getting the address.

addr is definitely not okay to be used if it wasn't initialized.

>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +    {
>>> +        *hvmemul_ctxt = old;
>>> +        rc = X86EMUL_OKAY;
>>> +    }
>>>   
>>> +    if ( gpa && hvmemul_send_vm_event(gpa, addr, gaddr_to_gfn(gpa),
>>> +                                      pfec, hvmemul_ctxt) )
>>> +        return X86EMUL_ACCESS_EXCEPTION;
>> 
>> Is there anything rendering gpa being zero an invalid / impossible
>> situation?
> 
> In tests I came across gpa == 0 so that is why the check was there I 
> will have to test is this can be solved with the X86EMUL_OKAY check from 
> above.

Even if you came across gpa 0 while testing, the solution to avoid
it (in case your possible solution turns out to not work) cannot be
to make it special in any way.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel