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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/01/2021 14:02, Jan Beulich wrote:
> It's not overly difficult for a domain to figure out its ID, so
> requiring the use of DOMID_SELF in a very limited set of places isn't
> really helpful towards keeping the ID opaque to the guest.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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.

> +
>      buf->ptr.domid = domid;
>  
>      return GNTST_okay;
> --- 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.

~Andrew