[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
On Wed, 2014-12-10 at 08:07 +0000, 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

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?


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

Daniel De Graaf
National Security Agency

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.