| Message ID | 20241104165348.2361299-3-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-gpu: coverity fixes | expand |
On 11/4/24 19:53, Alex Bennée wrote: > Coverity reports (CID 1564769, 1564770) that we potentially overflow > by doing some 32x32 multiplies for something that ends up in a 64 bit > value. Fix this by casting the first input to uint64_t to ensure a 64 > bit multiply is used. > > While we are at it note why we split the calculation into stride and > bytes_pp parts. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > hw/display/virtio-gpu.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index e7ca8fd1cf..572e4d92c6 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -741,9 +741,14 @@ bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb, > fb->stride = ss->strides[0]; > fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride; > > + /* > + * We calculate fb->stride for every line but the last which we > + * calculate purely by its width. The stride will often be larger > + * than width to meet alignment requirements. > + */ > fbend = fb->offset; > - fbend += fb->stride * (ss->r.height - 1); > - fbend += fb->bytes_pp * ss->r.width; > + fbend += (uint64_t) fb->stride * (ss->r.height - 1); ss->r.height=0 will result into overflow. I don't see why the last line needs to be treated differently, that's wrong. The last line shall have same stride as other lines, otherwise it may result into OOB reading of the last line depending on the reader implementation. Let's fix it too, all lines should have same stride.
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index e7ca8fd1cf..572e4d92c6 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -741,9 +741,14 @@ bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb, fb->stride = ss->strides[0]; fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride; + /* + * We calculate fb->stride for every line but the last which we + * calculate purely by its width. The stride will often be larger + * than width to meet alignment requirements. + */ fbend = fb->offset; - fbend += fb->stride * (ss->r.height - 1); - fbend += fb->bytes_pp * ss->r.width; + fbend += (uint64_t) fb->stride * (ss->r.height - 1); + fbend += (uint64_t) fb->bytes_pp * ss->r.width; if (fbend > blob_size) { qemu_log_mask(LOG_GUEST_ERROR,
Coverity reports (CID 1564769, 1564770) that we potentially overflow by doing some 32x32 multiplies for something that ends up in a 64 bit value. Fix this by casting the first input to uint64_t to ensure a 64 bit multiply is used. While we are at it note why we split the calculation into stride and bytes_pp parts. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- hw/display/virtio-gpu.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)