| Message ID | 6add36f5-93de-dc8e-7c14-dc5ae1c794eb@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | evtchn: recent XSAs follow-on | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 28 September 2020 11:59 > To: xen-devel@lists.xenproject.org > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian > Jackson <iwj@xenproject.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>; Stefano Stabellini > <sstabellini@kernel.org> > Subject: [PATCH 06/12] evtchn: don't bypass unlinking pIRQ when closing port > > There's no other path causing a terminal unlink_pirq_port() to be called > (evtchn_bind_vcpu() relinks it right away) and hence _if_ pirq can > indeed be NULL when closing the port, list corruption would occur when > bypassing the unlink (unless the structure never gets linked again). As > we can't come here after evtchn_destroy() anymore, (late) domain > destruction also isn't a reason for a possible exception, and hence the > only alternative looks to be that the check was pointless in the first > place. While I haven't observed the case, from code inspection I'm far > from sure I can exclude this being possible, so it feels more safe to > re-arrange the code instead. > > Fixes: c24536b636f2 ("replace d->nr_pirqs sized arrays with radix tree") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Paul Durrant <paul@xen.org> > > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -615,17 +615,18 @@ int evtchn_close(struct domain *d1, int > case ECS_PIRQ: { > struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq); > > - if ( !pirq ) > - break; > - if ( !is_hvm_domain(d1) ) > - pirq_guest_unbind(d1, pirq); > - pirq->evtchn = 0; > - pirq_cleanup_check(pirq, d1); > - unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]); > + if ( pirq ) > + { > + if ( !is_hvm_domain(d1) ) > + pirq_guest_unbind(d1, pirq); > + pirq->evtchn = 0; > + pirq_cleanup_check(pirq, d1); > #ifdef CONFIG_X86 > - if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 ) > - unmap_domain_pirq_emuirq(d1, pirq->pirq); > + if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 ) > + unmap_domain_pirq_emuirq(d1, pirq->pirq); > #endif > + } > + unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]); > break; > } > >
--- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -615,17 +615,18 @@ int evtchn_close(struct domain *d1, int case ECS_PIRQ: { struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq); - if ( !pirq ) - break; - if ( !is_hvm_domain(d1) ) - pirq_guest_unbind(d1, pirq); - pirq->evtchn = 0; - pirq_cleanup_check(pirq, d1); - unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]); + if ( pirq ) + { + if ( !is_hvm_domain(d1) ) + pirq_guest_unbind(d1, pirq); + pirq->evtchn = 0; + pirq_cleanup_check(pirq, d1); #ifdef CONFIG_X86 - if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 ) - unmap_domain_pirq_emuirq(d1, pirq->pirq); + if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 ) + unmap_domain_pirq_emuirq(d1, pirq->pirq); #endif + } + unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]); break; }
There's no other path causing a terminal unlink_pirq_port() to be called (evtchn_bind_vcpu() relinks it right away) and hence _if_ pirq can indeed be NULL when closing the port, list corruption would occur when bypassing the unlink (unless the structure never gets linked again). As we can't come here after evtchn_destroy() anymore, (late) domain destruction also isn't a reason for a possible exception, and hence the only alternative looks to be that the check was pointless in the first place. While I haven't observed the case, from code inspection I'm far from sure I can exclude this being possible, so it feels more safe to re-arrange the code instead. Fixes: c24536b636f2 ("replace d->nr_pirqs sized arrays with radix tree") Signed-off-by: Jan Beulich <jbeulich@suse.com>