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

Juergen Gross posted 1 patch 2 years, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220208074230.7901-1-jgross@suse.com
There is a newer version of this series
xen/include/public/memory.h | 5 +++++
1 file changed, 5 insertions(+)
[PATCH v2] xen/public: add comment to struct xen_mem_acquire_resource
Posted by Juergen Gross 2 years, 2 months ago
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.

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@suse.com>
---
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.
+     */
     uint32_t pad;
     /*
      * IN - the index of the initial frame to be mapped. This parameter
-- 
2.34.1


Re: [PATCH v2] xen/public: add comment to struct xen_mem_acquire_resource
Posted by Andrew Cooper 2 years, 2 months ago
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@suse.com>
> ---
> 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
Re: [PATCH v2] xen/public: add comment to struct xen_mem_acquire_resource
Posted by Juergen Gross 2 years, 2 months ago
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@suse.com>
>> ---
>> 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
Re: [PATCH v2] xen/public: add comment to struct xen_mem_acquire_resource
Posted by Jan Beulich 2 years, 2 months ago
On 08.02.2022 13:22, Juergen Gross wrote:
> On 08.02.22 12:55, Andrew Cooper wrote:
>> On 08/02/2022 07:42, Juergen Gross wrote:
>>> --- 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?

FWIW it is at least fine with me this way.

Jan