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

[Jan Beulich <jbeulich@xxxxxxxx>]

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, ...

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> # hypervisor
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?

> @@ -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".

Jan