Re: [PATCH v2 5/8] x86/gdbsx: convert "user" to "guest" accesses

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.02.2021 16:31, Roger Pau Monné wrote:
> On Wed, Feb 17, 2021 at 09:21:36AM +0100, Jan Beulich wrote:
>> Using copy_{from,to}_user(), this code was assuming to be called only by
>> PV guests. Use copy_{from,to}_guest() instead, transforming the incoming
>> structure field into a guest handle (the field should really have been
>> one in the first place). Also do not transform the debuggee address into
>> a pointer.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> One minor comment below that can be taken care of when committing I
> think.
> 
>> ---
>> v2: Re-base (bug fix side effect was taken care of already).
>>
>> --- a/xen/arch/x86/debug.c
>> +++ b/xen/arch/x86/debug.c
>> @@ -108,12 +108,11 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct doma
>>  }
>>  
>>  /* Returns: number of bytes remaining to be copied */
>> -static unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr,
>> -                                     void * __user buf, unsigned int len,
>> -                                     bool toaddr, uint64_t pgd3)
>> +static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr,
>> +                                     XEN_GUEST_HANDLE_PARAM(void) buf,
>> +                                     unsigned int len, bool toaddr,
>> +                                     uint64_t pgd3)
>>  {
>> -    unsigned long addr = (unsigned long)gaddr;
>> -
>>      while ( len > 0 )
>>      {
>>          char *va;
>> @@ -134,20 +133,18 @@ static unsigned int dbg_rw_guest_mem(str
>>  
>>          if ( toaddr )
>>          {
>> -            copy_from_user(va, buf, pagecnt);    /* va = buf */
>> +            copy_from_guest(va, buf, pagecnt);
>>              paging_mark_dirty(dp, mfn);
>>          }
>>          else
>> -        {
>> -            copy_to_user(buf, va, pagecnt);    /* buf = va */
>> -        }
>> +            copy_to_guest(buf, va, pagecnt);
>>  
>>          unmap_domain_page(va);
>>          if ( !gfn_eq(gfn, INVALID_GFN) )
>>              put_gfn(dp, gfn_x(gfn));
>>  
>>          addr += pagecnt;
>> -        buf += pagecnt;
>> +        guest_handle_add_offset(buf, pagecnt);
>>          len -= pagecnt;
>>      }
>>  
>> @@ -161,7 +158,7 @@ static unsigned int dbg_rw_guest_mem(str
>>   * pgd3: value of init_mm.pgd[3] in guest. see above.
>>   * Returns: number of bytes remaining to be copied.
>>   */
>> -unsigned int dbg_rw_mem(void * __user addr, void * __user buf,
>> +unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
>>                          unsigned int len, domid_t domid, bool toaddr,
>>                          uint64_t pgd3)
> 
> You change the prototype below to make pgd3 unsigned long, so you
> should change the type here also? (and likely in dbg_rw_guest_mem?)

I'd rather undo the change to the prototype, or else further
changes would be needed for consistency. I'll take it that
you're fine either way, and hence your ack stands.

Thanks for noticing, Jan