Page MenuHomeFreeBSD

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

Authored by ehem_freebsd_m5p.com on Jun 11 2021, 11:28 PM.

Details

Summary

A series of three 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.

Use isrc->xi_port whenever possible. This aids identifying duplicate sections, aiding potential merges.

This is actually 3 commits being submitted as one, since they're closely
related and were kind of small by themselves. Potentially they could be squashed on commit.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 41554
Build 38443: arc lint + arc unit

Event Timeline

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.

...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.

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.

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[].

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.

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

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.

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.

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.

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.

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.

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

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

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).

Adding a check for the newly spotted issue.

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

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.