Re: [PATCH v10 7/10] xen: implement new foreign copy hypercall

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Fri, 14 Aug 2026 at 15:13, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 14.08.2026 15:47, Frediano Ziglio wrote:
> > On Thu, 13 Aug 2026 at 15:22, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 13.08.2026 16:03, Frediano Ziglio wrote:
> >>> On Thu, 13 Aug 2026 at 10:41, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>> On 10.08.2026 12:30, Frediano Ziglio wrote:
> >>>>> ---
> >>>>>  xen/common/memory.c         | 149 ++++++++++++++++++++++++++++++++++++
> >>>>>  xen/include/public/memory.h |  45 ++++++++++-
> >>>>>  xen/include/xsm/dummy.h     |  14 ++++
> >>>>>  xen/include/xsm/hooks.h     |   2 +
> >>>>>  xen/xsm/flask/hooks.c       |  10 +++
> >>>>>  5 files changed, 219 insertions(+), 1 deletion(-)
> >>>>
> >>>> As before: If you insist on not implementing the compat case, that 
> >>>> decision
> >>>> wants justifying in the description. Without that it'll look like an
> >>>> oversight.
> >>>>
> >>>
> >>> Yes, I was just going to reply.
> >>> I spent multiple days trying to implement the compat case or simply
> >>> HVM support with an issue after the other:
> >>> - multiple distributions removed the 32 bit support so it was hard to
> >>> have a setup;
> >>> - the original hypercall this PR is trying to optimise is supported
> >>> only in PV (so no HVM or compat guests);
> >>> - migration and other operations can work only on PV (like dm_op
> >>> operation) due to the usage of userspace handles used.
> >>
> >> I don't understand how use of guest (not userspace) handles would get in
> >> the way of anything.
> >
> > In this case userspace is not a typo. For HVM
> > copy_from_user_hvm/copy_to_user_hvm are used and these functions
> > accept only kernel space pointers.
>
> I fear you've now completely lost me.
>

Try to pass a handle to a userspace page and the functions above will
fail because they won't accept userspace pages.

In guest_walk_tables you have:

    if ( walk & PFEC_user_mode ) /* Requested a user access. */
    {
        if ( !(ar & _PAGE_USER) )
            /* Got a supervisor walk?  Unconditional fail. */
            goto out;

        if ( (walk & PFEC_write_access) && !(ar & _PAGE_RW) )
            /* Requested a write and only got a read? Fail. */
            goto out;
    }
    else /* Requested a supervisor access. */
    {
        if ( ar & _PAGE_USER ) /* Got a user walk. */
        {
            if ( (walk & PFEC_insn_fetch) && guest_smep_enabled(v) )
                /* User insn fetch and smep? Fail. */
                goto out;

            if ( !(walk & PFEC_insn_fetch) && guest_smap_enabled(v) &&
                 ((walk & PFEC_implicit) ||
                  !(guest_cpu_user_regs()->eflags & X86_EFLAGS_AC)) )
                /* User data access and smap? Fail. */
                goto out;
        }

        if ( (walk & PFEC_write_access) && !(ar & _PAGE_RW) &&
             guest_wp_enabled(v) )
            /* Requested a write, got a read, and CR0.WP is set? Fail. */
            goto out;
    }

and we don't have a PFEC_user_mode set.

