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

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

[Jan Beulich]

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

--- 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;
+    }
+
     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;
 
     switch ( domid )
     {
@@ -2590,7 +2584,6 @@ struct domain *get_pg_owner(domid_t domi
         break;
     }
 
- out:
     return pg_owner;
 }
 

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

[Julien Grall]

Hi Jan,

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.

So I agree that a domid can be figured out really easily today and in 
principle it would be fine to relax it.

However, most of the guest OSes will care about running on older Xen 
versions. Therefore they are not going to be able to use this relaxation.

So I am not entirely convinced the relaxation is actually worth it for 
existing hypercalls.

Anyway, if we decide to relax it, then I think we should update the 
public headers because an OS using this relaxation will not work on 
older Xen. A developper will not be able to know that without looking at 
the implementation.

Cheers,

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- 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;
> +    }
> +
>       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;
>   
>       switch ( domid )
>       {
> @@ -2590,7 +2584,6 @@ struct domain *get_pg_owner(domid_t domi
>           break;
>       }
>   
> - out:
>       return pg_owner;
>   }
>   
> 

-- 
Julien Grall

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

[Jan Beulich]

On 14.01.2021 15:43, Julien Grall wrote:
> 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.
> 
> So I agree that a domid can be figured out really easily today and in 
> principle it would be fine to relax it.
> 
> However, most of the guest OSes will care about running on older Xen 
> versions. Therefore they are not going to be able to use this relaxation.
> 
> So I am not entirely convinced the relaxation is actually worth it for 
> existing hypercalls.

I'm aware of the concern (Andrew has voiced it both here and in
earlier contexts), but personally I think undue restrictions should
not be retained just because they used to be enforced. We've gone
this same route in a few other occasions not overly long ago, and
iirc there are two patches going in a similar direction (different
area of course) that I have still pending and which neither got
ack-ed nor firmly rejected.

> Anyway, if we decide to relax it, then I think we should update the 
> public headers because an OS using this relaxation will not work on 
> older Xen. A developper will not be able to know that without looking at 
> the implementation.

Well, DOMID_SELF exists because that's the preferred form to use.
I can certainly add commentary, but it would feel a little odd to
do so. To be honest I'm also not sure how helpful this is going to
be, considering that consumers often have their own clones of our
headers.

Jan

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

[Andrew Cooper]

On 14/01/2021 15:30, Jan Beulich wrote:
> On 14.01.2021 15:43, Julien Grall wrote:
>> 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.
>> So I agree that a domid can be figured out really easily today and in 
>> principle it would be fine to relax it.
>>
>> However, most of the guest OSes will care about running on older Xen 
>> versions. Therefore they are not going to be able to use this relaxation.
>>
>> So I am not entirely convinced the relaxation is actually worth it for 
>> existing hypercalls.
> I'm aware of the concern (Andrew has voiced it both here and in
> earlier contexts), but personally I think undue restrictions should
> not be retained just because they used to be enforced. We've gone
> this same route in a few other occasions not overly long ago, and
> iirc there are two patches going in a similar direction (different
> area of course) that I have still pending and which neither got
> ack-ed nor firmly rejected.
>
>> Anyway, if we decide to relax it, then I think we should update the 
>> public headers because an OS using this relaxation will not work on 
>> older Xen. A developper will not be able to know that without looking at 
>> the implementation.
> Well, DOMID_SELF exists because that's the preferred form to use.
> I can certainly add commentary, but it would feel a little odd to
> do so. To be honest I'm also not sure how helpful this is going to
> be, considering that consumers often have their own clones of our
> headers.

What I envisioned would be an RST ::warning in the "how to grant table"
guide for guest kernel developers in the sphinx docs.

Of course, this presupposed that such a doc exists, but its the only
useful place to put a note.

~Andrew

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

[Julien Grall]

Hi Jan,

On 14/01/2021 15:30, Jan Beulich wrote:
> On 14.01.2021 15:43, Julien Grall wrote:
>> 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.
>>
>> So I agree that a domid can be figured out really easily today and in
>> principle it would be fine to relax it.
>>
>> However, most of the guest OSes will care about running on older Xen
>> versions. Therefore they are not going to be able to use this relaxation.
>>
>> So I am not entirely convinced the relaxation is actually worth it for
>> existing hypercalls.
> 
> I'm aware of the concern (Andrew has voiced it both here and in
> earlier contexts), but personally I think undue restrictions should
> not be retained just because they used to be enforced.

I don't disagree about the undue restrictions. My main concern is it 
makes more difficult for a developper to write portable code. The more 
when there is no easy way to find out the differences between Xen 
versions (see more below).

> We've gone
> this same route in a few other occasions not overly long ago, and
> iirc there are two patches going in a similar direction (different
> area of course) that I have still pending and which neither got
> ack-ed nor firmly rejected. >> Anyway, if we decide to relax it, then I think we should update the
>> public headers because an OS using this relaxation will not work on
>> older Xen. A developper will not be able to know that without looking at
>> the implementation.
> 
> Well, DOMID_SELF exists because that's the preferred form to use.
> I can certainly add commentary, but it would feel a little odd to
> do so.

