[Qemu-devel] [PATCH 0/3] rtl8139: cleanups

[Qemu-devel] [PATCH 0/3] rtl8139: cleanups

[Stefan Hajnoczi]

After addressing CVE-2015-5165 there were several additional rtl8139 cleanups
that I'm sending separately since they are not security fixes.

These patches eliminate duplicate eth.h structs/macros and fix unaligned memory
accesses in tx offload.

Stefan Hajnoczi (3):
  rtl8139: remove duplicate net/eth.h definitions
  rtl8139: use net/eth.h macros instead of custom macros
  rtl8139: use ldl/stl wrapper for unaligned 32-bit access

 hw/net/rtl8139.c | 103 +++++++++++++++----------------------------------------
 1 file changed, 27 insertions(+), 76 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/3] rtl8139: remove duplicate net/eth.h definitions

[Stefan Hajnoczi]

The transmit offload features inspect Ethernet, IP, TCP, and UDP
headers.  Avoid redefining these net/eth.h structs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/rtl8139.c | 57 +++++---------------------------------------------------
 1 file changed, 5 insertions(+), 52 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index edbb61c..6de94d9 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -56,6 +56,7 @@
 #include "sysemu/dma.h"
 #include "qemu/timer.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "hw/loader.h"
 #include "sysemu/sysemu.h"
 #include "qemu/iov.h"
@@ -75,7 +76,6 @@
 #define ETHER_ADDR_LEN 6
 #define ETHER_TYPE_LEN 2
 #define ETH_HLEN (ETHER_ADDR_LEN * 2 + ETHER_TYPE_LEN)
-#define ETH_P_IP    0x0800      /* Internet Protocol packet */
 #define ETH_P_8021Q 0x8100      /* 802.1Q VLAN Extended Header  */
 #define ETH_MTU     1500
 
@@ -1868,55 +1868,8 @@ static int rtl8139_transmit_one(RTL8139State *s, int descriptor)
 }
 
 /* structures and macros for task offloading */
-typedef struct ip_header
-{
-    uint8_t  ip_ver_len;    /* version and header length */
-    uint8_t  ip_tos;        /* type of service */
-    uint16_t ip_len;        /* total length */
-    uint16_t ip_id;         /* identification */
-    uint16_t ip_off;        /* fragment offset field */
-    uint8_t  ip_ttl;        /* time to live */
-    uint8_t  ip_p;          /* protocol */
-    uint16_t ip_sum;        /* checksum */
-    uint32_t ip_src,ip_dst; /* source and dest address */
-} ip_header;
-
-#define IP_HEADER_VERSION_4 4
-#define IP_HEADER_VERSION(ip) ((ip->ip_ver_len >> 4)&0xf)
 #define IP_HEADER_LENGTH(ip) (((ip->ip_ver_len)&0xf) << 2)
 
-typedef struct tcp_header
-{
-    uint16_t th_sport;		/* source port */
-    uint16_t th_dport;		/* destination port */
-    uint32_t th_seq;			/* sequence number */
-    uint32_t th_ack;			/* acknowledgement number */
-    uint16_t th_offset_flags; /* data offset, reserved 6 bits, TCP protocol flags */
-    uint16_t th_win;			/* window */
-    uint16_t th_sum;			/* checksum */
-    uint16_t th_urp;			/* urgent pointer */
-} tcp_header;
-
-typedef struct udp_header
-{
-    uint16_t uh_sport; /* source port */
-    uint16_t uh_dport; /* destination port */
-    uint16_t uh_ulen;  /* udp length */
-    uint16_t uh_sum;   /* udp checksum */
-} udp_header;
-
-typedef struct ip_pseudo_header
-{
-    uint32_t ip_src;
-    uint32_t ip_dst;
-    uint8_t  zeros;
-    uint8_t  ip_proto;
-    uint16_t ip_payload;
-} ip_pseudo_header;
-
-#define IP_PROTO_TCP 6
-#define IP_PROTO_UDP 17
-
 #define TCP_HEADER_DATA_OFFSET(tcp) (((be16_to_cpu(tcp->th_offset_flags) >> 12)&0xf) << 2)
 #define TCP_FLAGS_ONLY(flags) ((flags)&0x3f)
 #define TCP_HEADER_FLAGS(tcp) TCP_FLAGS_ONLY(be16_to_cpu(tcp->th_offset_flags))
@@ -2151,12 +2104,12 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
             DPRINTF("+++ C+ mode offloaded task checksum\n");
 
             /* Large enough for Ethernet and IP headers? */
