Re: [RFC PATCH] xen/memory: Introduce a hypercall to provide unallocated space

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/28/21 12:18 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

...

> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index e07bd9a..3f9b816 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1811,6 +1811,62 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              start_extent);
>          break;
>  
> +    case XENMEM_get_unallocated_space:
> +    {
> +        struct xen_get_unallocated_space xgus;
> +        struct xen_unallocated_region *regions;
> +
> +        if ( unlikely(start_extent) )
> +            return -EINVAL;
> +
> +        if ( copy_from_guest(&xgus, arg, 1) ||
> +             !guest_handle_okay(xgus.buffer, xgus.nr_regions) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_domain_by_any_id(xgus.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        rc = xsm_get_unallocated_space(XSM_HOOK, d);

Not sure if you are aware but XSM_HOOK is a no-op check, meaning that
you are allowing any domain to do this operation on any other domain. In
most cases there is an XSM check at the beginning of the hypercall
processing to do an initial clamp down but I am pretty sure there is no
prior XSM check on this path. Based on my understanding of how this is
intended, which may be incorrect, but I think you would actually want
XSM_TARGET.

> +        if ( rc )
> +        {
> +            rcu_unlock_domain(d);
> +            return rc;
> +        }
> +
> +        if ( !xgus.nr_regions || xgus.nr_regions > 
> XEN_MAX_UNALLOCATED_REGIONS )
> +        {
> +            rcu_unlock_domain(d);
> +            return -EINVAL;
> +        }
> +
> +        regions = xzalloc_array(struct xen_unallocated_region, 
> xgus.nr_regions);
> +        if ( !regions )
> +        {
> +            rcu_unlock_domain(d);
> +            return -ENOMEM;
> +        }
> +
> +        rc = arch_get_unallocated_space(d, regions, &xgus.nr_regions);
> +        if ( rc )
> +            goto unallocated_out;
> +
> +        if ( __copy_to_guest(xgus.buffer, regions, xgus.nr_regions) )
> +        {
> +            rc = -EFAULT;
> +            goto unallocated_out;
> +        }
> +
> +        if ( __copy_to_guest(arg, &xgus, 1) )
> +            rc = -EFAULT;
> +
> +unallocated_out:
> +        rcu_unlock_domain(d);
> +        xfree(regions);
> +
> +        break;
> +    }
> +
>      default:
>          rc = arch_memory_op(cmd, arg);
>          break;

...

> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 363c6d7..2761fbd 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -772,3 +772,9 @@ static XSM_INLINE int 
> xsm_domain_resource_map(XSM_DEFAULT_ARG struct domain *d)
>      XSM_ASSERT_ACTION(XSM_DM_PRIV);
>      return xsm_default_action(action, current->domain, d);
>  }
> +
> +static XSM_INLINE int xsm_get_unallocated_space(XSM_DEFAULT_ARG struct 
> domain *d)
> +{
> +    XSM_ASSERT_ACTION(XSM_HOOK);

For completeness, if you switch to XSM_TARGET at the call site, you will
want to change it here as well.

> +    return xsm_default_action(action, current->domain, d);
> +}

V/r,
Daniel P. Smith