Lets imagine your are the developer of a new OS but don't know Xen 
internal. How would you find out the difference between Xen interfaces?

With no documentation you have two choices:
    1) Dig into Xen code to understand the parameters
    2) Rely on the testing to find interface

Neither of the two solutions are great for the developper.

> To be honest I'm also not sure how helpful this is going to
> be, considering that consumers often have their own clones of our
> headers.
Right, but IMHO, anyone writing code that interface with the hypervisor 
should read the latest documentation/interface.

At the moment, most of our documentations are in the public headers. So 
it makes a suitable place.

We may need to duplicate the comment, but it is small enough to be fine.

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

On 15.01.2021 10:59, Julien Grall wrote:
> On 14/01/2021 15:30, Jan Beulich wrote:
>> On 14.01.2021 15:43, Julien Grall wrote:
>>> 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.
>>>
>>> So I agree that a domid can be figured out really easily today and in
>>> principle it would be fine to relax it.
>>>
>>> However, most of the guest OSes will care about running on older Xen
>>> versions. Therefore they are not going to be able to use this relaxation.
>>>
>>> So I am not entirely convinced the relaxation is actually worth it for
>>> existing hypercalls.
>>
>> I'm aware of the concern (Andrew has voiced it both here and in
>> earlier contexts), but personally I think undue restrictions should
>> not be retained just because they used to be enforced.
> 
> I don't disagree about the undue restrictions. My main concern is it 
> makes more difficult for a developper to write portable code. The more 
> when there is no easy way to find out the differences between Xen 
> versions (see more below).
> 
>> We've gone
>> this same route in a few other occasions not overly long ago, and
>> iirc there are two patches going in a similar direction (different
>> area of course) that I have still pending and which neither got
>> ack-ed nor firmly rejected. >> Anyway, if we decide to relax it, then I think we should update the
>>> public headers because an OS using this relaxation will not work on
>>> older Xen. A developper will not be able to know that without looking at
>>> the implementation.
>>
>> Well, DOMID_SELF exists because that's the preferred form to use.
>> I can certainly add commentary, but it would feel a little odd to
>> do so.
> 
> Lets imagine your are the developer of a new OS but don't know Xen 
> internal. How would you find out the difference between Xen interfaces?
> 
> With no documentation you have two choices:
>     1) Dig into Xen code to understand the parameters
>     2) Rely on the testing to find interface
> 
> Neither of the two solutions are great for the developper.

But why would such a developer research how to figure out the domain
ID when they can use DOMID_SELF? As said, it's not difficult, but
one still needs to know a few details and write some extra code.

>> To be honest I'm also not sure how helpful this is going to
>> be, considering that consumers often have their own clones of our
>> headers.
> Right, but IMHO, anyone writing code that interface with the hypervisor 
> should read the latest documentation/interface.
> 
> At the moment, most of our documentations are in the public headers. So 
> it makes a suitable place.
> 
> We may need to duplicate the comment, but it is small enough to be fine.

Okay, I've added this hunk for the grant table side:

--- unstable.orig/xen/include/public/grant_table.h
+++ unstable/xen/include/public/grant_table.h
@@ -447,6 +447,12 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_
  * source_offset specifies an offset in the source frame, dest_offset
  * the offset in the target frame and  len specifies the number of
  * bytes to be copied.
+ *
+ * Note that operations not specifying GNTCOPY_*_gref will be restricted
+ * to the local domain for the respective operands (source and/or
+ * destination.  Note further that prior to Xen 4.15 only DOMID_SELF
+ * would be accepted to specify this, i.e. the actual ID of the local
+ * domain can only be used successfully on 4.15 and newer.
  */
 
 #define _GNTCOPY_source_gref      (0)

The duplication will be more noticable for the get_pg_owner() aspect,
as that is used by multiple hypercall handlers, which I first need to
all hunt down, locate a suitable place for putting the comment in the
headers, and then perhaps - since I have to do this hunting down now
anyway - also see whether Andrew's concern needs further code changes
to address. So far for a simple code change, where I don't really buy
in to the benefit of doing this extra exercise ...

Jan

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

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

[Jan Beulich]

On 14.01.2021 16:01, Andrew Cooper wrote:
> On 14/01/2021 14:02, Jan Beulich wrote:
>> --- 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.

Since Julien had me look up all the call sites anyway (for adding
respective commentary in the public headers), I checked this
property as well: The only case where a foreign domain is strictly
required is the handling of XENMAPSPACE_gmfn_foreign, and there a
respective check exists.

I actually question do_mmuext_op()'s use of the function, as
neither DOMID_IO nor DOMID_XEN ought to be sensibly usable there.

Jan