> >>>>> --- a/xen/common/memory.c
> >>>>> +++ b/xen/common/memory.c
> >>>>> @@ -1548,6 +1548,141 @@ static int acquire_resource(
> >>>>>      return rc;
> >>>>>  }
> >>>>>
> >>>>> +/*
> >>>>> + * The "noinline" qualifier avoids the compiler to create a large 
> >>>>> function
> >>>>> + * consuming quite a lot of stack.
> >>>>> + */
> >>>>> +static int noinline mem_foreigncopy(
> >>>>
> >>>> I'm wondering: Is the "mem" prefix really meaningful for a static 
> >>>> function in
> >>>> a file named memory.c?
> >>>>
> >>>
> >>> Changed
> >>>
> >>>>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> >>>>> +{
> >>>>> +    struct domain *d, *const currd = current->domain;
> >>>>
> >>>> With the comment on the new XSM hooks (below) in mind: currd wants to be
> >>>> pointer-to-const.
> >>>>
> >>>
> >>> Just rebased on master, all XSM hooks accept no-const pointers to domains.
> >>> So the suggested change would create warnings.
> >>
> >> Well, as per below, I pointed you at a particular pending patch, a single
> >> hunk of which could be broken out.
> >
> > Yes, but my changes would have to have casts from const pointers to
> > no-const pointers to avoid warnings and the patch you are pointing to
> > would have to remove these casts. I find this less clean than having
> > one patch using the current code style (that is no-const pointers) and
> > another that changes the style entirely.
> > But obviously this is just my opinion.
>
> Such casts would be unacceptable. What instead I have been trying to convey:
> Your patch wants to gain a dependency on my patch. And if my patch would
> take too long to make it in, that one hunk could be broken out into a
> separate, easy to get in patch.
>

Okay, then the only choice that's left is the code producing warnings
as const pointers are passed to functions requiring no-const pointers.
Is this acceptable? Apparently as you are suggesting it it is.

> >>>>> +            foreign = map_domain_page(foreign_mfn);
> >>>>> +            if ( direction == XENMEM_foreigncopy_from )
> >>>>> +                rc = copy_to_guest(copy.buffer, foreign, PAGE_SIZE);
> >>>>> +            else
> >>>>> +                rc = copy_from_guest(foreign, copy.buffer, PAGE_SIZE);
> >>>>
> >>>> What I continue to be missing prior to this is the obtaining of a 
> >>>> writable
> >>>> page ref. That's, as previously said, imperative for PV guests and at the
> >>>> very least advisable for HVM ones. (I really wonder how many more times I
> >>>> need to comment on this.)
> >>>
> >>> Unfortunately that does not work.
> >>> The code is coherent with MMU_UPDATE.
> >>
> >> How's that relevant? That's operating on page tables, when here we want to
> >> _prevent_ to copy into page tables (or descriptor ones, for that matter).
> >
> > This new ABI is to better support migration.
> > We are migrating all the VM status including page tables... how can we
> > not be able to write them but migrate them from one  host to another ?
> > You are basically explaining why changing the check the migration fails.
>
> No, what I'm trying to explain is that without such a check, you introduce
> a security issue (of privilege escalation kind). I hope you agree that we
> cannot knowingly allow such code to be committed.
>

Then the security issue is already present in the code without my changes.

> To migrate-in page tables, you'd need to copy their contents before they
> obtain their PGT_l<N>_page_table type, so that upon being converted to page
> tables, they can be properly audited by the mm.c functions we have for that
> exact purpose.
>

That makes sense.

> >>>>> --- a/xen/include/public/memory.h
> >>>>> +++ b/xen/include/public/memory.h
> >>>>> @@ -740,7 +740,50 @@ struct xen_vnuma_topology_info {
> >>>>>  typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
> >>>>>  DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
> >>>>>
> >>>>> -/* Next available subop number is 29 */
> >>>>> +/*
> >>>>> + * Copy memory from/to a given domain.
> >>>>> + * This calls is meant to replace expensive operations during 
> >>>>> migration which
> >>>>
> >>>> Nit: "This call is ..." However, is ...
> >>>>
> >>>>> + * are only supported for PV guests.
> >>>>
> >>>> ... this entire sentence really worth to have here (it looks more like
> >>>> something to have in the description)? For it to be possible to find if
> >>>> someone considered using those "expensive operations", I think it would 
> >>>> need
> >>>> to be less vague and name those operations. Furthermore, if those other
> >>>> operations were supported only for PV guests, how would migration work 
> >>>> for
> >>>> non-PV ones?
> >>>>
> >>>
> >>> Maybe:
> >>>     This call is meant to replace expensive operations (mmap/copy/munmap) 
> >>> during
> >>>     migration which can only be issued from PV guests.
> >>>
> >>> You can migrate any domain. Just from a PV guest (this is not a 
> >>> regression).
> >>
> >> Both Andrew and Roger confirm that this is supposed to work also from PVH
> >> Dom0 (not sure why you keep saying "guest"), and also used to work. If it
> >> doesn't, it would be a regression, and it would help if you supplied more
> >> detail on the observed failure.
> >
> > Indeed I tested the migration of various domains (PV, HVM, PV-in-PVH),
> > but only access to added hypercall from PV and HVM. I should add a
> > test from a PVH guest.
> > I say guest because to test HVM I used a hack to allow all guests (not
> > only dom0).
>
> And why would testing from PVH Dom0 not do?
>

Just that it's easier for me testing from a different guest.

> Jan

Frediano