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 <[email protected]>
> Release-acked-by: Henry Wang <[email protected]>
Reviewed-by: Jan Beulich <[email protected]> # 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
