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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 22, 2021 at 04:55:03PM +0100, Jan Beulich wrote:
> 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.

Yes, that's also fine as long as it's consistent.

Roger.