qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] virtio-gpu: coverity fixes
@ 2024-11-04 16:53 Alex Bennée
  2024-11-04 16:53 ` [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion Alex Bennée
  2024-11-04 16:53 ` [PATCH 2/2] hw/display: check frame buffer can hold blob Alex Bennée
  0 siblings, 2 replies; 6+ messages in thread
From: Alex Bennée @ 2024-11-04 16:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S. Tsirkin, Alex Bennée

Hi Dimitry,

I'd already started this before I saw your email so I thought I might
as well push what I had. Feel free to review/use as a base/ignore as
you wish ;-)

Alex.

Alex Bennée (2):
  hw/display: factor out the scanout blob to fb conversion
  hw/display: check frame buffer can hold blob

 include/hw/virtio/virtio-gpu.h | 15 ++++++++
 hw/display/virtio-gpu-virgl.c  | 21 +----------
 hw/display/virtio-gpu.c        | 65 ++++++++++++++++++++++------------
 3 files changed, 58 insertions(+), 43 deletions(-)

-- 
2.39.5



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion
  2024-11-04 16:53 [PATCH 0/2] virtio-gpu: coverity fixes Alex Bennée
@ 2024-11-04 16:53 ` Alex Bennée
  2024-11-06  0:33   ` Dmitry Osipenko
  2024-11-04 16:53 ` [PATCH 2/2] hw/display: check frame buffer can hold blob Alex Bennée
  1 sibling, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2024-11-04 16:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S. Tsirkin, Alex Bennée, Dmitry Osipenko

There are two identical sequences of a code doing the same thing that
raise warnings with Coverity. Before fixing those issues lets factor
out the common code into a helper function we can share.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 include/hw/virtio/virtio-gpu.h | 15 +++++++++
 hw/display/virtio-gpu-virgl.c  | 21 +-----------
 hw/display/virtio-gpu.c        | 60 +++++++++++++++++++++-------------
 3 files changed, 53 insertions(+), 43 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 553799b8cc..90e4abe788 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -333,6 +333,21 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
                                    struct virtio_gpu_scanout *s,
                                    uint32_t resource_id);
 
+/**
+ * virtio_gpu_scanout_blob_to_fb() - fill out fb based on scanout data
+ * fb: the frame-buffer descriptor to fill out
+ * ss: the scanout blob data
+ * blob_size: the maximum size the blob can accommodate
+ *
+ * This will check we have enough space for the frame taking into
+ * account that stride for all but the last line.
+ *
+ * Returns true on success, otherwise logs guest error and returns false
+ */
+bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
+                                   struct virtio_gpu_set_scanout_blob *ss,
+                                   uint64_t blob_size);
+
 /* virtio-gpu-udmabuf.c */
 bool virtio_gpu_have_udmabuf(void);
 void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index eedae7357f..35599cddab 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -852,26 +852,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
         return;
     }
 
-    fb.format = virtio_gpu_get_pixman_format(ss.format);
-    if (!fb.format) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %d\n",
-                      __func__, ss.format);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        return;
-    }
-
-    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
-    fb.width = ss.width;
-    fb.height = ss.height;
-    fb.stride = ss.strides[0];
-    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
-
-    fbend = fb.offset;
-    fbend += fb.stride * (ss.r.height - 1);
-    fbend += fb.bytes_pp * ss.r.width;
-    if (fbend > res->base.blob_size) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: fb end out of range\n",
-                      __func__);
+    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
         return;
     }
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index c0570ef856..e7ca8fd1cf 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -721,13 +721,48 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
                               &fb, res, &ss.r, &cmd->error);
 }
 
+bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
+                                   struct virtio_gpu_set_scanout_blob *ss,
+                                   uint64_t blob_size)
+{
+    uint64_t fbend;
+
+    fb->format = virtio_gpu_get_pixman_format(ss->format);
+    if (!fb->format) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: host couldn't handle guest format %d\n",
+                      __func__, ss->format);
+        return false;
+    }
+
+    fb->bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb->format), 8);
+    fb->width = ss->width;
+    fb->height = ss->height;
+    fb->stride = ss->strides[0];
+    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
+
+    fbend = fb->offset;
+    fbend += fb->stride * (ss->r.height - 1);
+    fbend += fb->bytes_pp * ss->r.width;
+
+    if (fbend > blob_size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: fb end out of range\n",
+                      __func__);
+        return false;
+    }
+
+    return true;
+}
+
+
+
 static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
                                         struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_simple_resource *res;
     struct virtio_gpu_framebuffer fb = { 0 };
     struct virtio_gpu_set_scanout_blob ss;
-    uint64_t fbend;
 
     VIRTIO_GPU_FILL_CMD(ss);
     virtio_gpu_scanout_blob_bswap(&ss);
@@ -753,28 +788,7 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
         return;
     }
 
-    fb.format = virtio_gpu_get_pixman_format(ss.format);
-    if (!fb.format) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: host couldn't handle guest format %d\n",
-                      __func__, ss.format);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        return;
-    }
-
-    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
-    fb.width = ss.width;
-    fb.height = ss.height;
-    fb.stride = ss.strides[0];
-    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
-
-    fbend = fb.offset;
-    fbend += fb.stride * (ss.r.height - 1);
-    fbend += fb.bytes_pp * ss.r.width;
-    if (fbend > res->blob_size) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: fb end out of range\n",
-                      __func__);
+    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
         return;
     }
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] hw/display: check frame buffer can hold blob
  2024-11-04 16:53 [PATCH 0/2] virtio-gpu: coverity fixes Alex Bennée
  2024-11-04 16:53 ` [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion Alex Bennée
@ 2024-11-04 16:53 ` Alex Bennée
  2024-11-06  0:56   ` Dmitry Osipenko
  1 sibling, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2024-11-04 16:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S. Tsirkin, Alex Bennée, Dmitry Osipenko

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion
  2024-11-04 16:53 ` [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion Alex Bennée
@ 2024-11-06  0:33   ` Dmitry Osipenko
  2024-11-06 17:36     ` Alex Bennée
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Osipenko @ 2024-11-06  0:33 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel; +Cc: Michael S. Tsirkin

On 11/4/24 19:53, Alex Bennée wrote:
> There are two identical sequences of a code doing the same thing that
> raise warnings with Coverity. Before fixing those issues lets factor
> out the common code into a helper function we can share.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  include/hw/virtio/virtio-gpu.h | 15 +++++++++
>  hw/display/virtio-gpu-virgl.c  | 21 +-----------
>  hw/display/virtio-gpu.c        | 60 +++++++++++++++++++++-------------
>  3 files changed, 53 insertions(+), 43 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 553799b8cc..90e4abe788 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -333,6 +333,21 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>                                     struct virtio_gpu_scanout *s,
>                                     uint32_t resource_id);
>  
> +/**
> + * virtio_gpu_scanout_blob_to_fb() - fill out fb based on scanout data
> + * fb: the frame-buffer descriptor to fill out
> + * ss: the scanout blob data
> + * blob_size: the maximum size the blob can accommodate

Nit: 'maximum size the blob can accommodate' makes it sound to me like
data will be copied into the blob. What about 'size of scanout blob data'.

> + *
> + * This will check we have enough space for the frame taking into
> + * account that stride for all but the last line.
> + *
> + * Returns true on success, otherwise logs guest error and returns false
> + */
> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
> +                                   struct virtio_gpu_set_scanout_blob *ss,
> +                                   uint64_t blob_size);
> +
>  /* virtio-gpu-udmabuf.c */
>  bool virtio_gpu_have_udmabuf(void);
>  void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index eedae7357f..35599cddab 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -852,26 +852,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>          return;
>      }
>  
> -    fb.format = virtio_gpu_get_pixman_format(ss.format);
> -    if (!fb.format) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %d\n",
> -                      __func__, ss.format);
> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> -        return;
> -    }
> -
> -    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
> -    fb.width = ss.width;
> -    fb.height = ss.height;
> -    fb.stride = ss.strides[0];
> -    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
> -
> -    fbend = fb.offset;
> -    fbend += fb.stride * (ss.r.height - 1);
> -    fbend += fb.bytes_pp * ss.r.width;
> -    if (fbend > res->base.blob_size) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: fb end out of range\n",
> -                      __func__);
> +    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {

This fails to compile, needs s/res->blob_size/res->base.blob_size/

../hw/display/virtio-gpu-virgl.c:855:53: error: 'struct
virtio_gpu_virgl_resource' has no member named 'blob_size'
  855 |     if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
      |                                                     ^~
../hw/display/virtio-gpu-virgl.c:808:14: error: unused variable 'fbend'
[-Werror=unused-variable]
  808 |     uint64_t fbend;
      |              ^~~~~
cc1: all warnings being treated as errors

Please correct in v2.

>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>          return;
>      }
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index c0570ef856..e7ca8fd1cf 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -721,13 +721,48 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>                                &fb, res, &ss.r, &cmd->error);
>  }
>  
> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
> +                                   struct virtio_gpu_set_scanout_blob *ss,
> +                                   uint64_t blob_size)
> +{
> +    uint64_t fbend;
> +
> +    fb->format = virtio_gpu_get_pixman_format(ss->format);
> +    if (!fb->format) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: host couldn't handle guest format %d\n",
> +                      __func__, ss->format);
> +        return false;
> +    }
> +
> +    fb->bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb->format), 8);
> +    fb->width = ss->width;
> +    fb->height = ss->height;
> +    fb->stride = ss->strides[0];
> +    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
> +
> +    fbend = fb->offset;
> +    fbend += fb->stride * (ss->r.height - 1);
> +    fbend += fb->bytes_pp * ss->r.width;
> +
> +    if (fbend > blob_size) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: fb end out of range\n",
> +                      __func__);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +
> +

Nit: extra newlines

-- 
Best regards,
Dmitry


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] hw/display: check frame buffer can hold blob
  2024-11-04 16:53 ` [PATCH 2/2] hw/display: check frame buffer can hold blob Alex Bennée
@ 2024-11-06  0:56   ` Dmitry Osipenko
  0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Osipenko @ 2024-11-06  0:56 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel; +Cc: Michael S. Tsirkin

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion
  2024-11-06  0:33   ` Dmitry Osipenko
@ 2024-11-06 17:36     ` Alex Bennée
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2024-11-06 17:36 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: qemu-devel, Michael S. Tsirkin

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 11/4/24 19:53, Alex Bennée wrote:
>> There are two identical sequences of a code doing the same thing that
>> raise warnings with Coverity. Before fixing those issues lets factor
>> out the common code into a helper function we can share.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>  include/hw/virtio/virtio-gpu.h | 15 +++++++++
>>  hw/display/virtio-gpu-virgl.c  | 21 +-----------
>>  hw/display/virtio-gpu.c        | 60 +++++++++++++++++++++-------------
>>  3 files changed, 53 insertions(+), 43 deletions(-)
>> 
>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
>> index 553799b8cc..90e4abe788 100644
>> --- a/include/hw/virtio/virtio-gpu.h
>> +++ b/include/hw/virtio/virtio-gpu.h
>> @@ -333,6 +333,21 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>>                                     struct virtio_gpu_scanout *s,
>>                                     uint32_t resource_id);
>>  
>> +/**
>> + * virtio_gpu_scanout_blob_to_fb() - fill out fb based on scanout data
>> + * fb: the frame-buffer descriptor to fill out
>> + * ss: the scanout blob data
>> + * blob_size: the maximum size the blob can accommodate
>
> Nit: 'maximum size the blob can accommodate' makes it sound to me like
> data will be copied into the blob. What about 'size of scanout blob data'.
>
>> + *
>> + * This will check we have enough space for the frame taking into
>> + * account that stride for all but the last line.
>> + *
>> + * Returns true on success, otherwise logs guest error and returns false
>> + */
>> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>> +                                   struct virtio_gpu_set_scanout_blob *ss,
>> +                                   uint64_t blob_size);
>> +
>>  /* virtio-gpu-udmabuf.c */
>>  bool virtio_gpu_have_udmabuf(void);
>>  void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index eedae7357f..35599cddab 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -852,26 +852,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>>          return;
>>      }
>>  
>> -    fb.format = virtio_gpu_get_pixman_format(ss.format);
>> -    if (!fb.format) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %d\n",
>> -                      __func__, ss.format);
>> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> -        return;
>> -    }
>> -
>> -    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
>> -    fb.width = ss.width;
>> -    fb.height = ss.height;
>> -    fb.stride = ss.strides[0];
>> -    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
>> -
>> -    fbend = fb.offset;
>> -    fbend += fb.stride * (ss.r.height - 1);
>> -    fbend += fb.bytes_pp * ss.r.width;
>> -    if (fbend > res->base.blob_size) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: fb end out of range\n",
>> -                      __func__);
>> +    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
>
> This fails to compile, needs s/res->blob_size/res->base.blob_size/
>
> ../hw/display/virtio-gpu-virgl.c:855:53: error: 'struct
> virtio_gpu_virgl_resource' has no member named 'blob_size'
>   855 |     if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
>       |                                                     ^~
> ../hw/display/virtio-gpu-virgl.c:808:14: error: unused variable 'fbend'
> [-Werror=unused-variable]
>   808 |     uint64_t fbend;
>       |              ^~~~~
> cc1: all warnings being treated as errors
>
> Please correct in v2.

Doh - I failed to compile that in my extra.libs config. Will fix.

>
>>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>>          return;
>>      }
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index c0570ef856..e7ca8fd1cf 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -721,13 +721,48 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>>                                &fb, res, &ss.r, &cmd->error);
>>  }
>>  
>> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>> +                                   struct virtio_gpu_set_scanout_blob *ss,
>> +                                   uint64_t blob_size)
>> +{
>> +    uint64_t fbend;
>> +
>> +    fb->format = virtio_gpu_get_pixman_format(ss->format);
>> +    if (!fb->format) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: host couldn't handle guest format %d\n",
>> +                      __func__, ss->format);
>> +        return false;
>> +    }
>> +
>> +    fb->bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb->format), 8);
>> +    fb->width = ss->width;
>> +    fb->height = ss->height;
>> +    fb->stride = ss->strides[0];
>> +    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
>> +
>> +    fbend = fb->offset;
>> +    fbend += fb->stride * (ss->r.height - 1);
>> +    fbend += fb->bytes_pp * ss->r.width;
>> +
>> +    if (fbend > blob_size) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: fb end out of range\n",
>> +                      __func__);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +
>> +
>
> Nit: extra newlines

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-11-06 17:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-04 16:53 [PATCH 0/2] virtio-gpu: coverity fixes Alex Bennée
2024-11-04 16:53 ` [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion Alex Bennée
2024-11-06  0:33   ` Dmitry Osipenko
2024-11-06 17:36     ` Alex Bennée
2024-11-04 16:53 ` [PATCH 2/2] hw/display: check frame buffer can hold blob Alex Bennée
2024-11-06  0:56   ` Dmitry Osipenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).