xen/intr: rework handling of event channel numbers in xen_intr.c

[ehem_freebsd_m5p.com]

Summary

A series of commits stemming from the interaction between how
indicies are best handled in C, versus the event channel API used by Xen.

One test line was wrong. The result would be an overflow under some
hard to produce conditions (not believed possible for attack), fix this.

The removal from xen_intr_port_to_isrc[] inside xen_intr_release_isrc() was wrong. If ->xi_port was invalid an entry would be incorrectly cleared. Previously this was mitigated by 0 being used as the invalid value for ->xi_port, but this will now manifest due to ~0 being used instead.

I've concluded is_valid_evtchn() was serving to obscure, rather than
usefully isolating anything. As such remove this macro.

There was some inconsistency when clearing xenisrc structures. Now
use ~0 which is guaranteed invalid, instead of 0 which is reserved not
invalid.

Locally this is broken into mini commits, since they're closely
related and were kind of small by themselves. Potentially they could be squashed on commit.

[ehem_freebsd_m5p.com]

Initial verification passed, final verification is in-progress (previous build failed, but the failure suggested a problem elsewhere and this was fine).

This is partially experimenting with Phabricator to confirm how it handles a few commits as a single differential. This group shares a common theme, so they seemed a good candidate for trying this.

As stated on the differential message, Xen event channels are numbered starting from one, whereas all our favored constructs prefer numbers starting with zero. The boundary had gotten a bit corrupt. Notably line 340 where the test matches event channel numbering, but needs to match array indices.

I'm unsure I've drawn the border well, but this is taking a shot at the issue.

[ehem_freebsd_m5p.com]

...and testing failed, ugh. I stick to my point the event channel numbering in xen_intr.c seems a bit mixed. I'm not entirely sure of the best course for this.

[ehem_freebsd_m5p.com]

Further revision. Nuking is_valid_evtchn() instead of spreading its use. Add a note to the one place which matters about the why which seems crucial for that spot.

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

376 ↗

