Repository navigation
virtio: reject regions whose address and length overflow - #4145
Conversation
try_build_gpn_list computed addr + len on caller-supplied region descriptors without checking for overflow, panicking in debug builds and wrapping to a bogus gpn range in release. Use checked_add and treat an overflowing region as invalid, which is how the function already reports every other malformed input.
There was a problem hiding this comment.
Pull request overview
This PR hardens virtio region handling by rejecting data regions whose addr + len would overflow u64, preventing debug-build panics and release-build miscomputation of the resulting GPN range.
Changes:
- Use
checked_addwhen computing the end address for each region and treat overflow as invalid input (None). - Recompute
last_gpnandprev_endfrom the checked end value to avoid wrapped/stale state. - Add unit tests covering a single wrapping region and a wrapping region followed by another region.
| prev_end = Some(end); | ||
| total_len += len; | ||
| } |
|
Is this something you hit coming from an actual device? Wouldn't such a range be invalid for the device to use anyways? John Starks (@jstarks) what does virtio-villain say about this? |
|
Anatol Belski (@weltling), it might be good to add some (more?) GPA overflow cases to virtio-villain. It looks like this case would have hit virtio-blk and virtio-vsock. |
|
Thanks for the contribution. |
Thanks for the ping, John Starks (@jstarks). I added these in https://github.com/weltling/virtio-villain/releases/tag/v0.8.1. The block cases run on OpenVMM, but the vsock cases skip. The runner has no vsock device on the bus. Passing Two questions. Are there other spots that build a paged range the same way, that would be worth a case as well? And would it be ok to add Thanks |
try_build_gpn_listcomputesaddr + lenon region descriptors without checking for overflow. A region whose address and length wrap pastu64::MAXpanics in debug builds and, with overflow checks off, wraps to a bogus gpn range in release -first_gpnends up abovelast_gpn, the range iterates zero times, andprev_endis left holding a wrapped valuethis uses
checked_addand treats an overflowing region as invalid, which is how the function already reports every other malformed input, so callers keep falling back to the bounce buffer path they use whenever this returnsNoneadded two tests: a single wrapping region, and a wrapping region followed by a well-formed one, which also covers the stale
prev_end. Both fail before the change withattempt to add with overflow