Re: [Xen-devel] [PATCH] xen/arm: Restrict access to most HVM_PARAM's

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Julien Grall <julien@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH] xen/arm: Restrict access to most HVM_PARAM's
Date: Wed, 19 Feb 2020 19:01:44 +0000	[thread overview]
Message-ID: <4e0bd8ce-5f51-ce1c-2429-348507a86edb@citrix.com> (raw)
In-Reply-To: <76a0538a-f4e5-65ba-4dc7-aa4fe5a4fe6f@xen.org>

On 19/02/2020 15:53, Julien Grall wrote:
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 76b27c9168..1446d4010c 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -31,6 +31,60 @@
>>     #include <asm/hypercall.h>
>>   +static int hvm_allow_set_param(const struct domain *d, unsigned
>> int param)
>> +{
>
> Should we move the XSM check here too? This is not too important though.

Currently there is a single XSM call, before the get/set split.  Moving
it in here would create two.

i.e. the current way compiles smaller and will run (fractionally) faster.

>
>> +    switch ( param )
>> +    {
>> +        /*
>> +         * The following parameters are intended for toolstack usage
>> only.
>> +         * They may not be set by the domain.
>> +         *
>> +         * The {STORE,CONSOLE}_EVTCHN values will need to become
>> read/write if
>> +         * a new ABI hasn't appeared by the time migration support
>> is added.
>
> The comment suggests {STORE, CONSOLE}_EVTCHN values should not be
> read/write. But you implement them as read/write. Is it intended?

It is currently read/write only to !SELF (i.e. only the toolstack or
qemu stub).

If it needs to change in the future, it needs to become a separate block
in the switch statement which returns 0 straight away.

Would "read/write to the guest if" be clearer in the comment?

>
>> +         */
>> +    case HVM_PARAM_CALLBACK_IRQ:
>> +    case HVM_PARAM_STORE_PFN:
>> +    case HVM_PARAM_STORE_EVTCHN:
>> +    case HVM_PARAM_CONSOLE_PFN:
>> +    case HVM_PARAM_CONSOLE_EVTCHN:
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>
> I would drop HVM_PARAM_PAGING_RING_PFN and HVM_PARAM_SHARING_RING_PFN
> as they are not used by Arm and AFAICT the toolstack will not set them.

Even better.

>
>> +        return d == current->domain ? -EPERM : 0;
>> +
>
> Looking at the list of HVM param, I think you forgot to add
> HVM_PARAM_VM_GENERATION_ID_ADDR.

It is a windows specific thing.  The spec dates from a pre-ARM time, and
while there is nothing obviously x86-only, it does depend on the AML
(via the DSDT or an SSDT).

Either way, its only relevant for migration and/or revert-to-snapshot,
so is of no use to Xen on ARM at this point in time.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel