diff mbox series

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

Message ID d1a1b9eb-33b4-4d07-9465-189699f88323@suse.com (mailing list archive)
State New
Headers show
Series x86/PV: avoid speculation abuse through guest accessors | expand

Commit Message

Jan Beulich Feb. 17, 2021, 8:21 a.m. UTC
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@suse.com>
---
v2: Re-base (bug fix side effect was taken care of already).

Comments

Roger Pau Monné Feb. 22, 2021, 3:31 p.m. UTC | #1
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@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

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?)

Thanks, Roger.
Jan Beulich Feb. 22, 2021, 3:55 p.m. UTC | #2
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@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

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
Roger Pau Monné Feb. 22, 2021, 4:08 p.m. UTC | #3
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@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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.
diff mbox series

Patch

--- 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)
 {
@@ -170,7 +167,7 @@  unsigned int dbg_rw_mem(void * __user ad
     if ( d )
     {
         if ( !d->is_dying )
-            len = dbg_rw_guest_mem(d, addr, buf, len, toaddr, pgd3);
+            len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3);
         rcu_unlock_domain(d);
     }
 
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -40,10 +40,8 @@ 
 #ifdef CONFIG_GDBSX
 static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
 {
-    void * __user gva = (void *)iop->gva, * __user uva = (void *)iop->uva;
-
-    iop->remain = dbg_rw_mem(gva, uva, iop->len, domid,
-                             !!iop->gwr, iop->pgd3val);
+    iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void),
+                             iop->len, domid, iop->gwr, iop->pgd3val);
 
     return iop->remain ? -EFAULT : 0;
 }
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -93,9 +93,9 @@  static inline bool debugger_trap_entry(
 #endif
 
 #ifdef CONFIG_GDBSX
-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);
+                        unsigned long pgd3);
 #endif
 
 #endif /* __X86_DEBUGGER_H__ */