Re: [PATCH 03/17] x86: split __copy_{from,to}_user() into "guest" and "unsafe" variants

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2021 17:06, Roger Pau Monné wrote:
> On Thu, Jan 14, 2021 at 04:04:32PM +0100, Jan Beulich wrote:
>> The "guest" variants are intended to work with (potentially) fully guest
>> controlled addresses, while the "unsafe" variants are not. Subsequently
>> we will want them to have different behavior, so as first step identify
>> which one is which. For now, both groups of constructs alias one another.
>>
>> Double underscore prefixes are retained only on
>> __copy_{from,to}_guest_pv(), to allow still distinguishing them from
>> their "checking" counterparts once they also get renamed (to
>> copy_{from,to}_guest_pv()).
>>
>> Add previously missing __user at some call sites.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Instead of __copy_{from,to}_guest_pv(), perhaps name them just
>> __copy_{from,to}_pv()?
>>
>> --- a/xen/arch/x86/gdbstub.c
>> +++ b/xen/arch/x86/gdbstub.c
>> @@ -33,13 +33,13 @@ gdb_arch_signal_num(struct cpu_user_regs
>>  unsigned int
>>  gdb_arch_copy_from_user(void *dest, const void *src, unsigned len)
>>  {
>> -    return __copy_from_user(dest, src, len);
>> +    return copy_from_unsafe(dest, src, len);
>>  }
>>  
>>  unsigned int 
>>  gdb_arch_copy_to_user(void *dest, const void *src, unsigned len)
>>  {
>> -    return __copy_to_user(dest, src, len);
>> +    return copy_to_unsafe(dest, src, len);
> 
> I assume we need to use the unsafe variants here, because the input
> addresses are fully controlled by gdb, and hence not suitable as
> speculation vectors?

Speculation doesn't matter when it comes to debugging, I
think. We were using the variants without access_ok()
checks already anyway to allow access to Xen addresses.
In fact it is my understanding ...

> Also could point to addresses belonging to both Xen or the guest
> address space AFAICT.

... that the primary goal here is to access Xen
addresses, and guest space only falls into the "may also
happen to be accessed" category.

>> --- a/xen/include/asm-x86/uaccess.h
>> +++ b/xen/include/asm-x86/uaccess.h
> 
> At some point we should also rename this to pvaccess.h maybe?

We could, but I'd rather not - this isn't about PV only.
Instead I would simply re-interpret 'u' from standing for
"user" (which didn't make much sense in Xen anyway, and
was only attributed to the Linux origin) to standing for
"unsafe" (both meanings - guest and in-Xen-but-unsafe).

>> @@ -197,21 +197,20 @@ do {
>>  #define get_guest_size get_unsafe_size
>>  
>>  /**
>> - * __copy_to_user: - Copy a block of data into user space, with less 
>> checking
>> - * @to:   Destination address, in user space.
>> - * @from: Source address, in kernel space.
>> + * __copy_to_guest_pv: - Copy a block of data into guest space, with less
>> + *                       checking
> 
> I would have preferred pv to be a prefix rather than a suffix, but we
> already have the hvm accessors using that nomenclature.

Right, I wanted to match that naming model. Later we can
think about renaming to copy_{to,from}_{hvm,pv}() or
whatever else naming scheme we like. I have to admit though
I'm not convinced the longer {hvm,pv}_copy_{from,to}_guest()
would really be better.

> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks!

Jan