xen/arch/x86/irq.c | 31 +++++++----------------------- xen/arch/x86/physdev.c | 43 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 25 deletions(-)
Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
complete_domain_destroy as an RCU callback. The source context was an
unexpected, random domain. Since this is a xen-internal operation,
going through the XSM hook is inapproriate.
Move the XSM hook up into physdev_unmap_pirq, which is the
guest-accessible path. This requires moving some of the sanity check
upwards as well since the hook needs the additional data to make its
decision. Since complete_domain_destroy still calls unmap_domain_pirq,
replace the moved runtime checking with assert. Only valid pirqs should
make their way into unmap_domain_pirq from complete_domain_destroy.
This is mostly code movement, but one style change is to pull `irq =
info->arch.irq` out of the if condition.
Label done is now unused and removed.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going
through map_domain_pirq, and I don't think the vpci code is directly
guest-accessible. So I think those are okay, but I not familiar with
that code. Hence, I am highlighting it.
xen/arch/x86/irq.c | 31 +++++++-----------------------
xen/arch/x86/physdev.c | 43 +++++++++++++++++++++++++++++++++++++++++-
2 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 285ac399fb..ddd3194fba 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2310,41 +2310,25 @@ int unmap_domain_pirq(struct domain *d, int pirq)
struct pirq *info;
struct msi_desc *msi_desc = NULL;
- if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
- return -EINVAL;
-
+ ASSERT(pirq >= 0);
+ ASSERT(pirq < d->nr_pirqs);
ASSERT(pcidevs_locked());
ASSERT(spin_is_locked(&d->event_lock));
info = pirq_info(d, pirq);
- if ( !info || (irq = info->arch.irq) <= 0 )
- {
- dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
- d->domain_id, pirq);
- ret = -EINVAL;
- goto done;
- }
+ ASSERT(info);
+
+ irq = info->arch.irq;
+ ASSERT(irq > 0);
desc = irq_to_desc(irq);
msi_desc = desc->msi_desc;
if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
{
- if ( msi_desc->msi_attrib.entry_nr )
- {
- printk(XENLOG_G_ERR
- "dom%d: trying to unmap secondary MSI pirq %d\n",
- d->domain_id, pirq);
- ret = -EBUSY;
- goto done;
- }
+ ASSERT(msi_desc->msi_attrib.entry_nr == 0);
nr = msi_desc->msi.nvec;
}
- ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
- msi_desc ? msi_desc->dev : NULL);
- if ( ret )
- goto done;
-
forced_unbind = pirq_guest_force_unbind(d, info);
if ( forced_unbind )
dprintk(XENLOG_G_WARNING, "dom%d: forcing unbind of pirq %d\n",
@@ -2405,7 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
if (msi_desc)
msi_free_irq(msi_desc);
- done:
return ret;
}
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 2ddcf44f33..a5ed257dca 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -140,8 +140,11 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
int physdev_unmap_pirq(domid_t domid, int pirq)
{
+ struct msi_desc *msi_desc;
+ struct irq_desc *desc;
+ struct pirq *info;
struct domain *d;
- int ret = 0;
+ int irq, ret = 0;
d = rcu_lock_domain_by_any_id(domid);
if ( d == NULL )
@@ -162,9 +165,47 @@ int physdev_unmap_pirq(domid_t domid, int pirq)
goto free_domain;
}
+ if ( (pirq < 0) || (pirq >= d->nr_pirqs) ) {
+ ret = -EINVAL;
+ goto free_domain;
+ }
+
pcidevs_lock();
spin_lock(&d->event_lock);
+
+ info = pirq_info(d, pirq);
+ irq = info ? info->arch.irq : 0;
+ if ( !info || irq <= 0 )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
+ d->domain_id, pirq);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ desc = irq_to_desc(irq);
+ msi_desc = desc->msi_desc;
+ if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
+ {
+ if ( msi_desc->msi_attrib.entry_nr )
+ {
+ printk(XENLOG_G_ERR
+ "dom%d: trying to unmap secondary MSI pirq %d\n",
+ d->domain_id, pirq);
+ ret = -EBUSY;
+ goto unlock;
+ }
+ }
+
+ ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
+ msi_desc ? msi_desc->dev : NULL);
+ if ( ret )
+ goto unlock;
+
ret = unmap_domain_pirq(d, pirq);
+
+ unlock:
+
spin_unlock(&d->event_lock);
pcidevs_unlock();
--
2.35.1On 3/25/22 10:18, Jason Andryuk wrote:
> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
>
> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
> complete_domain_destroy as an RCU callback. The source context was an
> unexpected, random domain. Since this is a xen-internal operation,
> going through the XSM hook is inapproriate.
To me this is the first problem that should be addressed. Why is current
pointing at a random domain and is it possible to ensure it gets
correctly set to the current domain, e.g. DOMID_IDLE or DOMID_XEN.
> Move the XSM hook up into physdev_unmap_pirq, which is the
> guest-accessible path. This requires moving some of the sanity check
> upwards as well since the hook needs the additional data to make its
> decision. Since complete_domain_destroy still calls unmap_domain_pirq,
> replace the moved runtime checking with assert. Only valid pirqs should
> make their way into unmap_domain_pirq from complete_domain_destroy.
This is moving the time of check a way from the time of use. While today
it may be safe because it only gives guest access through a limited
interface, vpci_msi_disabled, this now introduces risk through the
assumption that no future code will make this call without making the
appropriate XSM call when coming processing a guest request. This is one
of many reasons why I would dissuade moving resource access checks away
from the resource access.
While it is related, in this thread I will not disagree with your point
that XSM calls on internal calls has no meaning at this point. Still we
should not weaken the protection, is there any other way we can
determine the call is being made internally, like I suggested above in
getting `current` set to a system domain and then update fix the default
policy to allow system domains to make the call and those using flask
just update their policy to allow system domains to make the call.
> This is mostly code movement, but one style change is to pull `irq =
> info->arch.irq` out of the if condition.
>
> Label done is now unused and removed.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
> vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going
> through map_domain_pirq, and I don't think the vpci code is directly
> guest-accessible. So I think those are okay, but I not familiar with
> that code. Hence, I am highlighting it.
>
> xen/arch/x86/irq.c | 31 +++++++-----------------------
> xen/arch/x86/physdev.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 285ac399fb..ddd3194fba 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2310,41 +2310,25 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> struct pirq *info;
> struct msi_desc *msi_desc = NULL;
>
> - if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
> - return -EINVAL;
> -
> + ASSERT(pirq >= 0);
> + ASSERT(pirq < d->nr_pirqs);
> ASSERT(pcidevs_locked());
> ASSERT(spin_is_locked(&d->event_lock));
>
> info = pirq_info(d, pirq);
> - if ( !info || (irq = info->arch.irq) <= 0 )
> - {
> - dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
> - d->domain_id, pirq);
> - ret = -EINVAL;
> - goto done;
> - }
> + ASSERT(info);
> +
> + irq = info->arch.irq;
> + ASSERT(irq > 0);
>
> desc = irq_to_desc(irq);
> msi_desc = desc->msi_desc;
> if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
> {
> - if ( msi_desc->msi_attrib.entry_nr )
> - {
> - printk(XENLOG_G_ERR
> - "dom%d: trying to unmap secondary MSI pirq %d\n",
> - d->domain_id, pirq);
> - ret = -EBUSY;
> - goto done;
> - }
> + ASSERT(msi_desc->msi_attrib.entry_nr == 0);
> nr = msi_desc->msi.nvec;
> }
>
> - ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> - msi_desc ? msi_desc->dev : NULL);
> - if ( ret )
> - goto done;
> -
> forced_unbind = pirq_guest_force_unbind(d, info);
> if ( forced_unbind )
> dprintk(XENLOG_G_WARNING, "dom%d: forcing unbind of pirq %d\n",
> @@ -2405,7 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> if (msi_desc)
> msi_free_irq(msi_desc);
>
> - done:
> return ret;
> }
>
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 2ddcf44f33..a5ed257dca 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -140,8 +140,11 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
>
> int physdev_unmap_pirq(domid_t domid, int pirq)
> {
> + struct msi_desc *msi_desc;
> + struct irq_desc *desc;
> + struct pirq *info;
> struct domain *d;
> - int ret = 0;
> + int irq, ret = 0;
>
> d = rcu_lock_domain_by_any_id(domid);
> if ( d == NULL )
> @@ -162,9 +165,47 @@ int physdev_unmap_pirq(domid_t domid, int pirq)
> goto free_domain;
> }
>
> + if ( (pirq < 0) || (pirq >= d->nr_pirqs) ) {
> + ret = -EINVAL;
> + goto free_domain;
> + }
> +
> pcidevs_lock();
> spin_lock(&d->event_lock);
> +
> + info = pirq_info(d, pirq);
> + irq = info ? info->arch.irq : 0;
> + if ( !info || irq <= 0 )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
> + d->domain_id, pirq);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + desc = irq_to_desc(irq);
> + msi_desc = desc->msi_desc;
> + if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
> + {
> + if ( msi_desc->msi_attrib.entry_nr )
> + {
> + printk(XENLOG_G_ERR
> + "dom%d: trying to unmap secondary MSI pirq %d\n",
> + d->domain_id, pirq);
> + ret = -EBUSY;
> + goto unlock;
> + }
> + }
> +
> + ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> + msi_desc ? msi_desc->dev : NULL);
> + if ( ret )
> + goto unlock;
> +
> ret = unmap_domain_pirq(d, pirq);
> +
> + unlock:
> +
> spin_unlock(&d->event_lock);
> pcidevs_unlock();
>
On Fri, Mar 25, 2022 at 10:18:26AM -0400, Jason Andryuk wrote:
> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
>
> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
> complete_domain_destroy as an RCU callback. The source context was an
> unexpected, random domain. Since this is a xen-internal operation,
> going through the XSM hook is inapproriate.
>
> Move the XSM hook up into physdev_unmap_pirq, which is the
> guest-accessible path. This requires moving some of the sanity check
> upwards as well since the hook needs the additional data to make its
> decision. Since complete_domain_destroy still calls unmap_domain_pirq,
> replace the moved runtime checking with assert. Only valid pirqs should
> make their way into unmap_domain_pirq from complete_domain_destroy.
>
> This is mostly code movement, but one style change is to pull `irq =
> info->arch.irq` out of the if condition.
>
> Label done is now unused and removed.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
> vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going
> through map_domain_pirq, and I don't think the vpci code is directly
> guest-accessible. So I think those are okay, but I not familiar with
> that code. Hence, I am highlighting it.
Hm, for vpci_msi_disable it's not technically correct to drop the XSM
check. This is a guests accessible path, however the value of PIRQ is
not settable by the guest, so it's kind of OK just for that reason.
vioapic_hwdom_map_gsi is just for the hardware domain, so likely also
OK.
> xen/arch/x86/irq.c | 31 +++++++-----------------------
> xen/arch/x86/physdev.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 285ac399fb..ddd3194fba 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2310,41 +2310,25 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> struct pirq *info;
> struct msi_desc *msi_desc = NULL;
>
> - if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
> - return -EINVAL;
> -
> + ASSERT(pirq >= 0);
> + ASSERT(pirq < d->nr_pirqs);
> ASSERT(pcidevs_locked());
> ASSERT(spin_is_locked(&d->event_lock));
>
> info = pirq_info(d, pirq);
> - if ( !info || (irq = info->arch.irq) <= 0 )
> - {
> - dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
> - d->domain_id, pirq);
> - ret = -EINVAL;
> - goto done;
> - }
> + ASSERT(info);
> +
> + irq = info->arch.irq;
> + ASSERT(irq > 0);
>
> desc = irq_to_desc(irq);
> msi_desc = desc->msi_desc;
> if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
> {
> - if ( msi_desc->msi_attrib.entry_nr )
> - {
> - printk(XENLOG_G_ERR
> - "dom%d: trying to unmap secondary MSI pirq %d\n",
> - d->domain_id, pirq);
> - ret = -EBUSY;
> - goto done;
> - }
> + ASSERT(msi_desc->msi_attrib.entry_nr == 0);
> nr = msi_desc->msi.nvec;
> }
>
> - ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> - msi_desc ? msi_desc->dev : NULL);
Have you considered performing the check only if !d->is_dying?
Thanks, Roger.
On 28.03.2022 13:40, Roger Pau Monné wrote: > On Fri, Mar 25, 2022 at 10:18:26AM -0400, Jason Andryuk wrote: >> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq. >> >> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from >> complete_domain_destroy as an RCU callback. The source context was an >> unexpected, random domain. Since this is a xen-internal operation, >> going through the XSM hook is inapproriate. >> >> Move the XSM hook up into physdev_unmap_pirq, which is the >> guest-accessible path. This requires moving some of the sanity check >> upwards as well since the hook needs the additional data to make its >> decision. Since complete_domain_destroy still calls unmap_domain_pirq, >> replace the moved runtime checking with assert. Only valid pirqs should >> make their way into unmap_domain_pirq from complete_domain_destroy. >> >> This is mostly code movement, but one style change is to pull `irq = >> info->arch.irq` out of the if condition. And why is this better? You now have an extra conditional expression there. >> Label done is now unused and removed. >> >> Signed-off-by: Jason Andryuk <jandryuk@gmail.com> >> --- >> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and >> vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going >> through map_domain_pirq, and I don't think the vpci code is directly >> guest-accessible. So I think those are okay, but I not familiar with >> that code. Hence, I am highlighting it. > > Hm, for vpci_msi_disable it's not technically correct to drop the XSM > check. This is a guests accessible path, however the value of PIRQ is > not settable by the guest, so it's kind of OK just for that reason. Kind of - perhaps. But better to avoid if possible. Hence I would prefer the ->is_dying alternative you suggest at the end. Jan
On Mon, Mar 28, 2022 at 8:44 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.03.2022 13:40, Roger Pau Monné wrote:
> > On Fri, Mar 25, 2022 at 10:18:26AM -0400, Jason Andryuk wrote:
> >> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
> >>
> >> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
> >> complete_domain_destroy as an RCU callback. The source context was an
> >> unexpected, random domain. Since this is a xen-internal operation,
> >> going through the XSM hook is inapproriate.
> >>
> >> Move the XSM hook up into physdev_unmap_pirq, which is the
> >> guest-accessible path. This requires moving some of the sanity check
> >> upwards as well since the hook needs the additional data to make its
> >> decision. Since complete_domain_destroy still calls unmap_domain_pirq,
> >> replace the moved runtime checking with assert. Only valid pirqs should
> >> make their way into unmap_domain_pirq from complete_domain_destroy.
> >>
> >> This is mostly code movement, but one style change is to pull `irq =
> >> info->arch.irq` out of the if condition.
>
> And why is this better? You now have an extra conditional expression
> there.
It's better because it is clearer. The location is more readily
visible when scanning the code. It fits the pattern of `variable =
something`. Assigning inside the if condition makes it harder to see
where a variable is assigned, which is why I made the change.
This is the non-movement diff:
info = pirq_info(d, pirq);
- if ( !info || (irq = info->arch.irq) <= 0 )
+ irq = info ? info->arch.irq : 0;
+ if ( !info || irq <= 0 )
{
But given comments below, it seems this movement is not going to
happen, so this change will be dropped.
> >> Label done is now unused and removed.
> >>
> >> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> >> ---
> >> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
> >> vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going
> >> through map_domain_pirq, and I don't think the vpci code is directly
> >> guest-accessible. So I think those are okay, but I not familiar with
> >> that code. Hence, I am highlighting it.
> >
> > Hm, for vpci_msi_disable it's not technically correct to drop the XSM
> > check. This is a guests accessible path, however the value of PIRQ is
> > not settable by the guest, so it's kind of OK just for that reason.
Right, that's why I was figuring it was okay. If Xen is handling it
internally, it doesn't have to worry about untrusted data.
> Kind of - perhaps. But better to avoid if possible.
But I can also see how it is safer to leave the check in place. It's
better to go through an unnecessary XSM check than to not have a check
at all.
> Hence I would prefer
> the ->is_dying alternative you suggest at the end.
I had not considered the ->is_dying option. At first glance, it seems
like it would work.
I was hoping that we could determine that only sanitized data would
make it into unmap_domain_pirq. Then we can restructure the code
instead of adding a condition to skip the XSM hook. But if this
function is guest accessible via vpci, then the XSM check should
remain.
Regards,
Jason
© 2016 - 2024 Red Hat, Inc.