The following changes since commit bdee969c0e65d4d509932b1d70e3a3b2ffbff6d5:

Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2021-03-19 18:01:17 +0000)

for you to fetch changes up to c7274b5ef43614dd133daec1e2018f71d8744088:

net/eth: Add an assert() and invert if() statement to simplify code (2021-03-22 17:34:31 +0800)

----------------------------------------------------------------

Bin Meng (4):

net: eth: Add a helper to pad a short Ethernet frame

net: Add a 'do_not_pad" to NetClientState

net: Pad short frames to minimum size before sending from SLiRP/TAP

hw/net: virtio-net: Initialize nc->do_not_pad to true

Lukas Straub (2):

net/colo-compare.c: Fix memory leak for non-tcp packet

net/colo-compare.c: Optimize removal of secondary packet

Philippe Mathieu-Daudé (7):

net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr()

net/eth: Simplify _eth_get_rss_ex_dst_addr()

net/eth: Better describe _eth_get_rss_ex_dst_addr's offset argument

net/eth: Check size earlier in _eth_get_rss_ex_dst_addr()

net/eth: Check iovec has enough data earlier

net/eth: Read ip6_ext_hdr_routing buffer before accessing it

net/eth: Add an assert() and invert if() statement to simplify code

MAINTAINERS | 1 +

hw/net/virtio-net.c | 4 +++

include/net/eth.h | 17 ++++++++++++

include/net/net.h | 1 +

net/colo-compare.c | 3 ++-

net/eth.c | 61 +++++++++++++++++++++++++++---------------

net/slirp.c | 10 +++++++

net/tap-win32.c | 10 +++++++

net/tap.c | 10 +++++++

tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++++

tests/qtest/meson.build | 1 +

11 files changed, 148 insertions(+), 23 deletions(-)

create mode 100644 tests/qtest/fuzz-e1000e-test.c

From: Bin Meng <bmeng.cn@gmail.com>

Add a helper to pad a short Ethernet frame to the minimum required

length, which can be used by backends' code.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

include/net/eth.h | 17 +++++++++++++++++

net/eth.c | 17 +++++++++++++++++

2 files changed, 34 insertions(+)

diff --git a/include/net/eth.h b/include/net/eth.h

--- a/include/net/eth.h

+++ b/include/net/eth.h

#define ETH_ALEN 6

#define ETH_HLEN 14

+#define ETH_ZLEN 60 /* Min. octets in frame without FCS */

struct eth_header {

uint8_t h_dest[ETH_ALEN]; /* destination eth addr */

@@ -XXX,XX +XXX,XX @@ bool

eth_parse_ipv6_hdr(const struct iovec *pkt, int pkt_frags,

size_t ip6hdr_off, eth_ip6_hdr_info *info);

+/**

+ * eth_pad_short_frame - pad a short frame to the minimum Ethernet frame length

+ *

+ * If the Ethernet frame size is shorter than 60 bytes, it will be padded to

+ * 60 bytes at the address @padded_pkt.

+ *

+ * @padded_pkt: buffer address to hold the padded frame

+ * @padded_buflen: pointer holding length of @padded_pkt. If the frame is

+ * padded, the length will be updated to the padded one.

+ * @pkt: address to hold the original Ethernet frame

+ * @pkt_size: size of the original Ethernet frame

+ * @return true if the frame is padded, otherwise false

+ */

+bool eth_pad_short_frame(uint8_t *padded_pkt, size_t *padded_buflen,

+ const void *pkt, size_t pkt_size);

#endif

diff --git a/net/eth.c b/net/eth.c

index XXXXXXX..XXXXXXX 100644

--- a/net/eth.c

+++ b/net/eth.c

@@ -XXX,XX +XXX,XX @@ bool eth_parse_ipv6_hdr(const struct iovec *pkt, int pkt_frags,

info->l4proto = ext_hdr.ip6r_nxt;

return true;

}

+

+bool eth_pad_short_frame(uint8_t *padded_pkt, size_t *padded_buflen,

+ const void *pkt, size_t pkt_size)

+{

+ assert(padded_buflen && *padded_buflen >= ETH_ZLEN);

+

+ if (pkt_size >= ETH_ZLEN) {

+ return false;

+ /* pad to minimum Ethernet frame length */

+ memcpy(padded_pkt, pkt, pkt_size);

+ memset(&padded_pkt[pkt_size], 0, ETH_ZLEN - pkt_size);

+ *padded_buflen = ETH_ZLEN;

+ return true;

From: Bin Meng <bmeng.cn@gmail.com>

This adds a flag in NetClientState, so that a net client can tell

its peer that the packets do not need to be padded to the minimum

size of an Ethernet frame (60 bytes) before sending to it.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

1 file changed, 1 insertion(+)

int vnet_hdr_len;

bool is_netdev;

+ bool do_not_pad; /* do not pad to the minimum ethernet frame length */

QTAILQ_HEAD(, NetFilterState) filters;

From: Bin Meng <bmeng.cn@gmail.com>

The minimum Ethernet frame length is 60 bytes. For short frames with

smaller length like ARP packets (only 42 bytes), on a real world NIC

it can choose either padding its length to the minimum required 60

bytes, or sending it out directly to the wire. Such behavior can be

hardcoded or controled by a register bit. Similarly on the receive

path, NICs can choose either dropping such short frames directly or

handing them over to software to handle.

On the other hand, for the network backends like SLiRP/TAP, they

don't expose a way to control the short frame behavior. As of today

they just send/receive data from/to the other end connected to them,

which means any sized packet is acceptable. So they can send and

receive short frames without any problem. It is observed that ARP

packets sent from SLiRP/TAP are 42 bytes, and SLiRP/TAP just send

these ARP packets to the other end which might be a NIC model that

does not allow short frames to pass through.

To provide better compatibility, for packets sent from QEMU network

backends like SLiRP/TAP, we change to pad short frames before sending

it out to the other end, if the other end does not forbid it via the

nc->do_not_pad flag. This ensures a backend as an Ethernet sender

does not violate the spec. But with this change, the behavior of

dropping short frames from SLiRP/TAP interfaces in the NIC model

cannot be emulated because it always receives a packet that is spec

complaint. The capability of sending short frames from NIC models is

still supported and short frames can still pass through SLiRP/TAP.

This commit should be able to fix the issue as reported with some

NIC models before, that ARP requests get dropped, preventing the

guest from becoming visible on the network. It was workarounded in

these NIC models on the receive path, that when a short frame is

received, it is padded up to 60 bytes.

The following 2 commits seem to be the one to workaround this issue

in e1000 and vmxenet3 before, and should probably be reverted.

commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")

commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

net/slirp.c | 10 ++++++++++

net/tap-win32.c | 10 ++++++++++

net/tap.c | 10 ++++++++++

3 files changed, 30 insertions(+)

diff --git a/net/slirp.c b/net/slirp.c

--- a/net/slirp.c

+++ b/net/slirp.c

@@ -XXX,XX +XXX,XX @@

#include <pwd.h>

#include <sys/wait.h>

#endif

+#include "net/eth.h"

#include "net/net.h"

#include "clients.h"

#include "hub.h"

@@ -XXX,XX +XXX,XX @@ static ssize_t net_slirp_send_packet(const void *pkt, size_t pkt_len,

void *opaque)

{

SlirpState *s = opaque;

+ uint8_t min_pkt[ETH_ZLEN];

+ size_t min_pktsz = sizeof(min_pkt);

+ if (!s->nc.peer->do_not_pad) {

+ if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pkt_len)) {

+ pkt = min_pkt;

+ pkt_len = min_pktsz;

+ }

+ }

return qemu_send_packet(&s->nc, pkt, pkt_len);

}

diff --git a/net/tap-win32.c b/net/tap-win32.c

index XXXXXXX..XXXXXXX 100644

--- a/net/tap-win32.c

+++ b/net/tap-win32.c

@@ -XXX,XX +XXX,XX @@

#include "qemu-common.h"

#include "clients.h" /* net_init_tap */

+#include "net/eth.h"

#include "net/net.h"

#include "net/tap.h" /* tap_has_ufo, ... */

#include "qemu/error-report.h"

@@ -XXX,XX +XXX,XX @@ static void tap_win32_send(void *opaque)

uint8_t *buf;

int max_size = 4096;

int size;

+ uint8_t min_pkt[ETH_ZLEN];

+ size_t min_pktsz = sizeof(min_pkt);

size = tap_win32_read(s->handle, &buf, max_size);

if (size > 0) {

+ if (!s->nc.peer->do_not_pad) {

+ if (eth_pad_short_frame(min_pkt, &min_pktsz, buf, size)) {

+ buf = min_pkt;

+ size = min_pktsz;

+ }

+ }

+

qemu_send_packet(&s->nc, buf, size);

tap_win32_free_buffer(s->handle, buf);

}

diff --git a/net/tap.c b/net/tap.c

index XXXXXXX..XXXXXXX 100644

--- a/net/tap.c

+++ b/net/tap.c

@@ -XXX,XX +XXX,XX @@

#include <sys/socket.h>

#include <net/if.h>

+#include "net/eth.h"

#include "net/net.h"

#include "clients.h"

#include "monitor/monitor.h"

@@ -XXX,XX +XXX,XX @@ static void tap_send(void *opaque)

while (true) {

uint8_t *buf = s->buf;

+ uint8_t min_pkt[ETH_ZLEN];

+ size_t min_pktsz = sizeof(min_pkt);

size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));

if (size <= 0) {

@@ -XXX,XX +XXX,XX @@ static void tap_send(void *opaque)

size -= s->host_vnet_hdr_len;

From: Bin Meng <bmeng.cn@gmail.com>

For virtio-net, there is no need to pad the Ethernet frame size to

60 bytes before sending to it.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

hw/net/virtio-net.c | 4 ++++

1 file changed, 4 insertions(+)

object_get_typename(OBJECT(dev)), dev->id, n);

}

+ for (i = 0; i < n->max_queues; i++) {

+ n->nic->ncs[i].do_not_pad = true;

+ }

+

peer_test_vnet_hdr(n);

if (peer_has_vnet_hdr(n)) {

for (i = 0; i < n->max_queues; i++) {

From: Lukas Straub <lukasstraub2@web.de>

Additional to removing the packet from the secondary queue,

we also need to free it.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

Signed-off-by: Zhang Chen <chen.zhang@intel.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

net/colo-compare.c | 1 +

1 file changed, 1 insertion(+)

@@ -XXX,XX +XXX,XX @@ static void colo_compare_packet(CompareState *s, Connection *conn,

colo_release_primary_pkt(s, pkt);

+ packet_destroy(result->data, NULL);

g_queue_remove(&conn->secondary_list, result->data);

} else {

/*

From: Lukas Straub <lukasstraub2@web.de>

g_queue_remove needs to look up the list entry first, but we

already have it as result and can remove it directly with

g_queue_delete_link.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

Signed-off-by: Zhang Chen <chen.zhang@intel.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

net/colo-compare.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

@@ -XXX,XX +XXX,XX @@ static void colo_compare_packet(CompareState *s, Connection *conn,

if (result) {

colo_release_primary_pkt(s, pkt);

packet_destroy(result->data, NULL);

- g_queue_remove(&conn->secondary_list, result->data);

+ g_queue_delete_link(&conn->secondary_list, result);

} else {

/*

* If one packet arrive late, the secondary_list or

From: Philippe Mathieu-Daudé <philmd@redhat.com>

The in6_address comes after the ip6_ext_hdr_routing header,

not after the ip6_ext_hdr one. Fix the offset.

Cc: qemu-stable@nongnu.org

Reported-by: Stefano Garzarella <sgarzare@redhat.com>

Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

net/eth.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/eth.c b/net/eth.c

--- a/net/eth.c

+++ b/net/eth.c

@@ -XXX,XX +XXX,XX @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,

}

bytes_read = iov_to_buf(pkt, pkt_frags,

- rthdr_offset + sizeof(*ext_hdr),

+ rthdr_offset + sizeof(*rthdr),

dst_addr, sizeof(*dst_addr));

return bytes_read == sizeof(*dst_addr);

From: Philippe Mathieu-Daudé <philmd@redhat.com>

The length field is already contained in the ip6_ext_hdr structure.

Check it direcly in eth_parse_ipv6_hdr() before calling

_eth_get_rss_ex_dst_addr(), which gets a bit simplified.

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

net/eth.c | 14 +++++++-------

diff --git a/net/eth.c b/net/eth.c

--- a/net/eth.c

+++ b/net/eth.c

@@ -XXX,XX +XXX,XX @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,

{

struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;

- if ((rthdr->rtype == 2) &&

- (rthdr->len == sizeof(struct in6_address) / 8) &&

- (rthdr->segleft == 1)) {

+ if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {

size_t input_size = iov_size(pkt, pkt_frags);

size_t bytes_read;

@@ -XXX,XX +XXX,XX @@ bool eth_parse_ipv6_hdr(const struct iovec *pkt, int pkt_frags,

if (curr_ext_hdr_type == IP6_ROUTING) {

- info->rss_ex_dst_valid =

- _eth_get_rss_ex_dst_addr(pkt, pkt_frags,

- ip6hdr_off + info->full_hdr_len,

- &ext_hdr, &info->rss_ex_dst);

+ if (ext_hdr.ip6r_len == sizeof(struct in6_address) / 8) {

+ info->rss_ex_dst_valid =

+ _eth_get_rss_ex_dst_addr(pkt, pkt_frags,

+ ip6hdr_off + info->full_hdr_len,

+ &ext_hdr, &info->rss_ex_dst);

+ }

} else if (curr_ext_hdr_type == IP6_DESTINATON) {

info->rss_ex_src_valid =

_eth_get_rss_ex_src_addr(pkt, pkt_frags,

From: Philippe Mathieu-Daudé <philmd@redhat.com>

The 'offset' argument represents the offset to the ip6_ext_hdr

header, rename it as 'ext_hdr_offset'.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

net/eth.c | 6 +++---

diff --git a/net/eth.c b/net/eth.c

--- a/net/eth.c

+++ b/net/eth.c

@@ -XXX,XX +XXX,XX @@ eth_is_ip6_extension_header_type(uint8_t hdr_type)

static bool

_eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,

- size_t rthdr_offset,

+ size_t ext_hdr_offset,

struct ip6_ext_hdr *ext_hdr,

struct in6_address *dst_addr)

@@ -XXX,XX +XXX,XX @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,

size_t input_size = iov_size(pkt, pkt_frags);

size_t bytes_read;

- if (input_size < rthdr_offset + sizeof(*ext_hdr)) {

+ if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {

return false;

}

bytes_read = iov_to_buf(pkt, pkt_frags,

- rthdr_offset + sizeof(*rthdr),

+ ext_hdr_offset + sizeof(*rthdr),

dst_addr, sizeof(*dst_addr));

return bytes_read == sizeof(*dst_addr);

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

net/eth.c | 14 ++++++--------

1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/eth.c b/net/eth.c

--- a/net/eth.c

+++ b/net/eth.c

@@ -XXX,XX +XXX,XX @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,

struct in6_address *dst_addr)

struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;

+ size_t input_size = iov_size(pkt, pkt_frags);

+ size_t bytes_read;

- if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {

-

- size_t input_size = iov_size(pkt, pkt_frags);

- size_t bytes_read;

-

- if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {

- return false;

- }

+ if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {

+ return false;

+ }

+ if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {

bytes_read = iov_to_buf(pkt, pkt_frags,

ext_hdr_offset + sizeof(*rthdr),

dst_addr, sizeof(*dst_addr));

From: Philippe Mathieu-Daudé <philmd@redhat.com>

We want to check fields from ip6_ext_hdr_routing structure

and if correct read the full in6_address. Let's directly check

if our iovec contains enough data for everything, else return

early.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

net/eth.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/eth.c b/net/eth.c

--- a/net/eth.c

+++ b/net/eth.c

@@ -XXX,XX +XXX,XX @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,

size_t input_size = iov_size(pkt, pkt_frags);

size_t bytes_read;

- if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {

+ if (input_size < ext_hdr_offset + sizeof(*rthdr) + sizeof(*dst_addr)) {

return false;

From: Philippe Mathieu-Daudé <philmd@redhat.com>

We can't know the caller read enough data in the memory pointed

by ext_hdr to cast it as a ip6_ext_hdr_routing.

Declare rt_hdr on the stack and fill it again from the iovec.

Since we already checked there is enough data in the iovec buffer,

simply add an assert() call to consume the bytes_read variable.

This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported

by QEMU fuzzer:

$ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \

-accel qtest -monitor none \

-serial none -nographic -qtest stdio

outl 0xcf8 0x80001010

outl 0xcfc 0xe1020000

outl 0xcf8 0x80001004

outw 0xcfc 0x7

write 0x25 0x1 0x86

write 0x26 0x1 0xdd

write 0x4f 0x1 0x2b

write 0xe1020030 0x4 0x190002e1

write 0xe102003a 0x2 0x0807

write 0xe1020048 0x4 0x12077cdd

write 0xe1020400 0x4 0xba077cdd

write 0xe1020420 0x4 0x190002e1

write 0xe1020428 0x4 0x3509d807

write 0xe1020438 0x1 0xe2

EOF

=================================================================

==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818

READ of size 1 at 0x7ffdef904902 thread T0

#0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17

#1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17

#2 0x561cef7de639 in net_tx_pkt_parse_headers hw/net/net_tx_pkt.c:228:14

#3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9

#4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29