Re: [PATCH] common: don't require use of DOMID_SELF

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2021 16:01, Andrew Cooper wrote:
> On 14/01/2021 14:02, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -2776,15 +2776,19 @@ struct gnttab_copy_buf {
>>  static int gnttab_copy_lock_domain(domid_t domid, bool is_gref,
>>                                     struct gnttab_copy_buf *buf)
>>  {
>> -    /* Only DOMID_SELF may reference via frame. */
>> -    if ( domid != DOMID_SELF && !is_gref )
>> -        return GNTST_permission_denied;
>> -
>>      buf->domain = rcu_lock_domain_by_any_id(domid);
>>  
>>      if ( !buf->domain )
>>          return GNTST_bad_domain;
>>  
>> +    /* Only the local domain may reference via frame. */
>> +    if ( buf->domain != current->domain && !is_gref )
>> +    {
>> +        rcu_unlock_domain(buf->domain);
>> +        buf->domain = NULL;
>> +        return GNTST_permission_denied;
>> +    }
> 
> In this case, it's also a weird asymmetry where this is one grant table
> operation which a privileged domain can't issue on behalf of an
> unprivileged one.

Well, in a way, perhaps. If it was useful, perhaps it would have
been made work and allowed, so I wonder whether there simply is
no good use for it?

>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2566,13 +2566,7 @@ __initcall(register_heap_trigger);
>>  
>>  struct domain *get_pg_owner(domid_t domid)
>>  {
>> -    struct domain *pg_owner = NULL, *curr = current->domain;
>> -
>> -    if ( unlikely(domid == curr->domain_id) )
>> -    {
>> -        gdprintk(XENLOG_WARNING, "Cannot specify itself as foreign 
>> domain\n");
>> -        goto out;
>> -    }
>> +    struct domain *pg_owner;
> 
> I'm not sure this is correct.
> 
> It isn't a DOMID_SELF check.  It's a "confirm the nominated domid is
> remote" check, and I don't see all the callers of this interface having
> appropriate checks to prohibit trying to do a foreign operation on
> oneself, however they specify the foreign domid.

No, I don't think so. Prior to a625c335593e ("common: don't (kind
of) open-code rcu_lock_domain_by_any_id()") DOMID_SELF was explicitly
permitted. As of that change, it's implicitly permitted. I don't see
how using DOMID_SELF would be okay when using the numeric ID isn't.
(I'm not going to exclude there may be missing checks in some of the
callers, but from prior audits I don't recall recognizing any.)

Jan