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 <clo...@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
