Re: [Xen-devel] [PATCH v1] x86/hvm: Generic instruction re-execution mechanism for execute faults

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 19.11.2018 17:08, Roger Pau Monné wrote:
> On Mon, Nov 19, 2018 at 01:30:09PM +0000, Alexandru Stefan ISAILA wrote:
>>>> +    /* Now transform our RWX values in a XENMEM_access_* constant. */
>>>> +    if ( r == 0 && w == 0 && x == 0 )
>>>> +        new_access = XENMEM_access_n;
>>>> +    else if ( r == 0 && w == 0 && x == 1 )
>>>> +        new_access = XENMEM_access_x;
>>>> +    else if ( r == 0 && w == 1 && x == 0 )
>>>> +        new_access = XENMEM_access_w;
>>>> +    else if ( r == 0 && w == 1 && x == 1 )
>>>> +        new_access = XENMEM_access_wx;
>>>> +    else if ( r == 1 && w == 0 && x == 0 )
>>>> +        new_access = XENMEM_access_r;
>>>> +    else if ( r == 1 && w == 0 && x == 1 )
>>>> +        new_access = XENMEM_access_rx;
>>>> +    else if ( r == 1 && w == 1 && x == 0 )
>>>> +        new_access = XENMEM_access_rw;
>>>> +    else if ( r == 1 && w == 1 && x == 1 )
>>>> +        new_access = XENMEM_access_rwx;
>>>> +    else
>>>> +        new_access = required_access; /* Should never get here. */
>>>
>>> There seems to be a lot of translation from xenmem_access_t to bool
>>> fields and then to xenmem_access_t again. Can't you just avoid the
>>> booleans?
>>
>> The translation is done because the rights are cumulative and I think
>> this is the clear way to do this.
> 
> So the switch converts required_access using the following relation:
> 
> _r   -> r = 1 w = 0 x = 0
> _w   -> r = 0 w = 1 x = 0
> _x   -> r = 0 w = 0 x = 1
> _rx  -> r = 1 w = 1 x = 0
> _wx  -> r = 0 w = 1 x = 1
> _rw  -> r = 1 w = 1 x = 0
> _rwx -> r = 1 w = 1 x = 1
> 
> Then the if below performs the following transformation:
> 
> r = 0 w = 0 x = 0 -> _n
> r = 1 w = 0 x = 0 -> _r
> r = 0 w = 1 x = 0 -> _w
> r = 0 w = 0 x = 1 -> _x
> r = 1 w = 1 x = 0 -> _rw
> r = 0 w = 1 x = 1 -> _wx
> r = 1 w = 1 x = 0 -> _rw
> r = 1 w = 1 x = 1 -> _rwx
> 
> I'm not sure I understand this chunk of code, because you end up
> getting exactly the same type that you have as the input, and a type
> not listed here is just silently passed through, so I don't see the
> point in doing this transformation.

The first switch is for cur_access and it sets r,w,x accordingly,
the second switch is required_access where r,w,x are appended
and then in the last if().. part new_access is assigned according to the
previous assignments of r,w,x.

> 
>>
>>>>        if ( vm_event_check_ring(d->vm_event_monitor) &&
>>>>             d->arch.monitor.inguest_pagefault_disabled &&
>>>> -         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>>>> +         npfec.kind != npfec_kind_with_gla &&
>>>> +         hvm_funcs.start_reexecute_instruction ) /* don't send a 
>>>> mem_event */
>>>>        {
>>>> -        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, 
>>>> X86_EVENT_NO_EC);
>>>> -
>>>> +        v->arch.vm_event->emulate_flags = 0;
>>>> +        hvm_funcs.start_reexecute_instruction(v, gpa, XENMEM_access_rw);
>>>>            return true;
>>>>        }
>>>
>>> Don't you need to fallback to using hvm_emulate_one_vm_event if
>>> start_reexecute_instruction is not available?
>>
>> Fallback with hvm_emulate_one_vm_event can result in loosing events.
> 
> But by changing this here unconditionally you are removing this
> functionality on AMD hardware, which it used to have before by making
> use of hvm_emulate_one_vm_event.
> 
> I think this needs to at least be written in the commit message.

For AMD I could add if (cpu_has_svm()) and call emulate_one_vm_event. 
Introspection wise loosing valuable info is not a good thing to have 
because it could result in security breach.

> 
>>>> +        for_each_vcpu(d, v)
>>>> +        {
>>>> +            if ( !v->arch.rexec_level )
>>>> +                continue;
>>>> +
>>>> +            for ( i = v->arch.rexec_level - 1; i >= 0; i-- )
>>>
>>> Is there any reason this has to be done backwards?
>>>
>>> If you do it from 0 to v->arch.rexec_level you could use an unsigned
>>> int as the index.
>>
>> This is done backwards because of the corresponding code in
>> vmx_stop_reexecute_instruction() but here it can be turned the other way
>> if you insist on i to be unsigned.
> 
> Yes, Jan has also suggested a way to make i unsigned while keeping the
> loop backwards, but I don't see the point of performing the loop
> backwards if there's no need.
> 

There is no problem here, I can change it in the next version.

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