x86/xen: fix out of bounds access to the event channel masks on resume

[royger]

Summary

When resuming from migration or suspension all regular event channels ports are
reset to the INVALID_EVTCHN value, and drivers should re-initialize them
according to the new value provided by the other end of the connection.

However, the driver would first attempt to unbind the event channel handler
before attempting to bind it using the newly provided port. This unbind uses
the stale event channel port that has been set to INVALID_EVTCHN for some
operations (notably as a result of the handler removal the interrupt subsystem
ends up calling disable intr and source PIC hooks).

This was fine when INVALID_EVTCHN was 0, as then the operation would just
result in pointless setting of the 0 bit in the different event channel related
control arrays (evtchn_{pending,mask} for example). However with the change to
define INVALID_EVTCHN as ~0 the write is no longer pointless, and we end up
triggering a page-fault, or corrupting random data that happens to be mapped at
the array position + ~0 bits.

In hindsight the change of INVALID_EVTCHN from 0 to ~0 was way more risky than
initially assessed, and I believe has end up resulting in more fragile code for
no real benefit.

Fix the disable intr and source wrappers to check whether the event channel is
valid before attempting to use it.

Also introduce some extra KASSERTs in several array accesses in order to avoid
out of bounds accesses if INVALID_EVTCHN ever reaches those functions.

Fixes: 1797ff962769 ('xen/intr: cleanup event channel number use')
MFC after: 1 week
Sponsored by: Cloud Software Group

[markj]

sys/dev/xen/bus/xen_intr.c

169

No newlines in kassert strings.

192

sys/xen/evtchn/evtchnvar.h

42

[ehem_freebsd_m5p.com]

Depending on where xen_intr_disable_source() and xen_intr_disable_intr() were being called from, this is fixing both 1797ff962769 and some other commit. Certainly 1797ff962769 may have exposed this, but the is_valid_evtchn() call was needed earlier (could be rG76acc41fb7c7, or even before).

[royger]

In D43928#1003296, @ehem_freebsd_m5p.com wrote:

Depending on where xen_intr_disable_source() and xen_intr_disable_intr() were being called from, this is fixing both 1797ff962769 and some other commit. Certainly 1797ff962769 may have exposed this, but the is_valid_evtchn() call was needed earlier (could be rG76acc41fb7c7, or even before).

The usefulness of the Fixes tag is also to notice whether something should be backported or not, while we can argue whether this was a latent issue triggered by your change, it didn't cause the OS to malfunction previous to your change.