Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the paging mempool size

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 17/11/2022 10:18, Jan Beulich wrote:
> On 17.11.2022 02:08, Andrew Cooper wrote:
>> The existing XEN_DOMCTL_SHADOW_OP_{GET,SET}_ALLOCATION have problems:
>>
>>  * All set_allocation() flavours have an overflow-before-widen bug when
>>    calculating "sc->mb << (20 - PAGE_SHIFT)".
>>  * All flavours have a granularity of 1M.  This was tolerable when the size of
>>    the pool could only be set at the same granularity, but is broken now that
>>    ARM has a 16-page stopgap allocation in use.
>>  * All get_allocation() flavours round up, and in particular turn 0 into 1,
>>    meaning the get op returns junk before a successful set op.
>>  * The x86 flavours reject the hypercalls before the VM has vCPUs allocated,
>>    despite the pool size being a domain property.
>>  * Even the hypercall names are long-obsolete.
>>
>> Implement a better interface, which can be first used to unit test the
>> behaviour, and subsequently correct a broken implementation.  The old
>> interface will be retired in due course.
>>
>> The unit of bytes (as opposed pages) is a deliberate API/ABI improvement to
>> more easily support multiple page granularities.
>>
>> This is part of XSA-409 / CVE-2022-33747.
> While I'm not convinced of this attribution, ...

I think this very much depends on how critical the unit test is deemed.

If this was done the first time around, it would all have had
attribution.  We're on the 3rd set of fixes, and the unit test is a key
justification of the safety of the fix.

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com> # hypervisor

Thanks.

> albeit with remarks:
>
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -977,6 +977,49 @@ int __init paging_set_allocation(struct domain *d, unsigned int pages,
>>  }
>>  #endif
>>  
>> +int arch_get_paging_mempool_size(struct domain *d, uint64_t *size)
>> +{
>> +    int rc;
>> +
>> +    if ( is_pv_domain(d) )                 /* TODO: Relax in due course */
>> +        return -EOPNOTSUPP;
> I guess this is merely for symmetry with ...
>
>> +int arch_set_paging_mempool_size(struct domain *d, uint64_t size)
>> +{
>> +    unsigned long pages = size >> PAGE_SHIFT;
>> +    bool preempted = false;
>> +    int rc;
>> +
>> +    if ( is_pv_domain(d) )                 /* TODO: Relax in due course */
>> +        return -EOPNOTSUPP;
> ... this, since otherwise "get" ought to be fine for PV?

Its the safest course of action, given other known issues with PV. 
There's no need for a working get without a working set.

>
>> @@ -946,6 +949,22 @@ struct xen_domctl_cacheflush {
>>      xen_pfn_t start_pfn, nr_pfns;
>>  };
>>  
>> +/*
>> + * XEN_DOMCTL_get_paging_mempool_size / XEN_DOMCTL_set_paging_mempool_size.
>> + *
>> + * Get or set the paging memory pool size.  The size is in bytes.
>> + *
>> + * This is a dedicated pool of memory for Xen to use while managing the guest,
>> + * typically containing pagetables.  As such, there is an implementation
>> + * specific minimum granularity.
>> + *
>> + * The set operation can fail mid-way through the request (e.g. Xen running
>> + * out of memory, no free memory to reclaim from the pool, etc.).
>> + */
>> +struct xen_domctl_paging_mempool {
>> +    uint64_aligned_t size; /* IN/OUT.  Size in bytes. */
> While likely people will correctly infer what is meant, strictly speaking
> this is wrong: The field is IN for "set" and OUT for "get".

I'll drop them, to reduce any possible confusion.  As you say, the
meaning is entirely clear.

~Andrew