Re: [Xen-devel] [PATCH] x86/hvm: Fix altp2m_op hypercall continuations

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/04/2019 19:13, Razvan Cojocaru wrote:
> On 4/8/19 8:39 PM, Andrew Cooper wrote:
>> c/s 9383de210 "x86/altp2m: support for setting restrictions for an array of
>> pages" introduced this logic, but do_hvm_op() was already capable of handling
>> -ERESTART correctly.
>>
>> More problematic however is a continuation from compat_altp2m_op().  The arg
>> written back into register state points into the hypercall XLAT area, not at
>> the original parameter passed by the guest.  It may be truncated by the
>> vmentry, but definitely won't be correct on the next invocation.
>>
>> Delete the hypercall_create_continuation() call, and return -ERESTART, which
>> will cause the compat case to start working correctly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> CC: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/x86/hvm/hvm.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index e798b49..cc89ee7 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4729,12 +4729,10 @@ static int do_altp2m_op(
>>          if ( rc > 0 )
>>          {
>>              a.u.set_mem_access_multi.opaque = rc;
>> +            rc = -ERESTART;
>>              if ( __copy_field_to_guest(guest_handle_cast(arg, 
>> xen_hvm_altp2m_op_t),
>>                                         &a, u.set_mem_access_multi.opaque) )
>>                  rc = -EFAULT;
>> -            else
>> -                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, 
>> "lh",
>> -                                                   HVMOP_altp2m, arg);
>>          }
>>          break;
> Right, that part was taken from the XENMEM_access_op_set_access_multi
> code and plopped in. Didn't follow the call chain all the way through
> and so missed the simpler way of doing this.

do_memory_op() is the other way around.  The compat wrapper is entered
first, and calls into the native path, and any set-up continuations are
then adjusted by hypercall_xlat_continuation() on the way back out.

All of this parameter handling is chronically complicated, and in
serious need of a more simple way to be found.

> The changes certainly look correct. I'll run some tests tomorrow as well.

Thanks,

~Andrew

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