(On Diff #91040)

I need a double-check here. Right now event channel 0 is never used, so this extra test never fails. My belief is if event channel 0 did see service, its structure should follow the standard format and xi_close should be set appropriately rather than handling this as a special case.

437 ↗

(On Diff #91040)

This struck me as a recipe for future trouble. The moment isrc->xi_port is valid, that should always be used with xen_intr_port_to_isrc[].

[ehem_freebsd_m5p.com]

On looking at this, the reasoning behind one condition differs from what I thought. As a result the conditional needs to remain, but fix the condition.

[ehem_freebsd_m5p.com]

Re-diffing to ensure everything gets in. Some bits could potentially end up as part of several commits...

[ehem_freebsd_m5p.com]

I really think the first two commits are needed. Channel 0 being reserved versus invalid really confuses things. It is far easier to use ~0 for invalid ports since that is an automatically invalid port, rather than merely being reserved.

I'm less sure about renaming evtchn_cpu_(un)?mask_port() to xen_intr_*(). I like this as the two functions are in xen_intr.c and this also avoids confusion. This portion seems less important. If you feel that should be separate, I'm up for it. The rename isn't even required for getting the ARM64 version in (meanwhile the main cleanup is needed to ensure everything works).

sys/x86/xen/xen_intr.c

361

I finally figured out what was going on here. This is for EVTCHN_TYPE_PORT which can be marked as to be closed, yet due to not being rebound during suspect can have an invalid port. This is an example of where mixing reserved 0 versus invalid 0 can confuse.

[ehem_freebsd_m5p.com]

sys/x86/xen/xen_intr.c

361

^suspect^suspend
ie during suspend ->xi_port on these will be set invalid and may or may not be reset to a valid value before this function is called.

[ehem_freebsd_m5p.com]

Near-trivial update. Two small bits are missed in what is currently in Phabricator. These aren't urgent, but seem to make later commits clearer.

[ehem_freebsd_m5p.com]

Hmm, another bug showed up while perusing this. Will occur if a system gets suspended during operation and the given isrc doesn't get cleared later.

[ehem_freebsd_m5p.com]

sys/x86/xen/xen_intr.c

367

This is actually a bug. If xi_port is cleared due to a suspend and never reset, then this line will clear an invalid element of xen_intr_port_to_isrc[]. Previously this hadn't caused problems since xi_port was being cleared to 0 (which is never allocated), but with ~0 now being used this will manifest.

[ehem_freebsd_m5p.com]

Splitting off the rename, even though that showed during this task that is worthy of keeping separate.

[ehem_freebsd_m5p.com]

On checking it appears the previous arc diff run missed a portion. Updating to ensure this includes everything.

[ehem_freebsd_m5p.com]

Looks like my commit description was wrong. The failure with the comparison on 315 would result in overflowing the range reserved for Xen in the x86 interrupt table. Worst case, this would result in an overflow by one of the x86 interrupt table. This seems difficult to exploit, but is still a lurking nasty until this is fixed.

A larger concern is the return of intr_register_source() is left unchecked by xen_intr_alloc_isrc() (or xen_arch_intr_alloc_isrc() in later deltas). The Xen interface code assumes the range it allocates will be untouched. This is presently true, but I could readily see someone accidentally violating this.

A real problem with is_valid_evtchn() is all of the places which do the check use the macro. Meanwhile when ->xi_port is cleared, it is cleared to 0 instead of XEN_INVALID_EVTCHN. This mixing confuses things. I'm also quite sure ~0 should be used instead of 0 (since 0 is unused not invalid).

[ehem_freebsd_m5p.com]

Adding a check for the newly spotted issue.

[ehem_freebsd_m5p.com]

Pulling the one commit, it really didn't fit with the rest.

[ehem_freebsd_m5p.com]

One thing keeps coming to mind. Is the check in xen_intr_get_evtchn_from_port() correct? I'm puzzled at why retrieving the handle for channel 0 would be invalid, aside from previously 0 was being used as the invalid value.

[ehem_freebsd_m5p.com]

Another day, another "Hey! In theory I could cause *this* to happen!"

[ehem_freebsd_m5p.com]

Small tweak I had been pondering. Very minor difference.

[ehem_freebsd_m5p.com]

sys/x86/xen/xen_intr.c

350–364

I'm getting concerned about this entire section, due to the reason being pointed to near the end. If a channel is closed before xen_intr_release_isrc() is called (entirely possible for a PORT channel), then the channel number could be reused by Xen before this section is called.

That in turn would cause problems not just for this entire section, but mask handling as well. In particular xen_intr_bind_isrc() seems to assume the channel is masked and bound to vCPU 0.

This would suggest the entire section should be under a if (isrc->xi_port < NR_EVENT_CHANNELS && xen_intr_port_to_isrc[isrc->xi_port] == isrc) { test.

712–722

I'm unsure whether it is worthwhile including the preference for the use of isrc->xi_port over the misc port sources here. I kind of like clearing these out, but at the end of the day it might be best to leave these alone and let them disappear with D30598 (which merges these together and requires using isrc->xi_port) and potentially D31188 (which would nuke the one in xen_intr_bind_isrc()).

Say the word and these hunks will be dropped.

[ehem_freebsd_m5p.com]

As threatened, dropping the preference for isrc->xi_port. Mostly ends up in D30598 anyway.

On review the potential consequences for PORT channels being deallocated before xen_intr_unbind() are more widespread than I had thought. Cover the spot in xen_intr_bind_isrc() effected by this.

The effects on xen_intr_release_isrc() are also larger than I had thought. The effected section is larger.

[ehem_freebsd_m5p.com]

Tiny update, this line seems appropriate here, but also elsewhere.

[ehem_freebsd_m5p.com]

Upon review, the if() needs to be done while locked.

[ehem_freebsd_m5p.com]

Updating Phabricator's view of this.