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);
+    fbend += (uint64_t) fb->bytes_pp * ss->r.width;
 
     if (fbend > blob_size) {
         qemu_log_mask(LOG_GUEST_ERROR,
-- 
2.39.5

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.

-- 
Best regards,
Dmitry