Page MenuHomeFreeBSD

sys/x86: check for out of range interrupt allocations
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on Sep 14 2021, 5:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 8:45 PM
Unknown Object (File)
Sat, Mar 2, 2:22 PM
Unknown Object (File)
Sat, Mar 2, 2:43 AM
Unknown Object (File)
Jan 27 2024, 7:37 AM
Unknown Object (File)
Jan 17 2024, 12:07 PM
Unknown Object (File)
Jan 15 2024, 5:56 PM
Unknown Object (File)
Dec 30 2023, 10:55 AM
Unknown Object (File)
Dec 20 2023, 7:52 AM
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Nothing should ever be attempting to allocate an out of range vector
like this, but bugs do happen. Ensure interrupt_sources doesn't
overflow even in presence of bugs.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41509
Build 38398: arc lint + arc unit

Event Timeline

It shouldn't be possible to trigger, but part of D30743 fixes such an overflow. Actually more elegant to detect it at this layer and pass that back.

Actually on a second quick look, this is also an issue here.

The one question is whether it might instead be appropriate to panic() or printf() here.

@imp thoughts on this, or could you suggest reviewers? Perhaps in release builds the code elsewhere is supposed to ensure this doesn't happen, yet the one case it did get rather close. Even if the case remains a KASSERT(), vector being signed is a bug.

I'm unsure whether I've got the right reviewers/subscribers for this...

In theory this should be well-protected by code outside of this function. Yet despite that I found something which with a suitable push might have triggered this. As such I do like having an extra check in the core.

I can see arguments for this being a KASSERT() or panic(). I can also see an argument for including a printf() to flag trouble.

I would probably keep this as a KASSERT(). pic_vector itself returns an int rather than a u_int. vector should match that, so I would say either explicitly check for vector < 0' in the KASSERT, or change pic_vector` and the various implementations of it to return a u_int in addition to changing vector. (Calling it pic_first_irq while at it might be even better)

Making it fully unsigned seems a reasonable approach. How about D32070 then? I still think testing this in regular builds may be worthwhile.