| Message ID | 72c9f0ec-81e3-63f9-2513-46e463642219@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | common: don't require use of DOMID_SELF | expand |
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; > } > >
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
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
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
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
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,
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
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
--- 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; }
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>