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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

Thank you for stepping up and trying to make HVM_PARAM better :).

On 10/02/2020 18:45, Andrew Cooper wrote:
> ARM currently has no restrictions on toolstack and guest access to the entire
> HVM_PARAM block.  As the paging/monitor/sharing features aren't under security
> support, this doesn't need an XSA.
Actually, only monitor is effectively working (yet not security 
supported) on Arm. The two others are x86 specific.
> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details exposed read-only to
> the guest, while the *_RING_PFN details are restricted to only toolstack
> access.  No other parameters are used.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>
> This is only compile tested, and based on my reading of the source.  There
> might be other PARAMS needing including.
> ---
>   xen/arch/arm/hvm.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 62 insertions(+), 3 deletions(-)
>
> 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.

> +    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?
> +         */
> +    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.
> +        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.
> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        return -EINVAL;
> +    }
> +}
Cheers,

--
Julien Grall

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