[v1] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll()

[PATCH] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll()

Posted by Carlos López 1 year, 2 months ago

In vhost_svq_poll(), if vhost_svq_get_buf() fails due to a device
providing invalid descriptors, len is left uninitialized and returned
to the caller, potentally leaking stack data or causing undefined
behavior.

Fix this by initializing len to 0.

Found with GCC 13 and -fanalyzer (abridged):

../hw/virtio/vhost-shadow-virtqueue.c: In function ‘vhost_svq_poll’:
../hw/virtio/vhost-shadow-virtqueue.c:538:12: warning: use of uninitialized value ‘len’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
  538 |     return len;
      |            ^~~
  ‘vhost_svq_poll’: events 1-4
    |
    |  522 | size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
    |      |        ^~~~~~~~~~~~~~
    |      |        |
    |      |        (1) entry to ‘vhost_svq_poll’
    |......
    |  525 |     uint32_t len;
    |      |              ~~~
    |      |              |
    |      |              (2) region created on stack here
    |      |              (3) capacity: 4 bytes
    |......
    |  528 |         if (vhost_svq_more_used(svq)) {
    |      |             ~
    |      |             |
    |      |             (4) inlined call to ‘vhost_svq_more_used’ from ‘vhost_svq_poll’

    (...)

    |  528 |         if (vhost_svq_more_used(svq)) {
    |      |            ^~~~~~~~~~~~~~~~~~~~~~~~~
    |      |            ||
    |      |            |(8) ...to here
    |      |            (7) following ‘true’ branch...
    |......
    |  537 |     vhost_svq_get_buf(svq, &len);
    |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (9) calling ‘vhost_svq_get_buf’ from ‘vhost_svq_poll’
    |
    +--> ‘vhost_svq_get_buf’: events 10-11
           |
           |  416 | static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
           |      |                          ^~~~~~~~~~~~~~~~~
           |      |                          |
           |      |                          (10) entry to ‘vhost_svq_get_buf’
           |......
           |  423 |     if (!vhost_svq_more_used(svq)) {
           |      |          ~
           |      |          |
           |      |          (11) inlined call to ‘vhost_svq_more_used’ from ‘vhost_svq_get_buf’
           |

           (...)

           |
         ‘vhost_svq_get_buf’: event 14
           |
           |  423 |     if (!vhost_svq_more_used(svq)) {
           |      |        ^
           |      |        |
           |      |        (14) following ‘false’ branch...
           |
         ‘vhost_svq_get_buf’: event 15
           |
           |cc1:
           | (15): ...to here
           |
    <------+
    |
  ‘vhost_svq_poll’: events 16-17
    |
    |  537 |     vhost_svq_get_buf(svq, &len);
    |      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (16) returning to ‘vhost_svq_poll’ from ‘vhost_svq_get_buf’
    |  538 |     return len;
    |      |            ~~~
    |      |            |
    |      |            (17) use of uninitialized value ‘len’ here

Signed-off-by: Carlos López <clopez@suse.de>
---
 hw/virtio/vhost-shadow-virtqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 4307296358..515ccf870d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -522,7 +522,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
 size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
 {
     int64_t start_us = g_get_monotonic_time();
-    uint32_t len;
+    uint32_t len = 0;
 
     do {
         if (vhost_svq_more_used(svq)) {
-- 
2.35.3

Re: [PATCH] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll()

Posted by Michael S. Tsirkin 1 year, 2 months ago

On Fri, Feb 10, 2023 at 11:29:16AM +0100, Carlos López wrote:
> In vhost_svq_poll(), if vhost_svq_get_buf() fails due to a device
> providing invalid descriptors, len is left uninitialized and returned
> to the caller, potentally leaking stack data or causing undefined
> behavior.
> 
> Fix this by initializing len to 0.
> 
> Found with GCC 13 and -fanalyzer (abridged):
> 
> ../hw/virtio/vhost-shadow-virtqueue.c: In function ‘vhost_svq_poll’:
> ../hw/virtio/vhost-shadow-virtqueue.c:538:12: warning: use of uninitialized value ‘len’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
>   538 |     return len;
>       |            ^~~
>   ‘vhost_svq_poll’: events 1-4
>     |
>     |  522 | size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
>     |      |        ^~~~~~~~~~~~~~
>     |      |        |
>     |      |        (1) entry to ‘vhost_svq_poll’
>     |......
>     |  525 |     uint32_t len;
>     |      |              ~~~
>     |      |              |
>     |      |              (2) region created on stack here
>     |      |              (3) capacity: 4 bytes
>     |......
>     |  528 |         if (vhost_svq_more_used(svq)) {
>     |      |             ~
>     |      |             |
>     |      |             (4) inlined call to ‘vhost_svq_more_used’ from ‘vhost_svq_poll’
> 
>     (...)
> 
>     |  528 |         if (vhost_svq_more_used(svq)) {
>     |      |            ^~~~~~~~~~~~~~~~~~~~~~~~~
>     |      |            ||
>     |      |            |(8) ...to here
>     |      |            (7) following ‘true’ branch...
>     |......
>     |  537 |     vhost_svq_get_buf(svq, &len);
>     |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     |      |     |
>     |      |     (9) calling ‘vhost_svq_get_buf’ from ‘vhost_svq_poll’
>     |
>     +--> ‘vhost_svq_get_buf’: events 10-11
>            |
>            |  416 | static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>            |      |                          ^~~~~~~~~~~~~~~~~
>            |      |                          |
>            |      |                          (10) entry to ‘vhost_svq_get_buf’
>            |......
>            |  423 |     if (!vhost_svq_more_used(svq)) {
>            |      |          ~
>            |      |          |
>            |      |          (11) inlined call to ‘vhost_svq_more_used’ from ‘vhost_svq_get_buf’
>            |
> 
>            (...)
> 
>            |
>          ‘vhost_svq_get_buf’: event 14
>            |
>            |  423 |     if (!vhost_svq_more_used(svq)) {
>            |      |        ^
>            |      |        |
>            |      |        (14) following ‘false’ branch...
>            |
>          ‘vhost_svq_get_buf’: event 15
>            |
>            |cc1:
>            | (15): ...to here
>            |
>     <------+
>     |
>   ‘vhost_svq_poll’: events 16-17
>     |
>     |  537 |     vhost_svq_get_buf(svq, &len);
>     |      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     |      |     |
>     |      |     (16) returning to ‘vhost_svq_poll’ from ‘vhost_svq_get_buf’
>     |  538 |     return len;
>     |      |            ~~~
>     |      |            |
>     |      |            (17) use of uninitialized value ‘len’ here
> 
> Signed-off-by: Carlos López <clopez@suse.de>

Thanks for the fix!
Could you add a Fixes tag? Which version introduced this?


> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 4307296358..515ccf870d 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -522,7 +522,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>  size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
>  {
>      int64_t start_us = g_get_monotonic_time();
> -    uint32_t len;
> +    uint32_t len = 0;
>  
>      do {
>          if (vhost_svq_more_used(svq)) {
> -- 
> 2.35.3