Re: [Xen-devel] [PATCH v8] x86/altp2m: support for setting restrictions for an array of pages

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.10.17 at 12:19, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
> hence with the original altp2m design, where domains are allowed - with the
> proper altp2m access rights - to alter these settings), in the absence of an
> official position on the issue from the original altp2m designers.

I continue to disagree with this reasoning. I'm afraid I'm not really
willing to allow widening the badness, unless altp2m was formally
documented security-unsupported.

> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_t domid,
> +                                   uint16_t view_id, uint8_t *access,
> +                                   uint64_t *pages, uint32_t nr)
> +{
> +    int rc;
> +
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +    DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
> +                             XC_HYPERCALL_BUFFER_BOUNCE_IN);

This is confusing: Why "* sizeof()" in the latter expression, but not
in the former. It would anyway be better to use sizeof(*pages) (and
then also sizeof(*access)) to clearly make the connection.

> @@ -4568,6 +4571,37 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>  
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        if ( a.u.set_mem_access_multi.pad ||
> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }

Just like in a number of other recent cases: This operation should not
fail when .nr is zero. Yet as per above it will.

> +static int compat_altp2m_op(
> +    XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    int rc = 0;
> +    struct compat_hvm_altp2m_op a;
> +    union
> +    {
> +        XEN_GUEST_HANDLE_PARAM(void) hnd;
> +        struct xen_hvm_altp2m_op *altp2m_op;
> +    } nat;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return -EOPNOTSUPP;
> +
> +    if ( copy_from_guest(&a, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( a.pad1 || a.pad2 ||
> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> +        return -EINVAL;
> +
> +    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> +    switch ( a.cmd )
> +    {
> +    case HVMOP_altp2m_set_mem_access_multi:
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> +        guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> +        guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +        
> XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
> +                                             &a.u.set_mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> +        break;
> +    default:

Blank line between individual case blocks please (also below).

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -28,6 +28,7 @@ headers-$(CONFIG_X86)     += compat/arch-x86/xen.h
>  headers-$(CONFIG_X86)     += compat/arch-x86/xen-$(compat-arch-y).h
>  headers-$(CONFIG_X86)     += compat/hvm/hvm_vcpu.h
>  headers-$(CONFIG_X86)     += compat/hvm/dm_op.h
> +headers-$(CONFIG_X86)     += compat/hvm/hvm_op.h

I realize the dm_op.h insertion was already to the wrong spot, but
please let's not make things worse: Anywhere that order doesn't
otherwise matter, please sort alphabetically, to prevent everyone
adding to the end (and hence increasing the risk of patch conflicts).

> @@ -237,6 +246,20 @@ struct xen_hvm_altp2m_set_mem_access {
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>  
> +struct xen_hvm_altp2m_set_mem_access_multi {
> +    /* view */
> +    uint16_t view;
> +    uint16_t pad;
> +    /* Number of pages */
> +    uint32_t nr;
> +    /* Used for continuation purposes */
> +    uint64_t opaque;

The comment should also state that this needs to be set to zero
upon initial invocation.

Jan

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