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

[Michael S. Tsirkin]

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