ui: avoid sign extension using client width/height

qemu with hax to log dma reads & writes jcs.org/2018/11/12/vfio

Pixman returns a signed int for the image width/height, but the VNC
protocol only permits a unsigned int16. Effective framebuffer size
is determined by the guest, limited by the video RAM size, so the
dimensions are unlikely to exceed the range of an unsigned int16,
but this is not currently validated.

With the current use of 'int' for client width/height, the calculation
of offsets in vnc_update_throttle_offset() suffers from integer size
promotion and sign extension, causing coverity warnings

*** CID 1385147: Integer handling issues (SIGN_EXTENSION)
/ui/vnc.c: 979 in vnc_update_throttle_offset()
973 * than that the client would already suffering awful audio
974 * glitches, so dropping samples is no worse really).
975 */
976 static void vnc_update_throttle_offset(VncState *vs)
977 {
978 size_t offset =
>>> CID 1385147: Integer handling issues (SIGN_EXTENSION)
>>> Suspicious implicit sign extension:
"vs->client_pf.bytes_per_pixel" with type "unsigned char" (8 bits,
unsigned) is promoted in "vs->client_width * vs->client_height *
vs->client_pf.bytes_per_pixel" to type "int" (32 bits, signed), then
sign-extended to type "unsigned long" (64 bits, unsigned). If
"vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel"
is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
979 vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;

Change client_width / client_height to be a size_t to avoid sign
extension and integer promotion. Then validate that dimensions are in
range wrt the RFB protocol u16 limits.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-id: 20180118155254.17053-1-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

authored by

Daniel P. Berrange and committed by

Gerd Hoffmann 8 years ago 4c956bd8 834a336e

+11 -2

2 changed files

expand all

vnc.c

vnc.h

ui/vnc.c

··· 672 672 vs->client_height == pixman_image_get_height(vs->vd->server)) { 673 673 return; 674 674 } 675 + 676 + assert(pixman_image_get_width(vs->vd->server) < 65536 && 677 + pixman_image_get_width(vs->vd->server) >= 0); 678 + assert(pixman_image_get_height(vs->vd->server) < 65536 && 679 + pixman_image_get_height(vs->vd->server) >= 0); 675 680 vs->client_width = pixman_image_get_width(vs->vd->server); 676 681 vs->client_height = pixman_image_get_height(vs->vd->server); 677 682 vnc_lock_output(vs); ··· 2490 2495 return 0; 2491 2496 } 2492 2497 2498 + assert(pixman_image_get_width(vs->vd->server) < 65536 && 2499 + pixman_image_get_width(vs->vd->server) >= 0); 2500 + assert(pixman_image_get_height(vs->vd->server) < 65536 && 2501 + pixman_image_get_height(vs->vd->server) >= 0); 2493 2502 vs->client_width = pixman_image_get_width(vs->vd->server); 2494 2503 vs->client_height = pixman_image_get_height(vs->vd->server); 2495 2504 vnc_write_u16(vs, vs->client_width);

+2 -2

ui/vnc.h

··· 278 278 int last_x; 279 279 int last_y; 280 280 uint32_t last_bmask; 281 - int client_width; 282 - int client_height; 281 + size_t client_width; /* limited to u16 by RFB proto */ 282 + size_t client_height; /* limited to u16 by RFB proto */ 283 283 VncShareMode share_mode; 284 284 285 285 uint32_t vnc_encoding;