| Message ID | d1a1b9eb-33b4-4d07-9465-189699f88323@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/PV: avoid speculation abuse through guest accessors | expand |
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.
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
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.
--- 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__ */
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).