| Message ID | 1498585673-23268-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/27/17 7:48 PM >>> >Coverity warns that pirq_dpci unconditionally dereferences a NULL pointer. >This warning appears to be triggered by pirq_dpci() which is a hidden ternary >expression. In reality, it appears that both callers pass a non-NULL pirq >parameter, so the code is ok in practice. > >Rearange the logic to fail-safe, which should quiesce Coverity. > >Clean up bool_t => bool and trailing whitespace for hvm_domain_use_pirq() >while auditing this area. > >No (intended) functional change. > >Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com>
On Tue, Jun 27, 2017 at 06:47:53PM +0100, Andrew Cooper wrote: > Coverity warns that pirq_dpci unconditionally dereferences a NULL pointer. > This warning appears to be triggered by pirq_dpci() which is a hidden ternary > expression. In reality, it appears that both callers pass a non-NULL pirq > parameter, so the code is ok in practice. > > Rearange the logic to fail-safe, which should quiesce Coverity. > > Clean up bool_t => bool and trailing whitespace for hvm_domain_use_pirq() > while auditing this area. > > No (intended) functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> hvm_gsi_eoi obviously passes a non-NULL pirq (there's a check just before calling hvm_pirq_eoi. The same applies to __hvm_dpci_eoi, which is also only called from hvm_dpci_eoi (although there's no explicit check anywhere in that case) that iterates over the list of pirqs bound to specific guest GSI, and having a NULL there would cause havoc. Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Roger.
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 523d089..d9fea2c 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -2530,10 +2530,9 @@ void arch_evtchn_bind_pirq(struct domain *d, int pirq) spin_unlock_irqrestore(&desc->lock, flags); } -bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq) +bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq) { - return is_hvm_domain(d) && pirq && - pirq->arch.hvm.emuirq != IRQ_UNBOUND; + return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND; } static int allocate_pirq(struct domain *d, int index, int pirq, int irq, diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index 25e3fb4..ffeaf70 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -912,7 +912,15 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) static void hvm_pirq_eoi(struct pirq *pirq, const union vioapic_redir_entry *ent) { - struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq); + struct hvm_pirq_dpci *pirq_dpci; + + if ( !pirq ) + { + ASSERT_UNREACHABLE(); + return; + } + + pirq_dpci = pirq_dpci(pirq); /* * No need to get vector lock for timer diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h index 182caa4..abb1f1c 100644 --- a/xen/include/asm-x86/irq.h +++ b/xen/include/asm-x86/irq.h @@ -145,7 +145,7 @@ int get_free_pirqs(struct domain *, unsigned int nr); void free_domain_pirqs(struct domain *d); int map_domain_emuirq_pirq(struct domain *d, int pirq, int irq); int unmap_domain_pirq_emuirq(struct domain *d, int pirq); -bool_t hvm_domain_use_pirq(const struct domain *, const struct pirq *); +bool hvm_domain_use_pirq(const struct domain *, const struct pirq *); /* Reset irq affinities to match the given CPU mask. */ void fixup_irqs(const cpumask_t *mask, bool_t verbose);
Coverity warns that pirq_dpci unconditionally dereferences a NULL pointer. This warning appears to be triggered by pirq_dpci() which is a hidden ternary expression. In reality, it appears that both callers pass a non-NULL pirq parameter, so the code is ok in practice. Rearange the logic to fail-safe, which should quiesce Coverity. Clean up bool_t => bool and trailing whitespace for hvm_domain_use_pirq() while auditing this area. No (intended) functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/irq.c | 5 ++--- xen/drivers/passthrough/io.c | 10 +++++++++- xen/include/asm-x86/irq.h | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-)