From 3f9f6299a1bc46ff462142c7398a5953e3640cc2 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 16 Jul 2025 10:28:53 +0300 Subject: [PATCH 01/12] net/tap: drop too small packets Theoretically tap_read_packet() may return size less than s->host_vnet_hdr_len, and next, we'll work with negative size (in case of !s->using_vnet_hdr). Let's avoid it. Don't proceed with size == s->host_vnet_hdr_len as well in case of !s->using_vnet_hdr, it doesn't make sense. Tested-by: Lei Yang Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Jason Wang --- net/tap.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/tap.c b/net/tap.c index 23536c09b4..2a85936019 100644 --- a/net/tap.c +++ b/net/tap.c @@ -190,6 +190,11 @@ static void tap_send(void *opaque) break; } + if (s->host_vnet_hdr_len && size <= s->host_vnet_hdr_len) { + /* Invalid packet */ + break; + } + if (s->host_vnet_hdr_len && !s->using_vnet_hdr) { buf += s->host_vnet_hdr_len; size -= s->host_vnet_hdr_len; From ea263e8fd9a8f9cff615631a9bda775f5ddd3f98 Mon Sep 17 00:00:00 2001 From: Steve Sistare Date: Mon, 14 Jul 2025 13:36:54 -0700 Subject: [PATCH 02/12] tap: fix net_init_tap() return code net_init_tap intends to return 0 for success and -1 on error. However, when net_init_tap() succeeds for a multi-queue device, it returns 1, because of this code where ret becomes 1 when g_unix_set_fd_nonblocking succeeds: ret = g_unix_set_fd_nonblocking(fd, true, NULL); if (!ret) { ... error ... free_fail: ... return ret; Luckily, the only current call site checks for negative, rather than non-zero: net_client_init1() if (net_client_init_fun[](...) < 0) Also, in the unlikely case that g_unix_set_fd_nonblocking fails and returns false, ret=0 is returned, and net_client_init1 will use a broken interface. Fix it to be future proof. Signed-off-by: Steve Sistare Signed-off-by: Jason Wang --- net/tap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/tap.c b/net/tap.c index 2a85936019..f7df702f97 100644 --- a/net/tap.c +++ b/net/tap.c @@ -895,8 +895,8 @@ int net_init_tap(const Netdev *netdev, const char *name, goto free_fail; } - ret = g_unix_set_fd_nonblocking(fd, true, NULL); - if (!ret) { + if (!g_unix_set_fd_nonblocking(fd, true, NULL)) { + ret = -1; error_setg_errno(errp, errno, "%s: Can't use file descriptor %d", name, fd); goto free_fail; From 871a6e5b339f0b5e71925ec7d3f452944a1c82d3 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 14 Jul 2025 17:55:20 +0100 Subject: [PATCH 03/12] hw/net/npcm_gmac.c: Send the right data for second packet in a row The transmit loop in gmac_try_send_next_packet() is constructed in a way that means it will send incorrect data if it it sends more than one packet. The function assembles the outbound data in a dynamically allocated block of memory which is pointed to by tx_send_buffer. We track the first point in this block of memory which is not yet used with the prev_buf_size offset, initially zero. We track the size of the packet we're sending with the length variable, also initially zero. As we read chunks of data out of guest memory, we write them to tx_send_buffer[prev_buf_size], and then increment both prev_buf_size and length. (We might dynamically reallocate the buffer if needed.) When we send a packet, we checksum and send length bytes, starting at tx_send_buffer, and then we reset length to 0. This gives the right data for the first packet. But we don't reset prev_buf_size. This means that if we process more descriptors with further data for the next packet, that data will continue to accumulate at offset prev_buf_size, i.e. after the data for the first packet. But when we transmit that second packet, we send length bytes from tx_send_buffer, so we will send a packet which has the length of the second packet but the data of the first one. The fix for this is to also clear prev_buf_size after the packet has been sent -- we never need the data from packet one after we've sent it, so we can write packet two's data starting at the beginning of the buffer. Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/npcm_gmac.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c index a434112580..921327dd8c 100644 --- a/hw/net/npcm_gmac.c +++ b/hw/net/npcm_gmac.c @@ -615,6 +615,7 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length); buf = tx_send_buffer; length = 0; + prev_buf_size = 0; } /* step 6 */ From 01b327c9a609cb613da1e04c5af5f4fcc7d9c0a3 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 14 Jul 2025 17:55:21 +0100 Subject: [PATCH 04/12] hw/net/npcm_gmac.c: Unify length and prev_buf_size variables After the bug fix in the previous commit, the length and prev_buf_size variables are identical, except that prev_buf_size is uint32_t and length is uint16_t. We can therefore unify them. The only place where the type makes a difference is that we will truncate the packet at 64K when sending it; this commit preserves that behaviour by using a local variable when doing the packet send. Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/npcm_gmac.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c index 921327dd8c..a0050a7725 100644 --- a/hw/net/npcm_gmac.c +++ b/hw/net/npcm_gmac.c @@ -516,7 +516,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) uint32_t desc_addr; struct NPCMGMACTxDesc tx_desc; uint32_t tx_buf_addr, tx_buf_len; - uint16_t length = 0; uint8_t *buf = tx_send_buffer; uint32_t prev_buf_size = 0; int csum = 0; @@ -583,7 +582,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) __func__, tx_buf_addr); return; } - length += tx_buf_len; prev_buf_size += tx_buf_len; /* If not chained we'll have a second buffer. */ @@ -606,15 +604,18 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) __func__, tx_buf_addr); return; } - length += tx_buf_len; prev_buf_size += tx_buf_len; } if (tx_desc.tdes1 & TX_DESC_TDES1_LAST_SEG_MASK) { + /* + * This will truncate the packet at 64K. + * TODO: find out if this is the correct behaviour. + */ + uint16_t length = prev_buf_size; net_checksum_calculate(tx_send_buffer, length, csum); qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length); trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length); buf = tx_send_buffer; - length = 0; prev_buf_size = 0; } From e96a1aa6766f1b188dada3ad8d172c0fbe2f3a5f Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 14 Jul 2025 17:55:22 +0100 Subject: [PATCH 05/12] hw/net/npcm_gmac.c: Correct test for when to reallocate packet buffer In gmac_try_send_next_packet() we have code that does "if this block of data won't fit in the buffer, reallocate it". However, the condition it uses is if ((prev_buf_size + tx_buf_len) > sizeof(buf)) where buf is a uint8_t *. This means that sizeof(buf) is always 8 bytes, and the condition will almost always be true, so we will reallocate the buffer more often than we need to. Correct the condition to test against tx_buffer_size, which is where we track how big the allocated buffer is. Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/npcm_gmac.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c index a0050a7725..0c17ae9b2a 100644 --- a/hw/net/npcm_gmac.c +++ b/hw/net/npcm_gmac.c @@ -569,7 +569,7 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) tx_buf_len = TX_DESC_TDES1_BFFR1_SZ_MASK(tx_desc.tdes1); buf = &tx_send_buffer[prev_buf_size]; - if ((prev_buf_size + tx_buf_len) > sizeof(buf)) { + if ((prev_buf_size + tx_buf_len) > tx_buffer_size) { tx_buffer_size = prev_buf_size + tx_buf_len; tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size); buf = &tx_send_buffer[prev_buf_size]; @@ -591,7 +591,7 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) tx_buf_len = TX_DESC_TDES1_BFFR2_SZ_MASK(tx_desc.tdes1); buf = &tx_send_buffer[prev_buf_size]; - if ((prev_buf_size + tx_buf_len) > sizeof(buf)) { + if ((prev_buf_size + tx_buf_len) > tx_buffer_size) { tx_buffer_size = prev_buf_size + tx_buf_len; tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size); buf = &tx_send_buffer[prev_buf_size]; From a6d6e37901551cbd61b0a61c6bab5745fb1833ff Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 14 Jul 2025 17:55:23 +0100 Subject: [PATCH 06/12] hw/net/npcm_gmac.c: Drop 'buf' local variable We use the local variable 'buf' only when we call dma_memory_read(), and it is always set to &tx_send_buffer[prev_buf_size] immediately before both of those calls. So remove the variable and pass tx_send_buffer + prev_buf_size to dma_memory_read(). This fixes in passing a place where we set buf = tx_send_buffer but never used that value because we always updated buf to something else later before using it. Coverity: CID 1534027 Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/npcm_gmac.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c index 0c17ae9b2a..5e32cd3edf 100644 --- a/hw/net/npcm_gmac.c +++ b/hw/net/npcm_gmac.c @@ -516,7 +516,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) uint32_t desc_addr; struct NPCMGMACTxDesc tx_desc; uint32_t tx_buf_addr, tx_buf_len; - uint8_t *buf = tx_send_buffer; uint32_t prev_buf_size = 0; int csum = 0; @@ -567,16 +566,15 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) tx_buf_addr = tx_desc.tdes2; gmac->regs[R_NPCM_DMA_CUR_TX_BUF_ADDR] = tx_buf_addr; tx_buf_len = TX_DESC_TDES1_BFFR1_SZ_MASK(tx_desc.tdes1); - buf = &tx_send_buffer[prev_buf_size]; if ((prev_buf_size + tx_buf_len) > tx_buffer_size) { tx_buffer_size = prev_buf_size + tx_buf_len; tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size); - buf = &tx_send_buffer[prev_buf_size]; } /* step 5 */ - if (dma_memory_read(&address_space_memory, tx_buf_addr, buf, + if (dma_memory_read(&address_space_memory, tx_buf_addr, + tx_send_buffer + prev_buf_size, tx_buf_len, MEMTXATTRS_UNSPECIFIED)) { qemu_log_mask(LOG_GUEST_ERROR, "%s: Failed to read packet @ 0x%x\n", __func__, tx_buf_addr); @@ -589,15 +587,14 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) tx_buf_addr = tx_desc.tdes3; gmac->regs[R_NPCM_DMA_CUR_TX_BUF_ADDR] = tx_buf_addr; tx_buf_len = TX_DESC_TDES1_BFFR2_SZ_MASK(tx_desc.tdes1); - buf = &tx_send_buffer[prev_buf_size]; if ((prev_buf_size + tx_buf_len) > tx_buffer_size) { tx_buffer_size = prev_buf_size + tx_buf_len; tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size); - buf = &tx_send_buffer[prev_buf_size]; } - if (dma_memory_read(&address_space_memory, tx_buf_addr, buf, + if (dma_memory_read(&address_space_memory, tx_buf_addr, + tx_send_buffer + prev_buf_size, tx_buf_len, MEMTXATTRS_UNSPECIFIED)) { qemu_log_mask(LOG_GUEST_ERROR, "%s: Failed to read packet @ 0x%x\n", @@ -615,7 +612,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac) net_checksum_calculate(tx_send_buffer, length, csum); qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length); trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length); - buf = tx_send_buffer; prev_buf_size = 0; } From 2ed74e3c005f236acbf7d85d396a653bfb004a17 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Thu, 17 Jul 2025 17:08:00 +0200 Subject: [PATCH 07/12] net/passt: Remove unused "err" from passt_vhost_user_event() (CID 1612375) The "err" variable was declared but never used within the passt_vhost_user_event() function. This resulted in a dead code warning (CID 1612375) from Coverity. Remove the unused variable and the associated error block to resolve the issue. Signed-off-by: Laurent Vivier Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- net/passt.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/net/passt.c b/net/passt.c index 6f616ba3c2..9cd5b3e6f2 100644 --- a/net/passt.c +++ b/net/passt.c @@ -397,7 +397,6 @@ err: static void passt_vhost_user_event(void *opaque, QEMUChrEvent event) { NetPasstState *s = opaque; - Error *err = NULL; switch (event) { case CHR_EVENT_OPENED: @@ -428,10 +427,6 @@ static void passt_vhost_user_event(void *opaque, QEMUChrEvent event) /* Ignore */ break; } - - if (err) { - error_report_err(err); - } } static int net_passt_vhost_user_init(NetPasstState *s, Error **errp) From 37c8b208cc3161c2bcd4e2ff1e1077a64872ced3 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Thu, 17 Jul 2025 17:08:01 +0200 Subject: [PATCH 08/12] net/vhost-user: Remove unused "err" from net_vhost_user_event() (CID 1612372) The "err" variable was declared but never used within the net_vhost_user_event() function. This resulted in a dead code warning (CID 1612372) from Coverity. Remove the unused variable and the associated error block to resolve the issue. Signed-off-by: Laurent Vivier Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- net/vhost-user.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/net/vhost-user.c b/net/vhost-user.c index 1c3b8b36f3..cec83e925f 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -329,7 +329,6 @@ static void net_vhost_user_event(void *opaque, QEMUChrEvent event) NetClientState *ncs[MAX_QUEUE_NUM]; NetVhostUserState *s; Chardev *chr; - Error *err = NULL; int queues; queues = qemu_find_net_clients_except(name, ncs, @@ -375,10 +374,6 @@ static void net_vhost_user_event(void *opaque, QEMUChrEvent event) /* Ignore */ break; } - - if (err) { - error_report_err(err); - } } static int net_vhost_user_init(NetClientState *peer, const char *device, From c40ef7243fdbec816242cb0ded569a61270ea7c3 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Thu, 17 Jul 2025 17:08:02 +0200 Subject: [PATCH 09/12] net/passt: Remove dead code in passt_vhost_user_start error path (CID 1612371) In passt_vhost_user_start(), if vhost_net_init() fails, the "net" variable is NULL and execution jumps to the "err:" label. The cleanup code within this label is conditioned on "if (net)", which can never be true in this error case. This makes the cleanup block dead code, as reported by Coverity (CID 1612371). Refactor the error handling to occur inline, removing the goto and the unreachable cleanup block. Signed-off-by: Laurent Vivier Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- net/passt.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/net/passt.c b/net/passt.c index 9cd5b3e6f2..ef59d0682b 100644 --- a/net/passt.c +++ b/net/passt.c @@ -375,7 +375,8 @@ static int passt_vhost_user_start(NetPasstState *s, VhostUserState *be) net = vhost_net_init(&options); if (!net) { error_report("failed to init passt vhost_net"); - goto err; + passt_vhost_user_stop(s); + return -1; } if (s->vhost_net) { @@ -385,13 +386,6 @@ static int passt_vhost_user_start(NetPasstState *s, VhostUserState *be) s->vhost_net = net; return 0; -err: - if (net) { - vhost_net_cleanup(net); - g_free(net); - } - passt_vhost_user_stop(s); - return -1; } static void passt_vhost_user_event(void *opaque, QEMUChrEvent event) From f74e4f2e60c3b2c6e2f13eb115478df116148fe6 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Thu, 17 Jul 2025 17:08:03 +0200 Subject: [PATCH 10/12] net/passt: Check return value of g_remove() in net_passt_cleanup() (CID 1612369) If g_remove() fails, use warn_report() to log an error. Signed-off-by: Laurent Vivier Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- net/passt.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/passt.c b/net/passt.c index ef59d0682b..43c336e596 100644 --- a/net/passt.c +++ b/net/passt.c @@ -103,7 +103,10 @@ static void net_passt_cleanup(NetClientState *nc) #endif kill(s->pid, SIGTERM); - g_remove(s->pidfile); + if (g_remove(s->pidfile) != 0) { + warn_report("Failed to remove passt pidfile %s: %s", + s->pidfile, strerror(errno)); + } g_free(s->pidfile); g_ptr_array_free(s->args, TRUE); } From 667ad57b7636881eb79b0b319951f3cba9bbf27e Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Thu, 17 Jul 2025 17:08:04 +0200 Subject: [PATCH 11/12] net/passt: Initialize "error" variable in net_passt_send() (CID 1612368) This was flagged by Coverity as a memory illegal access. Initialize the pointer to NULL at declaration. Signed-off-by: Laurent Vivier Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- net/passt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/passt.c b/net/passt.c index 43c336e596..32ecffb763 100644 --- a/net/passt.c +++ b/net/passt.c @@ -124,7 +124,7 @@ static gboolean net_passt_send(QIOChannel *ioc, GIOCondition condition, { if (net_stream_data_send(ioc, condition, data) == G_SOURCE_REMOVE) { NetPasstState *s = DO_UPCAST(NetPasstState, data, data); - Error *error; + Error *error = NULL; /* we need to restart passt */ kill(s->pid, SIGTERM); From ae9b09972bbf8ff49ae0edf3241fb413391b15ce Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Thu, 17 Jul 2025 17:08:05 +0200 Subject: [PATCH 12/12] net/vhost-user: Remove unused "err" from chr_closed_bh() (CID 1612365) The "err" variable was declared but never used within the chr_closed_bh() function. This resulted in a dead code warning (CID 1612365) from Coverity. Remove the unused variable and the associated error block to resolve the issue. Signed-off-by: Laurent Vivier Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- net/vhost-user.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/net/vhost-user.c b/net/vhost-user.c index cec83e925f..8b96157145 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -298,7 +298,6 @@ static void chr_closed_bh(void *opaque) const char *name = opaque; NetClientState *ncs[MAX_QUEUE_NUM]; NetVhostUserState *s; - Error *err = NULL; int queues, i; queues = qemu_find_net_clients_except(name, ncs, @@ -317,9 +316,6 @@ static void chr_closed_bh(void *opaque) qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event, NULL, opaque, NULL, true); - if (err) { - error_report_err(err); - } qapi_event_send_netdev_vhost_user_disconnected(name); }