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

[Oleksandr <olekstysh@gmail.com>]

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@epam.com>
> ...
>
>> 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