Do not overflow when calculating region end in lookup#1109
Merged
Conversation
This fixes a relatively boring bug I noticed in following what a particularly evil NVMe PRP could cause us to do: if a guest offers a PRP referencing the last page (i.e. `u64::MAX & !0xfff`) and we try to get a region covering that page, the addition in `region_mappings` could overflow. Yet another win for panic-on-overflow, I wouldn't have noticed this in a release build. I say this is "boring" because there won't be memory mapped up at the end of u64 space, so to be "exciting" there would have to be a very large region being looked up. Something like a length of `u64::MAX - 0x1000` where the overflow lands in the same region and we pass `addr + rlen < end`. Anything else and we'll fail the check and return `None`, just later than we really should have.
hawkw
reviewed
Apr 9, 2026
Member
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
nice catch! i had a couple little nitpicks.
| } | ||
| } | ||
|
|
||
| #[test] |
Member
There was a problem hiding this comment.
this file has a test submodule, shouldn't this be in there?
Member
Author
There was a problem hiding this comment.
i was on the fence about that, usually i think about mod test { ... } as more comprehensive tests for either a composition of items in super or at least pulling in "a bunch" of stuff. this is just "the function directly above this works in these ways" so moving it feels like it's easier to miss the examples?
Member
There was a problem hiding this comment.
🤷♀️ fair enough, normally i feel like i expect to see every test in mod test but i don't actually care about this
hawkw
approved these changes
Apr 9, 2026
papertigers
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Apr 13, 2026
This bumps propolis to commit bc489dd. This brings in the following: - oxidecomputer/propolis#1112 **(the reason for this PR)** - oxidecomputer/propolis#1109 - oxidecomputer/propolis#1084
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a relatively boring bug I noticed in following what a particularly evil NVMe PRP could cause us to do: if a guest offers a PRP referencing the last page (i.e.
u64::MAX & !0xfff) and we try to get a region covering that page, the addition inregion_mappingscould overflow.Yet another win for panic-on-overflow, I wouldn't have noticed this in a release build. I say this is "boring" because there won't be memory mapped up at the end of u64 space, so to be "exciting" there would have to be a very large region being looked up. Something like a length of
u64::MAX - 0x1000where the overflow lands in the same region and we passaddr + rlen < end. Anything else and we'll fail the check and returnNone, just later than we really should have.