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

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

Hey Oleksandr!

On 8/5/21 11:59 AM, Oleksandr wrote:
> 
> 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 you unravel the craftiness that is xsm_default_action, there is
actually a bit of hierarchy there. If you set the default_action to
XSM_TARGET, it will first check if calling domain(src) is the target,
then falls into the XSM_DM_PRIV check which is if src->target == target,
and then finally checks if is_control_domain(src). That will constrict
the operation so that a domain can call it on itself, a device model
domain (stubdom) can call it on the domain it is backing, and the
control domain can make the call. I am not a 100% sure on this but I do
not believe a hardware domain would be able to make the call with it set
to XSM_TARGET and not employing Flask. Hope this helps.