On 13/7/26 13:23, Philippe Mathieu-Daudé wrote:
On 6/7/26 14:53, Marc-André Lureau wrote:
A malicious VNC client can send a SetPixelFormat message with shift
values >= 32, causing UB mask computation
(e.g. red_max << red_shift where red_shift is 255). Apparently, this is
not covered by -fwrapv.
Reject color shifts >= bits_per_pixel before computing masks, and cast
the max values to uint32_t to avoid signed integer overflow when a
valid shift (e.g. 24) would set bit 31 of a signed int.
Fixes: 9f64916da20 ("pixman/vnc: use pixman images in vnc.")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3948
Reported-by: huntr bubble
Signed-off-by: Marc-Andre Lureau <[email protected]>
---
ui/vnc.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 559d3954b87..94a38242c56 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2276,18 +2276,25 @@ static void set_pixel_format(VncState *vs, int
bits_per_pixel,
return;
}
+ if (red_shift >= bits_per_pixel ||
+ green_shift >= bits_per_pixel ||
+ blue_shift >= bits_per_pixel) {
+ vnc_client_error(vs);
+ return;
+ }
+
vs->client_pf.rmax = red_max ? red_max : 0xFF;
vs->client_pf.rbits = ctpopl(red_max);
vs->client_pf.rshift = red_shift;
- vs->client_pf.rmask = red_max << red_shift;
+ vs->client_pf.rmask = (uint32_t)red_max << red_shift;
vs->client_pf.gmax = green_max ? green_max : 0xFF;
vs->client_pf.gbits = ctpopl(green_max);
vs->client_pf.gshift = green_shift;
- vs->client_pf.gmask = green_max << green_shift;
+ vs->client_pf.gmask = (uint32_t)green_max << green_shift;
vs->client_pf.bmax = blue_max ? blue_max : 0xFF;
vs->client_pf.bbits = ctpopl(blue_max);
vs->client_pf.bshift = blue_shift;
- vs->client_pf.bmask = blue_max << blue_shift;
+ vs->client_pf.bmask = (uint32_t)blue_max << blue_shift;
vs->client_pf.bits_per_pixel = bits_per_pixel;
vs->client_pf.bytes_per_pixel = bits_per_pixel / 8;
vs->client_pf.depth = bits_per_pixel == 32 ? 24 : bits_per_pixel;
What about instead of the casts (keeping the if(shift) block):
-static void set_pixel_format(VncState *vs, int bits_per_pixel,
- int big_endian_flag, int true_color_flag,
- int red_max, int green_max, int blue_max,
- int red_shift, int green_shift, int
blue_shift)
+static void set_pixel_format(VncState *vs, uint8_t bits_per_pixel,
+ bool big_endian_flag, bool true_color_flag,
+ uint16_t red_max, uint16_t green_max,
uint16_t blue_max,
+ uint8_t red_shift, uint8_t green_shift,
uint8_t blue_shift)
?
Haha this is what the next patch does ;) I got confused by the
(uint32_t) casts, they belong to the next patch.
So with only the if(shift) here:
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
