[Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
Qemu needs access to this for the domain it controls, both due to it being used by xc_domain_memory_mapping() (which qemu calls) and the explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend permissions to that of any "ordinary" domctl: A domain controlling the targeted domain can invoke this operation for that target domain (which is being achieved by no longer passing NULL to xsm_domctl()). This at once avoids a for_each_domain() loop when the ID of an existing domain gets passed in. Reported-by: Marek Marczykowski-Górecki Signed-off-by: Jan Beulich --- v2: Add a comment. Clarify description as to what additional permission is being granted. --- I know there had been an alternative patch suggestion, but that one doesn't seem have seen a formal submission so far, so here is my original proposal. I wonder what good the duplication of the returned domain ID does: I'm tempted to remove the one in the command-specific structure. Does anyone have insight into why it was done that way? I further wonder why we have XSM_OTHER: The respective conversion into other XSM_* values in xsm/dummy.h could as well move into the callers, making intentions more obvious when looking at the actual code. --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -149,7 +149,7 @@ define(`device_model', ` create_channel($2, $1, $2_channel) allow $1 $2_channel:event create; - allow $1 $2_target:domain shutdown; + allow $1 $2_target:domain { getdomaininfo shutdown }; allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; ') --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe switch ( op->cmd ) { case XEN_DOMCTL_createdomain: -case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; default: d = rcu_lock_domain_by_id(op->domain); -if ( d == NULL ) +if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) return -ESRCH; } @@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_getdomaininfo: { -domid_t dom = op->domain; - -rcu_read_lock(&domlist_read_lock); +domid_t dom = DOMID_INVALID; -for_each_domain ( d ) -if ( d->domain_id >= dom ) +if ( !d ) +{ +ret = -EINVAL; +if ( op->domain >= DOMID_FIRST_RESERVED ) break; +rcu_read_lock(&domlist_read_lock); + +dom = op->domain; +for_each_domain ( d ) +if ( d->domain_id >= dom ) +break; +} + ret = -ESRCH; if ( d == NULL ) goto getdomaininfo_out; @@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe copyback = 1; getdomaininfo_out: +/* When d was non-NULL upon entry, no cleanup is needed. */ +if ( dom == DOMID_INVALID ) +break; + rcu_read_unlock(&domlist_read_lock); d = NULL; break; --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -61,7 +61,12 @@ static always_inline int xsm_default_act return 0; case XSM_TARGET: if ( src == target ) +{ return 0; +case XSM_XS_PRIV: +if ( src->is_xenstore ) +return 0; +} /* fall through */ case XSM_DM_PRIV: if ( target && src->target == target ) @@ -71,10 +76,6 @@ static always_inline int xsm_default_act if ( src->is_privileged ) return 0; return -EPERM; -case XSM_XS_PRIV: -if ( src->is_xenstore || src->is_privileged ) -return 0; -return -EPERM; default: LINKER_BUG_ON(1); return -EPERM; domctl: relax getdomaininfo permissions Qemu needs access to this for the domain it controls, both due to it being used by xc_domain_memory_mapping() (which qemu calls) and the explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend permissions to that of any "ordinary" domctl: A domain controlling the targeted domain can invoke this operation for that target domain (which is being achieved by no longer passing NULL to xsm_domctl()). This at once avoids a for_each_domain() loop when the ID of an existing domain gets passed in. Reported-by: Marek Marczykowski-Górecki Signed-off-by: Jan Beulich --- v2: Add a comment. Clarify description as to what additional permission is being granted. --- I know there had been an alternative patch suggestion, but that one doesn't seem have seen a formal submission so far, so here is my original proposal. I wonder what good the duplication of the returned d
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
On 05/08/16 12:20, Jan Beulich wrote: > Qemu needs access to this for the domain it controls, both due to it > being used by xc_domain_memory_mapping() (which qemu calls) and the > explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend > permissions to that of any "ordinary" domctl: A domain controlling the > targeted domain can invoke this operation for that target domain (which > is being achieved by no longer passing NULL to xsm_domctl()). > > This at once avoids a for_each_domain() loop when the ID of an > existing domain gets passed in. > > Reported-by: Marek Marczykowski-Górecki > Signed-off-by: Jan Beulich > --- > v2: Add a comment. Clarify description as to what additional permission > is being granted. > --- > I know there had been an alternative patch suggestion, but that one > doesn't seem have seen a formal submission so far, so here is my > original proposal. > > I wonder what good the duplication of the returned domain ID does: I'm > tempted to remove the one in the command-specific structure. Does > anyone have insight into why it was done that way? I wonder whether the first incarnation of this hypercall lacked a domid field in the returned structure? It seems like the kind of thing which would be omitted, until the sysctl list version got introduced. > > I further wonder why we have XSM_OTHER: The respective conversion into > other XSM_* values in xsm/dummy.h could as well move into the callers, > making intentions more obvious when looking at the actual code. > > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -61,7 +61,12 @@ static always_inline int xsm_default_act > return 0; > case XSM_TARGET: > if ( src == target ) > +{ > return 0; > +case XSM_XS_PRIV: > +if ( src->is_xenstore ) > +return 0; > +} > /* fall through */ > case XSM_DM_PRIV: > if ( target && src->target == target ) > @@ -71,10 +76,6 @@ static always_inline int xsm_default_act > if ( src->is_privileged ) > return 0; > return -EPERM; > -case XSM_XS_PRIV: > -if ( src->is_xenstore || src->is_privileged ) > -return 0; > -return -EPERM; > default: > LINKER_BUG_ON(1); > return -EPERM; What is this change in relation to? I can't see how it is related to the XSM changes mentioned in the commit, as that is strictly for the use of XSM_OTHER. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
>>> On 05.08.16 at 15:10, wrote: > On 05/08/16 12:20, Jan Beulich wrote: >> I wonder what good the duplication of the returned domain ID does: I'm >> tempted to remove the one in the command-specific structure. Does >> anyone have insight into why it was done that way? > > I wonder whether the first incarnation of this hypercall lacked a domid > field in the returned structure? It seems like the kind of thing which > would be omitted, until the sysctl list version got introduced. Oh, good point - that makes clear why the field can't be dropped: That sysctl would break then. >> --- a/xen/include/xsm/dummy.h >> +++ b/xen/include/xsm/dummy.h >> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act >> return 0; >> case XSM_TARGET: >> if ( src == target ) >> +{ >> return 0; >> +case XSM_XS_PRIV: >> +if ( src->is_xenstore ) >> +return 0; >> +} >> /* fall through */ >> case XSM_DM_PRIV: >> if ( target && src->target == target ) >> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act >> if ( src->is_privileged ) >> return 0; >> return -EPERM; >> -case XSM_XS_PRIV: >> -if ( src->is_xenstore || src->is_privileged ) >> -return 0; >> -return -EPERM; >> default: >> LINKER_BUG_ON(1); >> return -EPERM; > > What is this change in relation to? I can't see how it is related to > the XSM changes mentioned in the commit, as that is strictly for the use > of XSM_OTHER. I don't see any XSM changes mentioned in the description, there was only the XSM_OTHER related question outside the description. Anyway - the change above is what guarantees the XSM_XS_PRIV check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo case, to fall through into XSM_DM_PRIV - after all that's what the whole patch is about. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
On 05/08/16 14:54, Jan Beulich wrote: On 05.08.16 at 15:10, wrote: >> On 05/08/16 12:20, Jan Beulich wrote: >>> I wonder what good the duplication of the returned domain ID does: I'm >>> tempted to remove the one in the command-specific structure. Does >>> anyone have insight into why it was done that way? >> I wonder whether the first incarnation of this hypercall lacked a domid >> field in the returned structure? It seems like the kind of thing which >> would be omitted, until the sysctl list version got introduced. > Oh, good point - that makes clear why the field can't be dropped: > That sysctl would break then. Which domid were you referring to then? The domid in the xen_domctl_getdomaininfo structure clearly needs to stay, but the domctl "op->domain = op->u.getdomaininfo.domain;" needn't. OTOH, as we need to copy back the entire domctl structure anyway, it doesn't hurt to keep it. > >>> --- a/xen/include/xsm/dummy.h >>> +++ b/xen/include/xsm/dummy.h >>> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act >>> return 0; >>> case XSM_TARGET: >>> if ( src == target ) >>> +{ >>> return 0; >>> +case XSM_XS_PRIV: >>> +if ( src->is_xenstore ) >>> +return 0; >>> +} >>> /* fall through */ >>> case XSM_DM_PRIV: >>> if ( target && src->target == target ) >>> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act >>> if ( src->is_privileged ) >>> return 0; >>> return -EPERM; >>> -case XSM_XS_PRIV: >>> -if ( src->is_xenstore || src->is_privileged ) >>> -return 0; >>> -return -EPERM; >>> default: >>> LINKER_BUG_ON(1); >>> return -EPERM; >> What is this change in relation to? I can't see how it is related to >> the XSM changes mentioned in the commit, as that is strictly for the use >> of XSM_OTHER. > I don't see any XSM changes mentioned in the description, there > was only the XSM_OTHER related question outside the description. > Anyway - the change above is what guarantees the XSM_XS_PRIV > check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo > case, to fall through into XSM_DM_PRIV - after all that's what the > whole patch is about. But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
>>> On 05.08.16 at 19:07, wrote: > On 05/08/16 14:54, Jan Beulich wrote: > On 05.08.16 at 15:10, wrote: >>> On 05/08/16 12:20, Jan Beulich wrote: I wonder what good the duplication of the returned domain ID does: I'm tempted to remove the one in the command-specific structure. Does anyone have insight into why it was done that way? >>> I wonder whether the first incarnation of this hypercall lacked a domid >>> field in the returned structure? It seems like the kind of thing which >>> would be omitted, until the sysctl list version got introduced. >> Oh, good point - that makes clear why the field can't be dropped: >> That sysctl would break then. > > Which domid were you referring to then? > > The domid in the xen_domctl_getdomaininfo structure clearly needs to > stay, but the domctl "op->domain = op->u.getdomaininfo.domain;" > needn't. OTOH, as we need to copy back the entire domctl structure > anyway, it doesn't hurt to keep it. The comment was about removal of the field, not just the assignment. But as you did make obvious, the sysctl side needs it to stay. --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -61,7 +61,12 @@ static always_inline int xsm_default_act return 0; case XSM_TARGET: if ( src == target ) +{ return 0; +case XSM_XS_PRIV: +if ( src->is_xenstore ) +return 0; +} /* fall through */ case XSM_DM_PRIV: if ( target && src->target == target ) @@ -71,10 +76,6 @@ static always_inline int xsm_default_act if ( src->is_privileged ) return 0; return -EPERM; -case XSM_XS_PRIV: -if ( src->is_xenstore || src->is_privileged ) -return 0; -return -EPERM; default: LINKER_BUG_ON(1); return -EPERM; >>> What is this change in relation to? I can't see how it is related to >>> the XSM changes mentioned in the commit, as that is strictly for the use >>> of XSM_OTHER. >> I don't see any XSM changes mentioned in the description, there >> was only the XSM_OTHER related question outside the description. >> Anyway - the change above is what guarantees the XSM_XS_PRIV >> check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo >> case, to fall through into XSM_DM_PRIV - after all that's what the >> whole patch is about. > > But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV. The point of the patch is to _extend_ permissions of this domctl from XS_PRIV to DM_PRIV. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
On 08/08/16 07:12, Jan Beulich wrote: On 05.08.16 at 19:07, wrote: >> On 05/08/16 14:54, Jan Beulich wrote: >> On 05.08.16 at 15:10, wrote: On 05/08/16 12:20, Jan Beulich wrote: > I wonder what good the duplication of the returned domain ID does: I'm > tempted to remove the one in the command-specific structure. Does > anyone have insight into why it was done that way? I wonder whether the first incarnation of this hypercall lacked a domid field in the returned structure? It seems like the kind of thing which would be omitted, until the sysctl list version got introduced. >>> Oh, good point - that makes clear why the field can't be dropped: >>> That sysctl would break then. >> Which domid were you referring to then? >> >> The domid in the xen_domctl_getdomaininfo structure clearly needs to >> stay, but the domctl "op->domain = op->u.getdomaininfo.domain;" >> needn't. OTOH, as we need to copy back the entire domctl structure >> anyway, it doesn't hurt to keep it. > The comment was about removal of the field, not just the > assignment. But as you did make obvious, the sysctl side needs > it to stay. > > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -61,7 +61,12 @@ static always_inline int xsm_default_act > return 0; > case XSM_TARGET: > if ( src == target ) > +{ > return 0; > +case XSM_XS_PRIV: > +if ( src->is_xenstore ) > +return 0; > +} > /* fall through */ > case XSM_DM_PRIV: > if ( target && src->target == target ) > @@ -71,10 +76,6 @@ static always_inline int xsm_default_act > if ( src->is_privileged ) > return 0; > return -EPERM; > -case XSM_XS_PRIV: > -if ( src->is_xenstore || src->is_privileged ) > -return 0; > -return -EPERM; > default: > LINKER_BUG_ON(1); > return -EPERM; What is this change in relation to? I can't see how it is related to the XSM changes mentioned in the commit, as that is strictly for the use of XSM_OTHER. >>> I don't see any XSM changes mentioned in the description, there >>> was only the XSM_OTHER related question outside the description. >>> Anyway - the change above is what guarantees the XSM_XS_PRIV >>> check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo >>> case, to fall through into XSM_DM_PRIV - after all that's what the >>> whole patch is about. >> But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV. > The point of the patch is to _extend_ permissions of this domctl > from XS_PRIV to DM_PRIV. Aah - and this only exists because of the xsm_domctl() bodge with XSM_OTHER, which actually makes getdomaininfo protected with XS_PRIV. Reviewed-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
>>> On 05.08.16 at 13:20, wrote: Daniel, I've only now realized that I forgot to Cc you on this v2. Jan > Qemu needs access to this for the domain it controls, both due to it > being used by xc_domain_memory_mapping() (which qemu calls) and the > explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend > permissions to that of any "ordinary" domctl: A domain controlling the > targeted domain can invoke this operation for that target domain (which > is being achieved by no longer passing NULL to xsm_domctl()). > > This at once avoids a for_each_domain() loop when the ID of an > existing domain gets passed in. > > Reported-by: Marek Marczykowski-Górecki > Signed-off-by: Jan Beulich > --- > v2: Add a comment. Clarify description as to what additional permission > is being granted. > --- > I know there had been an alternative patch suggestion, but that one > doesn't seem have seen a formal submission so far, so here is my > original proposal. > > I wonder what good the duplication of the returned domain ID does: I'm > tempted to remove the one in the command-specific structure. Does > anyone have insight into why it was done that way? > > I further wonder why we have XSM_OTHER: The respective conversion into > other XSM_* values in xsm/dummy.h could as well move into the callers, > making intentions more obvious when looking at the actual code. > > --- a/tools/flask/policy/modules/xen.if > +++ b/tools/flask/policy/modules/xen.if > @@ -149,7 +149,7 @@ define(`device_model', ` > create_channel($2, $1, $2_channel) > allow $1 $2_channel:event create; > > - allow $1 $2_target:domain shutdown; > + allow $1 $2_target:domain { getdomaininfo shutdown }; > allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack > }; > allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl > irqlevel > pciroute pcilevel cacheattr send_irq }; > ') > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > switch ( op->cmd ) > { > case XEN_DOMCTL_createdomain: > -case XEN_DOMCTL_getdomaininfo: > case XEN_DOMCTL_test_assign_device: > case XEN_DOMCTL_gdbsx_guestmemio: > d = NULL; > break; > default: > d = rcu_lock_domain_by_id(op->domain); > -if ( d == NULL ) > +if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) > return -ESRCH; > } > > @@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > > case XEN_DOMCTL_getdomaininfo: > { > -domid_t dom = op->domain; > - > -rcu_read_lock(&domlist_read_lock); > +domid_t dom = DOMID_INVALID; > > -for_each_domain ( d ) > -if ( d->domain_id >= dom ) > +if ( !d ) > +{ > +ret = -EINVAL; > +if ( op->domain >= DOMID_FIRST_RESERVED ) > break; > > +rcu_read_lock(&domlist_read_lock); > + > +dom = op->domain; > +for_each_domain ( d ) > +if ( d->domain_id >= dom ) > +break; > +} > + > ret = -ESRCH; > if ( d == NULL ) > goto getdomaininfo_out; > @@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > copyback = 1; > > getdomaininfo_out: > +/* When d was non-NULL upon entry, no cleanup is needed. */ > +if ( dom == DOMID_INVALID ) > +break; > + > rcu_read_unlock(&domlist_read_lock); > d = NULL; > break; > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -61,7 +61,12 @@ static always_inline int xsm_default_act > return 0; > case XSM_TARGET: > if ( src == target ) > +{ > return 0; > +case XSM_XS_PRIV: > +if ( src->is_xenstore ) > +return 0; > +} > /* fall through */ > case XSM_DM_PRIV: > if ( target && src->target == target ) > @@ -71,10 +76,6 @@ static always_inline int xsm_default_act > if ( src->is_privileged ) > return 0; > return -EPERM; > -case XSM_XS_PRIV: > -if ( src->is_xenstore || src->is_privileged ) > -return 0; > -return -EPERM; > default: > LINKER_BUG_ON(1); > return -EPERM; ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions
On 08/11/2016 07:33 AM, Jan Beulich wrote: On 05.08.16 at 13:20, wrote: Daniel, I've only now realized that I forgot to Cc you on this v2. Jan Qemu needs access to this for the domain it controls, both due to it being used by xc_domain_memory_mapping() (which qemu calls) and the explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend permissions to that of any "ordinary" domctl: A domain controlling the targeted domain can invoke this operation for that target domain (which is being achieved by no longer passing NULL to xsm_domctl()). This at once avoids a for_each_domain() loop when the ID of an existing domain gets passed in. Reported-by: Marek Marczykowski-Górecki Signed-off-by: Jan Beulich Acked-by: Daniel De Graaf [...] I know there had been an alternative patch suggestion, but that one doesn't seem have seen a formal submission so far, so here is my original proposal. I wonder what good the duplication of the returned domain ID does: I'm tempted to remove the one in the command-specific structure. Does anyone have insight into why it was done that way? I further wonder why we have XSM_OTHER: The respective conversion into other XSM_* values in xsm/dummy.h could as well move into the callers, making intentions more obvious when looking at the actual code. The XSM_* values are not actually present in the XSM hook functions, so they have to be a static value per function. Otherwise, the dummy XSM module won't have enough information to make the same decision as the inlined dummy.h version does. An alternate solution would be to add an explicit action parameter to the hooks that currently use XSM_OTHER, but that mostly just moves the conversion switch statement around and adds a pointless computation in the case when the parameter is not used. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel