From 781b5470ec38d0b44991a9f4624fc5ffe91ee5b0 Mon Sep 17 00:00:00 2001 From: Jonah Palmer Date: Thu, 21 Aug 2025 14:26:41 +0000 Subject: [PATCH 1/5] net/hub: make net_hub_port_cleanup idempotent Makes the net_hub_port_cleanup function idempotent to avoid double removals by guarding its QLIST_REMOVE with a flag. When using a Xen networking device with hubport backends, e.g.: -accel kvm,xen-version=0x40011 -netdev hubport,... -device xen-net-device,... the shutdown order starts with net_cleanup, which walks the list and deletes netdevs (including hubports). Then Xen's xen_device_unrealize is called, which eventually leads to a second net_hub_port_cleanup call, resulting in a segfault. Fixes: e7891c57 ("net: move backend cleanup to NIC cleanup") Reported-by: David Woodhouse Signed-off-by: Jonah Palmer Signed-off-by: Jason Wang --- net/hub.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/net/hub.c b/net/hub.c index e3b58b1c4f..ee5881f6d5 100644 --- a/net/hub.c +++ b/net/hub.c @@ -34,6 +34,7 @@ typedef struct NetHubPort { QLIST_ENTRY(NetHubPort) next; NetHub *hub; int id; + bool listed; } NetHubPort; struct NetHub { @@ -129,7 +130,10 @@ static void net_hub_port_cleanup(NetClientState *nc) { NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc); - QLIST_REMOVE(port, next); + if (port->listed) { + QLIST_REMOVE(port, next); + port->listed = false; + } } static NetClientInfo net_hub_port_info = { @@ -159,8 +163,10 @@ static NetHubPort *net_hub_port_new(NetHub *hub, const char *name, port = DO_UPCAST(NetHubPort, nc, nc); port->id = id; port->hub = hub; + port->listed = false; QLIST_INSERT_HEAD(&hub->ports, port, next); + port->listed = true; return port; } From 6da0c9828194eb21e54fe4264cd29a1b85a29f33 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 3 Nov 2025 17:58:49 +0000 Subject: [PATCH 2/5] hw/net/e1000e_core: Don't advance desc_offset for NULL buffer RX descriptors In e1000e_write_packet_to_guest() we don't write data for RX descriptors where the buffer address is NULL (as required by the i82574 datasheet section 7.1.7.2). However, when we do this we still update desc_offset by the amount of data we would have written to the RX descriptor if it had a valid buffer pointer, resulting in our dropping that data entirely. The data sheet is not 100% clear on the subject, but this seems unlikely to be the correct behaviour. Rearrange the null-descriptor logic so that we don't treat these do-nothing descriptors as if we'd really written the data. This both fixes a bug and also is a prerequisite to cleaning up the size calculation logic in the next patch. (Cc to stable largely because it will be needed for the next patch, which fixes a more serious bug.) Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Akihiko Odaki Signed-off-by: Jason Wang --- hw/net/e1000e_core.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index 8fef598b49..ba77cb6011 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -1481,7 +1481,6 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, PCIDevice *d = core->owner; dma_addr_t base; union e1000_rx_desc_union desc; - size_t desc_size; size_t desc_offset = 0; size_t iov_ofs = 0; @@ -1500,12 +1499,6 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, E1000EBAState bastate = { { 0 } }; bool is_last = false; - desc_size = total_size - desc_offset; - - if (desc_size > core->rx_desc_buf_size) { - desc_size = core->rx_desc_buf_size; - } - if (e1000e_ring_empty(core, rxi)) { return; } @@ -1519,6 +1512,12 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, e1000e_read_rx_descr(core, &desc, ba); if (ba[0]) { + size_t desc_size = total_size - desc_offset; + + if (desc_size > core->rx_desc_buf_size) { + desc_size = core->rx_desc_buf_size; + } + if (desc_offset < size) { static const uint32_t fcs_pad; size_t iov_copy; @@ -1582,13 +1581,13 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, (const char *) &fcs_pad, e1000x_fcs_len(core->mac)); } } + desc_offset += desc_size; + if (desc_offset >= total_size) { + is_last = true; + } } else { /* as per intel docs; skip descriptors with null buf addr */ trace_e1000e_rx_null_descriptor(); } - desc_offset += desc_size; - if (desc_offset >= total_size) { - is_last = true; - } e1000e_write_rx_descr(core, &desc, is_last ? core->rx_pkt : NULL, rss_info, do_ps ? ps_hdr_len : 0, &bastate.written); From 9d946d56a2ac8a6c2df186e20d24810255c83a3f Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 3 Nov 2025 17:58:50 +0000 Subject: [PATCH 3/5] hw/net/e1000e_core: Correct rx oversize packet checks In e1000e_write_packet_to_guest() we attempt to ensure that we don't write more of a packet to a descriptor than will fit in the guest configured receive buffers. However, this code does not allow for the "packet split" feature. When packet splitting is enabled, the first of up to 4 buffers in the descriptor is used for the packet header only, with the payload going into buffers 2, 3 and 4. Our length check only checks against the total sizes of all 4 buffers, which meant that if an incoming packet was large enough to fit in (1 + 2 + 3 + 4) but not into (2 + 3 + 4) and packet splitting was enabled, we would run into the assertion in e1000e_write_hdr_frag_to_rx_buffers() that we had enough buffers for the data: qemu-system-i386: ../../hw/net/e1000e_core.c:1418: void e1000e_write_payload_frag_to_rx_buffers(E1000ECore *, hwaddr *, E1000EBAState *, const char *, dma_addr_t): Assertion `bastate->cur_idx < MAX_PS_BUFFERS' failed. A malicious guest could provoke this assertion by configuring the device into loopback mode, and then sending itself a suitably sized packet into a suitably arrange rx descriptor. The code also fails to deal with the possibility that the descriptor buffers are sized such that the trailing checksum word does not fit into the last descriptor which has actual data, which might also trigger this assertion. Rework the length handling to use two variables: * desc_size is the total amount of data DMA'd to the guest for the descriptor being processed in this iteration of the loop * rx_desc_buf_size is the total amount of space left in it As we copy data to the guest (packet header, payload, checksum), update these two variables. (Previously we attempted to calculate desc_size once at the top of the loop, but this is too difficult to do correctly.) Then we can use the variables to ensure that we clamp the amount of copied payload data to the remaining space in the descriptor's buffers, even if we've used one of the buffers up in the packet-split code, and we can tell whether we have enough space for the full checksum word in this descriptor or whether we're going to need to split that to the following descriptor. I have included comments that hopefully help to make the loop logic a little clearer. Cc: qemu-stable@nongnu.org Resolves: https://gitlab.com/qemu-project/qemu/-/issues/537 Reviewed-by: Akihiko Odaki Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/e1000e_core.c | 61 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index ba77cb6011..471c3ed20b 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -1495,6 +1495,13 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, rxi = rxr->i; do { + /* + * Loop processing descriptors while we have packet data to + * DMA to the guest. desc_offset tracks how much data we have + * sent to the guest in total over all descriptors, and goes + * from 0 up to total_size (the size of everything to send to + * the guest including possible trailing 4 bytes of CRC data). + */ hwaddr ba[MAX_PS_BUFFERS]; E1000EBAState bastate = { { 0 } }; bool is_last = false; @@ -1512,23 +1519,27 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, e1000e_read_rx_descr(core, &desc, ba); if (ba[0]) { - size_t desc_size = total_size - desc_offset; - - if (desc_size > core->rx_desc_buf_size) { - desc_size = core->rx_desc_buf_size; - } + /* Total amount of data DMA'd to the guest in this iteration */ + size_t desc_size = 0; + /* + * Total space available in this descriptor (we will update + * this as we use it up) + */ + size_t rx_desc_buf_size = core->rx_desc_buf_size; if (desc_offset < size) { - static const uint32_t fcs_pad; size_t iov_copy; + /* Amount of data to copy from the incoming packet */ size_t copy_size = size - desc_offset; - if (copy_size > core->rx_desc_buf_size) { - copy_size = core->rx_desc_buf_size; - } /* For PS mode copy the packet header first */ if (do_ps) { if (is_first) { + /* + * e1000e_do_ps() guarantees that buffer 0 has enough + * space for the header; otherwise we will not split + * the packet (i.e. do_ps is false). + */ size_t ps_hdr_copied = 0; do { iov_copy = MIN(ps_hdr_len - ps_hdr_copied, @@ -1550,14 +1561,26 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, } while (ps_hdr_copied < ps_hdr_len); is_first = false; + desc_size += ps_hdr_len; } else { /* Leave buffer 0 of each descriptor except first */ /* empty as per spec 7.1.5.1 */ e1000e_write_hdr_frag_to_rx_buffers(core, ba, &bastate, NULL, 0); } + rx_desc_buf_size -= core->rxbuf_sizes[0]; } + /* + * Clamp the amount of packet data we copy into what will fit + * into the remaining buffers in the descriptor. + */ + if (copy_size > rx_desc_buf_size) { + copy_size = rx_desc_buf_size; + } + desc_size += copy_size; + rx_desc_buf_size -= copy_size; + /* Copy packet payload */ while (copy_size) { iov_copy = MIN(copy_size, iov->iov_len - iov_ofs); @@ -1574,12 +1597,22 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt, iov_ofs = 0; } } + } - if (desc_offset + desc_size >= total_size) { - /* Simulate FCS checksum presence in the last descriptor */ - e1000e_write_payload_frag_to_rx_buffers(core, ba, &bastate, - (const char *) &fcs_pad, e1000x_fcs_len(core->mac)); - } + if (rx_desc_buf_size && + desc_offset >= size && desc_offset < total_size) { + /* + * We are in the last 4 bytes corresponding to the FCS checksum. + * We only ever write zeroes here (unlike the hardware). + */ + static const uint32_t fcs_pad; + /* Amount of space for the trailing checksum */ + size_t fcs_len = MIN(rx_desc_buf_size, + total_size - desc_offset); + e1000e_write_payload_frag_to_rx_buffers(core, ba, &bastate, + (const char *)&fcs_pad, + fcs_len); + desc_size += fcs_len; } desc_offset += desc_size; if (desc_offset >= total_size) { From bab496a18358643b686f69e2b97d73fb98d37e79 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 3 Nov 2025 17:58:51 +0000 Subject: [PATCH 4/5] hw/net/e1000e_core: Adjust e1000e_write_payload_frag_to_rx_buffers() assert An assertion in e1000e_write_payload_frag_to_rx_buffers() attempts to guard against the calling code accidentally trying to write too much data to a single RX descriptor, such that the E1000EBAState::cur_idx indexes off the end of the EB1000BAState::written[] array. Unfortunately it is overzealous: it asserts that cur_idx is in range after it has been incremented. This will fire incorrectly for the case where the guest configures four buffers and exactly enough bytes are written to fill all four of them. The only places where we use cur_idx and index in to the written[] array are the functions e1000e_write_hdr_frag_to_rx_buffers() and e1000e_write_payload_frag_to_rx_buffers(), so we can rewrite this to assert before doing the array dereference, rather than asserting after updating cur_idx. Cc: qemu-stable@nongnu.org Reviewed-by: Akihiko Odaki Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/e1000e_core.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index 471c3ed20b..46e156a5dd 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -1392,10 +1392,13 @@ e1000e_write_payload_frag_to_rx_buffers(E1000ECore *core, dma_addr_t data_len) { while (data_len > 0) { - uint32_t cur_buf_len = core->rxbuf_sizes[bastate->cur_idx]; - uint32_t cur_buf_bytes_left = cur_buf_len - - bastate->written[bastate->cur_idx]; - uint32_t bytes_to_write = MIN(data_len, cur_buf_bytes_left); + uint32_t cur_buf_len, cur_buf_bytes_left, bytes_to_write; + + assert(bastate->cur_idx < MAX_PS_BUFFERS); + + cur_buf_len = core->rxbuf_sizes[bastate->cur_idx]; + cur_buf_bytes_left = cur_buf_len - bastate->written[bastate->cur_idx]; + bytes_to_write = MIN(data_len, cur_buf_bytes_left); trace_e1000e_rx_desc_buff_write(bastate->cur_idx, ba[bastate->cur_idx], @@ -1414,8 +1417,6 @@ e1000e_write_payload_frag_to_rx_buffers(E1000ECore *core, if (bastate->written[bastate->cur_idx] == cur_buf_len) { bastate->cur_idx++; } - - assert(bastate->cur_idx < MAX_PS_BUFFERS); } } From a01344d9d78089e9e585faaeb19afccff2050abf Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 28 Oct 2025 16:00:42 +0000 Subject: [PATCH 5/5] net: pad packets to minimum length in qemu_receive_packet() In commits like 969e50b61a28 ("net: Pad short frames to minimum size before sending from SLiRP/TAP") we switched away from requiring network devices to handle short frames to instead having the net core code do the padding of short frames out to the ETH_ZLEN minimum size. We then dropped the code for handling short frames from the network devices in a series of commits like 140eae9c8f7 ("hw/net: e1000: Remove the logic of padding short frames in the receive path"). This missed one route where the device's receive code can still see a short frame: if the device is in loopback mode and it transmits a short frame via the qemu_receive_packet() function, this will be fed back into its own receive code without being padded. Add the padding logic to qemu_receive_packet(). This fixes a buffer overrun which can be triggered in the e1000_receive_iov() logic via the loopback code path. Other devices that use qemu_receive_packet() to implement loopback are cadence_gem, dp8393x, lan9118, msf2-emac, pcnet, rtl8139 and sungem. Cc: qemu-stable@nongnu.org Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3043 Reviewed-by: Akihiko Odaki Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- net/net.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/net/net.c b/net/net.c index 27e0d27807..8aefdb3424 100644 --- a/net/net.c +++ b/net/net.c @@ -775,10 +775,20 @@ ssize_t qemu_send_packet(NetClientState *nc, const uint8_t *buf, int size) ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf, int size) { + uint8_t min_pkt[ETH_ZLEN]; + size_t min_pktsz = sizeof(min_pkt); + if (!qemu_can_receive_packet(nc)) { return 0; } + if (net_peer_needs_padding(nc)) { + if (eth_pad_short_frame(min_pkt, &min_pktsz, buf, size)) { + buf = min_pkt; + size = min_pktsz; + } + } + return qemu_net_queue_receive(nc->incoming_queue, buf, size); }