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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

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.

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.

~Andrew