Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jason Andryuk <jandryuk@gmail.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Xen Security Team <security@xen.org>,
	Jan Beulich <JBeulich@suse.com>,
	Roger Pau Monne <roger.pau@citrix.com>, Wei Liu <wl@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Henry Wang <Henry.Wang@arm.com>,
	Anthony Perard <anthony.perard@citrix.com>
Subject: Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls
Date: Wed, 26 Oct 2022 13:25:46 +0000	[thread overview]
Message-ID: <ce5f35a9-c3d0-5c07-f8e9-d1a53eadb1bd@citrix.com> (raw)
In-Reply-To: <CAKf6xptnDHePBLxE06bBUEkrgetg5-0X1TFFSDyJqPv4TCvA4A@mail.gmail.com>

On 26/10/2022 14:22, Jason Andryuk wrote:
> On Wed, Oct 26, 2022 at 6:21 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>
>> First of all, with ARM borrowing x86's implementation, the logic to set the
>> pool size should have been common, not duplicated.  Introduce
>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>> libxl/xl can reasonably function without the ability to query the pool size...
>>
>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>> replaced with a working and unit tested interface.
>>
>> This is part of XSA-409 / CVE-2022-33747.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
>> index 2abaab439c4f..f93b221f1c1f 100644
>> --- a/tools/libs/light/libxl_dom.c
>> +++ b/tools/libs/light/libxl_dom.c
>> @@ -1448,6 +1448,25 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
>>      return rc;
>>  }
>>
>> +int libxl__domain_set_p2m_pool_size(
>> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
>> +{
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +    uint64_t shadow_mem;
>> +
>> +    shadow_mem = d_config->b_info.shadow_memkb;
>> +    shadow_mem <<= 10;
>> +
>> +    int r = xc_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem);
> Should this be xc_*set*_p2m_mempool_size?

Hmm, yes it should be.

And the reason this doesn't break any tests is because all examples in
CI match the default that Xen that sets.

~Andrew