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 <jbeul...@suse.com>
> >
> > Acked-by: Roger Pau Monné <roger....@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.