-            if (saved_size < ETH_HLEN + sizeof(ip_header)) {
+            if (saved_size < ETH_HLEN + sizeof(struct ip_header)) {
                 goto skip_offload;
             }
 
             /* ip packet header */
-            ip_header *ip = NULL;
+            struct ip_header *ip = NULL;
             int hlen = 0;
             uint8_t  ip_protocol = 0;
             uint16_t ip_data_len = 0;
@@ -2176,7 +2129,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
             eth_payload_data = saved_buffer + ETH_HLEN;
             eth_payload_len  = saved_size   - ETH_HLEN;
 
-            ip = (ip_header*)eth_payload_data;
+            ip = (struct ip_header*)eth_payload_data;
 
             if (IP_HEADER_VERSION(ip) != IP_HEADER_VERSION_4) {
                 DPRINTF("+++ C+ mode packet has bad IP version %d "
@@ -2186,7 +2139,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
             }
 
             hlen = IP_HEADER_LENGTH(ip);
-            if (hlen < sizeof(ip_header) || hlen > eth_payload_len) {
+            if (hlen < sizeof(struct ip_header) || hlen > eth_payload_len) {
                 goto skip_offload;
             }
 
-- 
2.4.3

[Qemu-devel] [PATCH 2/3] rtl8139: use net/eth.h macros instead of custom macros

[Stefan Hajnoczi]

Eliminate the following "custom" macros since they are just duplicates
of net/eth.h macros under a different name:

  ETHER_ADDR_LEN -> ETH_ALEN
  ETH_P_8021Q -> ETH_P_VLAN
  IP_HEADER_LENGTH -> IP_HDR_GET_LEN
  TCP_FLAG_FIN -> TH_FIN
  TCP_FLAG_PUSH -> TH_PUSH

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/rtl8139.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 6de94d9..36be22b 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -73,10 +73,8 @@
 #define MOD2(input, size) \
     ( ( input ) & ( size - 1 )  )
 
-#define ETHER_ADDR_LEN 6
 #define ETHER_TYPE_LEN 2
-#define ETH_HLEN (ETHER_ADDR_LEN * 2 + ETHER_TYPE_LEN)
-#define ETH_P_8021Q 0x8100      /* 802.1Q VLAN Extended Header  */
+#define ETH_HLEN (ETH_ALEN * 2 + ETHER_TYPE_LEN)
 #define ETH_MTU     1500
 
 #define VLAN_TCI_LEN 2
@@ -1016,8 +1014,8 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
 
         /* write VLAN info to descriptor variables. */
         if (s->CpCmd & CPlusRxVLAN && be16_to_cpup((uint16_t *)
-                &buf[ETHER_ADDR_LEN * 2]) == ETH_P_8021Q) {
-            dot1q_buf = &buf[ETHER_ADDR_LEN * 2];
+                &buf[ETH_ALEN * 2]) == ETH_P_VLAN) {
+            dot1q_buf = &buf[ETH_ALEN * 2];
             size -= VLAN_HLEN;
             /* if too small buffer, use the tailroom added duing expansion */
             if (size < MIN_BUF_SIZE) {
@@ -1058,10 +1056,10 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
 
         /* receive/copy to target memory */
         if (dot1q_buf) {
-            pci_dma_write(d, rx_addr, buf, 2 * ETHER_ADDR_LEN);
-            pci_dma_write(d, rx_addr + 2 * ETHER_ADDR_LEN,
-                          buf + 2 * ETHER_ADDR_LEN + VLAN_HLEN,
-                          size - 2 * ETHER_ADDR_LEN);
+            pci_dma_write(d, rx_addr, buf, 2 * ETH_ALEN);
+            pci_dma_write(d, rx_addr + 2 * ETH_ALEN,
+                          buf + 2 * ETH_ALEN + VLAN_HLEN,
+                          size - 2 * ETH_ALEN);
         } else {
             pci_dma_write(d, rx_addr, buf, size);
         }
@@ -1783,12 +1781,12 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
         return;
     }
 
-    if (dot1q_buf && size >= ETHER_ADDR_LEN * 2) {
+    if (dot1q_buf && size >= ETH_ALEN * 2) {
         iov = (struct iovec[3]) {
-            { .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 },
+            { .iov_base = buf, .iov_len = ETH_ALEN * 2 },
             { .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN },
-            { .iov_base = buf + ETHER_ADDR_LEN * 2,
-                .iov_len = size - ETHER_ADDR_LEN * 2 },
+            { .iov_base = buf + ETH_ALEN * 2,
+                .iov_len = size - ETH_ALEN * 2 },
         };
 
         memcpy(vlan_iov, iov, sizeof(vlan_iov));
@@ -1868,17 +1866,12 @@ static int rtl8139_transmit_one(RTL8139State *s, int descriptor)
 }
 
 /* structures and macros for task offloading */
-#define IP_HEADER_LENGTH(ip) (((ip->ip_ver_len)&0xf) << 2)
-
 #define TCP_HEADER_DATA_OFFSET(tcp) (((be16_to_cpu(tcp->th_offset_flags) >> 12)&0xf) << 2)
 #define TCP_FLAGS_ONLY(flags) ((flags)&0x3f)
 #define TCP_HEADER_FLAGS(tcp) TCP_FLAGS_ONLY(be16_to_cpu(tcp->th_offset_flags))
 
 #define TCP_HEADER_CLEAR_FLAGS(tcp, off) ((tcp)->th_offset_flags &= cpu_to_be16(~TCP_FLAGS_ONLY(off)))
 
-#define TCP_FLAG_FIN  0x01
-#define TCP_FLAG_PUSH 0x08
-
 /* produces ones' complement sum of data */
 static uint16_t ones_complement_sum(uint8_t *data, size_t len)
 {
@@ -2087,7 +2080,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                 bswap16(txdw1 & CP_TX_VLAN_TAG_MASK));
 
             dot1q_buffer = (uint16_t *) dot1q_buffer_space;
-            dot1q_buffer[0] = cpu_to_be16(ETH_P_8021Q);
+            dot1q_buffer[0] = cpu_to_be16(ETH_P_VLAN);
             /* BE + le_to_cpu() + ~cpu_to_le()~ = BE */
             dot1q_buffer[1] = cpu_to_le16(txdw1 & CP_TX_VLAN_TAG_MASK);
         } else {
@@ -2138,7 +2131,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                 goto skip_offload;
             }
 
-            hlen = IP_HEADER_LENGTH(ip);
+            hlen = IP_HDR_GET_LEN(ip);
             if (hlen < sizeof(struct ip_header) || hlen > eth_payload_len) {
                 goto skip_offload;
             }
@@ -2240,7 +2233,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                     /* keep PUSH and FIN flags only for the last frame */
                     if (!is_last_frame)
                     {
-                        TCP_HEADER_CLEAR_FLAGS(p_tcp_hdr, TCP_FLAG_PUSH|TCP_FLAG_FIN);
+                        TCP_HEADER_CLEAR_FLAGS(p_tcp_hdr, TH_PUSH | TH_FIN);
                     }
 
                     /* recalculate TCP checksum */
-- 
2.4.3

[Qemu-devel] [PATCH 3/3] rtl8139: use ldl/stl wrapper for unaligned 32-bit access

[Stefan Hajnoczi]

The tx offload feature accesses a 16-bit aligned TCP header struct.  The
32-bit fields must be accessed using ldl/stl wrappers since some host
architectures fault on unaligned access.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/rtl8139.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 36be22b..366d1b5 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2118,7 +2118,11 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
             DPRINTF("+++ C+ mode has IP packet\n");
 
-            /* not aligned */
+            /* Note on memory alignment: eth_payload_data is 16-bit aligned
+             * since saved_buffer is allocated with g_malloc() and ETH_HLEN is
+             * even.  32-bit accesses must use ldl/stl wrappers to avoid
+             * unaligned accesses.
+             */
             eth_payload_data = saved_buffer + ETH_HLEN;
             eth_payload_len  = saved_size   - ETH_HLEN;
 
@@ -2215,7 +2219,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                     }
 
                     DPRINTF("+++ C+ mode TSO TCP seqno %08x\n",
-                        be32_to_cpu(p_tcp_hdr->th_seq));
+                            ldl_be_p(&p_tcp_hdr->th_seq));
 
                     /* add 4 TCP pseudoheader fields */
                     /* copy IP source and destination fields */
@@ -2271,7 +2275,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                         0, (uint8_t *) dot1q_buffer);
 
                     /* add transferred count to TCP sequence number */
