Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On 12/11/2014 06:44 AM, Jan Beulich wrote: On 10.12.14 at 10:53, wrote: On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote: --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_irq_permission: { -unsigned int pirq = op->u.irq_permission.pirq; +unsigned int pirq = op->u.irq_permission.pirq, irq; int allow = op->u.irq_permission.allow_access; if ( pirq >= d->nr_pirqs ) ret = -EINVAL; -else if ( !pirq_access_permitted(current->domain, pirq) || +else if ( !(irq = pirq_access_permitted(current->domain, pirq)) || I find hiding an assignment inside the second condition in a chain of if's to be rather obfuscated. Doing an assignment in a standalone if statement is one thing, this is going to far IMHO. Changed. My main intention was to avoid having to add another "break;". Also, you range check pirq against d->nr_pirqs but then translate it against current->domain, is that correct? No, it's not. As much as xsm_irq_permission(XSM_HOOK, d, pirq, allow) is bogus, yet it's not clear to me whether switching that to use the Xen IRQ number is okay without any other changes. Daniel? Jan At the moment, this XSM hook does not inspect the PIRQ argument, so it is safe to change it to use the Xen IRQ number. The XSM hooks currently only inspect the IRQ at mapping time; with this change (and the prior changes that fixed up the IRQ permissions rangesets), it may make sense to either add a check here or move the check in order to catch the error earlier. -- Daniel De Graaf National Security Agency ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
>>> On 10.12.14 at 10:53, wrote: > On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote: >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe >> >> case XEN_DOMCTL_irq_permission: >> { >> -unsigned int pirq = op->u.irq_permission.pirq; >> +unsigned int pirq = op->u.irq_permission.pirq, irq; >> int allow = op->u.irq_permission.allow_access; >> >> if ( pirq >= d->nr_pirqs ) >> ret = -EINVAL; >> -else if ( !pirq_access_permitted(current->domain, pirq) || >> +else if ( !(irq = pirq_access_permitted(current->domain, pirq)) || > > I find hiding an assignment inside the second condition in a chain of > if's to be rather obfuscated. Doing an assignment in a standalone if > statement is one thing, this is going to far IMHO. Changed. My main intention was to avoid having to add another "break;". > Also, you range check pirq against d->nr_pirqs but then translate it > against current->domain, is that correct? No, it's not. As much as xsm_irq_permission(XSM_HOOK, d, pirq, allow) is bogus, yet it's not clear to me whether switching that to use the Xen IRQ number is okay without any other changes. Daniel? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
>>> On 10.12.14 at 11:19, wrote: > Hi Jan, > > On 10/12/2014 08:07, Jan Beulich wrote: >> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") >> wasn't really consistent in one respect: The granting of access to an >> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both >> domains. In fact it is wrong to assume that a translation is already/ >> still in place at the time access is being granted/revoked. > > With the change to the interface, some part of libxl may misuse > xc_domain_irq_permission. For instance in tools/libxl/libxl_create.c: > > 1178 ret = irq >= 0 ? xc_physdev_map_pirq(CTX->xch, domid, irq, > &irq) > > We get the PIRQ of domain domid in irq. > > 1179: -EOVERFLOW; > 1180 if (!ret) > 1181 ret = xc_domain_irq_permission(CTX->xch, domid, irq, > 1) > > Here, the PIRQ of the current domain should be passed. Fortunately, for > this specific case, the PIRQs are the same. But this is confusing. Agreed, but I'd leave it to the tools maintainers to clean this up. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
Hi Jan, On 10/12/2014 08:07, Jan Beulich wrote: Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") wasn't really consistent in one respect: The granting of access to an IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both domains. In fact it is wrong to assume that a translation is already/ still in place at the time access is being granted/revoked. With the change to the interface, some part of libxl may misuse xc_domain_irq_permission. For instance in tools/libxl/libxl_create.c: 1178 ret = irq >= 0 ? xc_physdev_map_pirq(CTX->xch, domid, irq, &irq) We get the PIRQ of domain domid in irq. 1179: -EOVERFLOW; 1180 if (!ret) 1181 ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1) Here, the PIRQ of the current domain should be passed. Fortunately, for this specific case, the PIRQs are the same. But this is confusing. Signed-off-by: Jan Beulich --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_irq_permission: { -unsigned int pirq = op->u.irq_permission.pirq; +unsigned int pirq = op->u.irq_permission.pirq, irq; int allow = op->u.irq_permission.allow_access; if ( pirq >= d->nr_pirqs ) ret = -EINVAL; -else if ( !pirq_access_permitted(current->domain, pirq) || +else if ( !(irq = pirq_access_permitted(current->domain, pirq)) || xsm_irq_permission(XSM_HOOK, d, pirq, allow) ) As the pirq may not be the same in domain d, the XSM permission is wrong here. In anycase, it looks weird to use pirq here because the guy who defines the policy may not know the PIRQ value. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On Wed, 2014-12-10 at 10:00 +, Jan Beulich wrote: > >>> On 10.12.14 at 10:53, wrote: > > On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote: > >> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") > >> wasn't really consistent in one respect: The granting of access to an > >> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both > >> domains. In fact it is wrong to assume that a translation is already/ > >> still in place at the time access is being granted/revoked. > > > > Specifically you need to do the translation using the mapping of the > > domain doing the granting, not the domain being granted too, correct? > > > > It takes a little bit of thought to figure out which domain to check > > here, it would be worth a sentence or two explaining why this is the > > right one. > > Would > > "What is wanted is to translate the incoming pIRQ to an IRQ for > the invoking domain (as the pIRQ is the only notion the invoking > domain has of the IRQ), and grant the subject domain access to > the resulting IRQ." > > make this more clear without being purely redundant with the code? Yes, thanks. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
>>> On 10.12.14 at 10:53, wrote: > On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote: >> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") >> wasn't really consistent in one respect: The granting of access to an >> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both >> domains. In fact it is wrong to assume that a translation is already/ >> still in place at the time access is being granted/revoked. > > Specifically you need to do the translation using the mapping of the > domain doing the granting, not the domain being granted too, correct? > > It takes a little bit of thought to figure out which domain to check > here, it would be worth a sentence or two explaining why this is the > right one. Would "What is wanted is to translate the incoming pIRQ to an IRQ for the invoking domain (as the pIRQ is the only notion the invoking domain has of the IRQ), and grant the subject domain access to the resulting IRQ." make this more clear without being purely redundant with the code? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote: > Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") > wasn't really consistent in one respect: The granting of access to an > IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both > domains. In fact it is wrong to assume that a translation is already/ > still in place at the time access is being granted/revoked. Specifically you need to do the translation using the mapping of the domain doing the granting, not the domain being granted too, correct? It takes a little bit of thought to figure out which domain to check here, it would be worth a sentence or two explaining why this is the right one. > Signed-off-by: Jan Beulich > > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > > case XEN_DOMCTL_irq_permission: > { > -unsigned int pirq = op->u.irq_permission.pirq; > +unsigned int pirq = op->u.irq_permission.pirq, irq; > int allow = op->u.irq_permission.allow_access; > > if ( pirq >= d->nr_pirqs ) > ret = -EINVAL; > -else if ( !pirq_access_permitted(current->domain, pirq) || > +else if ( !(irq = pirq_access_permitted(current->domain, pirq)) || I find hiding an assignment inside the second condition in a chain of if's to be rather obfuscated. Doing an assignment in a standalone if statement is one thing, this is going to far IMHO. Also, you range check pirq against d->nr_pirqs but then translate it against current->domain, is that correct? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") wasn't really consistent in one respect: The granting of access to an IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both domains. In fact it is wrong to assume that a translation is already/ still in place at the time access is being granted/revoked. Signed-off-by: Jan Beulich --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_irq_permission: { -unsigned int pirq = op->u.irq_permission.pirq; +unsigned int pirq = op->u.irq_permission.pirq, irq; int allow = op->u.irq_permission.allow_access; if ( pirq >= d->nr_pirqs ) ret = -EINVAL; -else if ( !pirq_access_permitted(current->domain, pirq) || +else if ( !(irq = pirq_access_permitted(current->domain, pirq)) || xsm_irq_permission(XSM_HOOK, d, pirq, allow) ) ret = -EPERM; else if ( allow ) -ret = pirq_permit_access(d, pirq); +ret = irq_permit_access(d, irq); else -ret = pirq_deny_access(d, pirq); +ret = irq_deny_access(d, irq); } break; --- a/xen/include/xen/iocap.h +++ b/xen/include/xen/iocap.h @@ -28,22 +28,11 @@ #define irq_access_permitted(d, i) \ rangeset_contains_singleton((d)->irq_caps, i) -#define pirq_permit_access(d, i) ({ \ -struct domain *d__ = (d); \ -int i__ = domain_pirq_to_irq(d__, i); \ -i__ > 0 ? rangeset_add_singleton(d__->irq_caps, i__)\ -: -EINVAL; \ -}) -#define pirq_deny_access(d, i) ({ \ -struct domain *d__ = (d); \ -int i__ = domain_pirq_to_irq(d__, i); \ -i__ > 0 ? rangeset_remove_singleton(d__->irq_caps, i__)\ -: -EINVAL; \ -}) #define pirq_access_permitted(d, i) ({ \ struct domain *d__ = (d); \ -rangeset_contains_singleton(d__->irq_caps, \ -domain_pirq_to_irq(d__, i));\ +int irq__ = domain_pirq_to_irq(d__, i); \ +irq__ > 0 && irq_access_permitted(d__, irq__) \ +? irq__ : 0;\ }) #endif /* __XEN_IOCAP_H__ */ domctl: fix IRQ permission granting/revocation Commit 545607eb3c ("x86: fix various issues with handling guest IRQs") wasn't really consistent in one respect: The granting of access to an IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both domains. In fact it is wrong to assume that a translation is already/ still in place at the time access is being granted/revoked. Signed-off-by: Jan Beulich --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_irq_permission: { -unsigned int pirq = op->u.irq_permission.pirq; +unsigned int pirq = op->u.irq_permission.pirq, irq; int allow = op->u.irq_permission.allow_access; if ( pirq >= d->nr_pirqs ) ret = -EINVAL; -else if ( !pirq_access_permitted(current->domain, pirq) || +else if ( !(irq = pirq_access_permitted(current->domain, pirq)) || xsm_irq_permission(XSM_HOOK, d, pirq, allow) ) ret = -EPERM; else if ( allow ) -ret = pirq_permit_access(d, pirq); +ret = irq_permit_access(d, irq); else -ret = pirq_deny_access(d, pirq); +ret = irq_deny_access(d, irq); } break; --- a/xen/include/xen/iocap.h +++ b/xen/include/xen/iocap.h @@ -28,22 +28,11 @@ #define irq_access_permitted(d, i) \ rangeset_contains_singleton((d)->irq_caps, i) -#define pirq_permit_access(d, i) ({ \ -struct domain *d__ = (d); \ -int i__ = domain_pirq_to_irq(d__, i); \ -i__ > 0 ? rangeset_add_singleton(d__->irq_caps, i__)\ -: -EINVAL; \ -}) -#define pirq_deny_access(d, i) ({ \ -struct domain *d__ = (d); \ -int i__ = domain_pirq_to_irq(d__, i); \ -i__ > 0 ? rangeset_remove_singleton(d__->irq_caps, i__)\ -: -EINVAL; \ -}) #define pirq_access_permitted(d, i) ({ \ struct domain *d__ = (d); \ -rangeset_contains_singleton(d__->irq_caps, \ -domain_pirq_to_irq(d__, i));\ +int irq__ = domain_pirq_to_irq(d__, i); \ +irq__ > 0 && irq_access_permitted(d__, irq__) \ +? irq__ : 0;