Validate all inputs to wl_shm_pool::create_buffer#4885
Validate all inputs to wl_shm_pool::create_buffer#4885robert-ancell wants to merge 8 commits intomainfrom
Conversation
Replace existing check that could overflow an int32_t.
|
Specifically |
There was a problem hiding this comment.
Pull request overview
This PR hardens Wayland wl_shm_pool handling by adding stricter input validation for create_buffer() and enforcing that SHM pools cannot be resized smaller, helping prevent integer overflow and invalid range accesses in the SHM backing.
Changes:
- Add
size()to SHM pool interfaces and implement it for the RW SHM-backed pool. - Validate
create_buffer()inputs (format, dimensions, stride) before mapping ranges. - Reject
wl_shm_pool::resize()requests that are negative or would shrink the pool.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/server/shm_backing.h | Extends SHM pool interfaces with a size() query to support resize validation. |
| src/server/shm_backing.cpp | Implements ShmBacking::size() and wires it through RWShmBackedPool::size(). |
| src/server/frontend_wayland/shm.cpp | Adds validation for SHM buffer creation and prevents shrinking SHM pools on resize. |
RAOF
left a comment
There was a problem hiding this comment.
While you're here, can I get you to do just one small bit of extra work 🥺
| auto const max_size = std::numeric_limits<int32_t>::max() / pixel_size; | ||
| auto const max_height = max_size / stride; | ||
| if (width > max_size || height > max_height) | ||
| { | ||
| throw wayland::ProtocolError{ | ||
| resource, | ||
| wayland::Shm::Error::invalid_stride, | ||
| "Requested SHM buffer size %dx%d (stride %d) is too large", width, height, stride}; |
There was a problem hiding this comment.
Rather than limiting our buffer size to those representable in int, why don't we do our size calculations in size_t?
Although I guess we still need to check when sizeof(size_t) == sizeof(int) 🤷♀️
There was a problem hiding this comment.
If you use size_t it become less clear when you would have overflowed an int32_t - instead starting with the maximum and reducing it by dividing ensures your upper limits are always representable,
There was a problem hiding this comment.
But our maximum size isn't limited by int32_t; ShmBacking uses size_t everywhere, so we're perfectly capable of representing int32_t::max() * int32_t::max(). (On size_t-is-64-bit platforms, to be fair).
Now, the maximum size that the Wayland frontend is going to resize the backing store to is int32_t::max(), because that size comes over as an int, but that means the ShmBacking is going to catch it (and, if in the incredibly unlikely future Wayland adds a the ability to allocate a >4GB shm pool, this code will still work 😛 )
There was a problem hiding this comment.
That said, part of what was confusing here is the width > max_size leg of the if; that's subsumed in height > max_height and the later stride-is-not-impossible check.
Perhaps remove the width bit?
There was a problem hiding this comment.
Looking at include/core/mir/geometry/forward.h both Size and Stride use int, not size_t:
using Stride = generic::Value<int, StrideTag>;
using Size = generic::Size<int>;Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Replace existing check that could overflow an int32_t.
Check pools only increase size.