-                    p_tcp_hdr->th_seq = cpu_to_be32(chunk_size + be32_to_cpu(p_tcp_hdr->th_seq));
+                    stl_be_p(&p_tcp_hdr->th_seq,
+                             chunk_size + ldl_be_p(&p_tcp_hdr->th_seq));
                     ++send_count;
                 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/3] rtl8139: cleanups

[Jason Wang]

On 08/03/2015 08:15 PM, Stefan Hajnoczi wrote:
> After addressing CVE-2015-5165 there were several additional rtl8139 cleanups
> that I'm sending separately since they are not security fixes.
>
> These patches eliminate duplicate eth.h structs/macros and fix unaligned memory
> accesses in tx offload.
>
> Stefan Hajnoczi (3):
>   rtl8139: remove duplicate net/eth.h definitions
>   rtl8139: use net/eth.h macros instead of custom macros
>   rtl8139: use ldl/stl wrapper for unaligned 32-bit access
>
>  hw/net/rtl8139.c | 103 +++++++++++++++----------------------------------------
>  1 file changed, 27 insertions(+), 76 deletions(-)
>

Reviewed-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] rtl8139: cleanups

[Stefan Hajnoczi]

On Mon, Aug 03, 2015 at 01:15:54PM +0100, Stefan Hajnoczi wrote:
> After addressing CVE-2015-5165 there were several additional rtl8139 cleanups
> that I'm sending separately since they are not security fixes.
> 
> These patches eliminate duplicate eth.h structs/macros and fix unaligned memory
> accesses in tx offload.
> 
> Stefan Hajnoczi (3):
>   rtl8139: remove duplicate net/eth.h definitions
>   rtl8139: use net/eth.h macros instead of custom macros
>   rtl8139: use ldl/stl wrapper for unaligned 32-bit access
> 
>  hw/net/rtl8139.c | 103 +++++++++++++++----------------------------------------
>  1 file changed, 27 insertions(+), 76 deletions(-)
> 
> -- 
> 2.4.3
> 
> 

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan