Re: [PATCH v2] xen/public: add comment to struct xen_mem_acquire_resource

[Juergen Gross <jgross@xxxxxxxx>]

On 08.02.22 12:55, Andrew Cooper wrote:
> On 08/02/2022 07:42, Juergen Gross wrote:
> > Commit 7c7f7e8fba01 changed xen/include/public/memory.h in an incompatible
> > way. Unfortunately the changed parts were already in use in the Linux
> > kernel, so an update of the header in the kernel would result in a build
> > breakage.
> >
> > As the change of above commit was in a section originally meant to be not
> > stable, it was the usage in the kernel which was wrong.
> While I hate to drag the argument on, this is wrong.
>
> Instead of speculating, why don't we actually look at the code.  From Linux:
>
> unsigned int domid =
>          (xdata.flags & XENMEM_rsrc_acq_caller_owned) ?
>          DOMID_SELF : kdata.dom;
> ...
> num = xen_remap_domain_mfn_array(vma,
>                                   kdata.addr & PAGE_MASK,
>                                   pfns, kdata.num, errs,
>                                   vma->vm_page_prot,
>                                   domid);
>
> Under the original implementation, it was literally not possible for a
> kernel to avoid using XENMEM_rsrc_acq_caller_owned, because it
> determined which domid needed feeding into a subsequent foreign map command.
While nasty I think it would have been possible to split the operation
done in the kernel's privcmd_ioctl_mmap_resource() into several pieces
and let the Xen tools decide which domid to use.

The original interface definition did not mandate to be usable in the
kernel only, it was just done this way, because it was easier.

> The constant was therefore always part of the fully public ABI, however
> it may have been intended, and subsequent claims to the contrary
> (notably, those used to justify its deletion) are false.
>
> The security fix for Xen was to prohibit creating situations where we
> fed caller_owned back to the kernel.  This is ABI compatible, merely
> creating a dead logic path in the kernel.
>
> > Add a comment to the modified struct for not reusing the now removed bit,
> > in order to avoid kernels using it stumbling over a possible new meaning
> > of the bit.
> >
> > In case the kernel is updating to a new version of the header, the wrong
> > use case must be removed first.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - only add comment instead of reverting commit 7c7f7e8fba01 (Jan Beulich)
> > ---
> >   xen/include/public/memory.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index 383a9468c3..86513057f7 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -662,6 +662,11 @@ struct xen_mem_acquire_resource {
> >        * two calls.
> >        */
> >       uint32_t nr_frames;
> > +    /*
> > +     * Padding field, must be zero on input.
> > +     * The lowest bit was named XENMEM_rsrc_acq_caller_owned in a previous
> > +     * version and should not be reused in future.
> s/should/will/.  This is a statement of how Xen shall behave.
Okay.

> I think it's also worth somehow fitting in that it's an output only
> bit.  It will be important when inevitably we end up changing this back
> to being a flags field when we need to extend the hypercall.
Okay.

In the end the bit only needs to be reserved, if pad _is_ zero on input.
So the correct way to phrase it would be:

/*
 * Padding field, must be zero on input.
 * In a previous version this was an output field with the lowest
 * bit named XENMEM_rsrc_acq_caller_owned. Future versions of this
 * interface will not reuse this bit with the field being zero on
 * input.
 */

Is this fine with you?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature