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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 05.08.21 18:03, Daniel P. Smith wrote:

Hi Daniel.

> 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.the
Thank you for pointing this out.
I am aware what the XSM_HOOK is, but I was thinking what the default 
action would be better suited for that hypercall, and failed to think of 
a better alternative.
I was going to choose XSM_TARGET, but the description "/* Can perform on 
self or your target domain */" confused me a bit, as there was no target 
domain involved as I thought, XSM_PRIV
sounded too strictly to me, etc. So, I decided to leave a "hook" for the 
RFC version. But, now I see that XSM_TARGET might be indeed better 
choice across all possible variants.

> > +        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.
Yes.


> > +    return xsm_default_action(action, current->domain, d);
> > +}
> V/r,
> Daniel P. Smith
--
Regards,

Oleksandr Tyshchenko