* [PATCH] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll()
@ 2023-02-10 10:29 Carlos López
2023-02-12 9:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 2+ messages in thread
From: Carlos López @ 2023-02-10 10:29 UTC (permalink / raw)
To: qemu-devel; +Cc: Carlos López, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll()
2023-02-10 10:29 [PATCH] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll() Carlos López
@ 2023-02-12 9:58 ` Michael S. Tsirkin
0 siblings, 0 replies; 2+ messages in thread
From: Michael S. Tsirkin @ 2023-02-12 9:58 UTC (permalink / raw)
To: Carlos López; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-02-12 9:59 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-10 10:29 [PATCH] vhost: avoid a potential use of an uninitialized variable in vhost_svq_poll() Carlos López
2023-02-12 9:58 ` Michael S. Tsirkin
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).