Analyse header layout in ether_extract_headers().
authorbluhm <bluhm@openbsd.org>
Tue, 13 Feb 2024 13:58:19 +0000 (13:58 +0000)
committerbluhm <bluhm@openbsd.org>
Tue, 13 Feb 2024 13:58:19 +0000 (13:58 +0000)
Several drivers need IPv4 header length and TCP offset for checksum
offload, TSO and LRO.  Accessing these fields directly caused crashes
on sparc64 due to misaligned access.  It cannot be guaranteed that
IP and TCP header is 4 byte aligned in driver level.  Also gcc 4.2.1
assumes that bit fields can be accessed with 32 bit load instructions.

Use memcpy() in ether_extract_headers() to get the bits from IPv4
and TCP header and store the header length in struct ether_extracted.
From there network drivers can esily use it without caring about
alignment and bit shift.  Do some sanity checks with the length
values to prevent that invalid values from evil packets get stored
into hardware registers.  If check fails, clear the pointer to the
header to hide it from the driver.  Add debug prints that help to
figure out the reason for bad packets and provide information when
debugging drivers.

OK mglocker@

sys/dev/pci/if_bnxt.c
sys/dev/pci/if_em.c
sys/dev/pci/if_igc.c
sys/dev/pci/if_ix.c
sys/dev/pci/if_ixl.c
sys/dev/pv/if_vio.c
sys/net/if_ethersubr.c
sys/netinet/if_ether.h

index 4771121..d6d79e9 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_bnxt.c,v 1.45 2024/01/19 03:25:13 jmatthew Exp $   */
+/*     $OpenBSD: if_bnxt.c,v 1.46 2024/02/13 13:58:19 bluhm Exp $      */
 /*-
  * Broadcom NetXtreme-C/E network driver.
  *
@@ -1433,13 +1433,13 @@ bnxt_start(struct ifqueue *ifq)
                                lflags |= TX_BD_LONG_LFLAGS_LSO;
                                hdrsize = sizeof(*ext.eh);
                                if (ext.ip4)
-                                       hdrsize += ext.ip4->ip_hl << 2;
+                                       hdrsize += ext.ip4hlen;
                                else if (ext.ip6)
                                        hdrsize += sizeof(*ext.ip6);
                                else
                                        tcpstat_inc(tcps_outbadtso);
 
-                               hdrsize += ext.tcp->th_off << 2;
+                               hdrsize += ext.tcphlen;
                                txhi->hdr_size = htole16(hdrsize / 2);
 
                                outlen = m->m_pkthdr.ph_mss;
index 2c668af..18a4ce3 100644 (file)
@@ -31,7 +31,7 @@ POSSIBILITY OF SUCH DAMAGE.
 
 ***************************************************************************/
 
-/* $OpenBSD: if_em.c,v 1.371 2024/01/28 18:42:58 mglocker Exp $ */
+/* $OpenBSD: if_em.c,v 1.372 2024/02/13 13:58:19 bluhm Exp $ */
 /* $FreeBSD: if_em.c,v 1.46 2004/09/29 18:28:28 mlaier Exp $ */
 
 #include <dev/pci/if_em.h>
@@ -2433,7 +2433,7 @@ em_tx_ctx_setup(struct em_queue *que, struct mbuf *mp, u_int head,
        vlan_macip_lens |= (sizeof(*ext.eh) << E1000_ADVTXD_MACLEN_SHIFT);
 
        if (ext.ip4) {
-               iphlen = ext.ip4->ip_hl << 2;
+               iphlen = ext.ip4hlen;
 
                type_tucmd_mlhl |= E1000_ADVTXD_TUCMD_IPV4;
                if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) {
index 5deb53d..952a920 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_igc.c,v 1.15 2024/01/23 08:48:12 kevlo Exp $       */
+/*     $OpenBSD: if_igc.c,v 1.16 2024/02/13 13:58:19 bluhm Exp $       */
 /*-
  * SPDX-License-Identifier: BSD-2-Clause
  *
@@ -2028,7 +2028,7 @@ igc_tx_ctx_setup(struct tx_ring *txr, struct mbuf *mp, int prod,
        ether_extract_headers(mp, &ext);
 
        if (ext.ip4) {
-               iphlen = ext.ip4->ip_hl << 2;
+               iphlen = ext.ip4hlen;
 
                type_tucmd_mlhl |= IGC_ADVTXD_TUCMD_IPV4;
                if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) {
index 9b838f4..87a6bfe 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_ix.c,v 1.206 2023/11/10 15:51:20 bluhm Exp $       */
+/*     $OpenBSD: if_ix.c,v 1.207 2024/02/13 13:58:19 bluhm Exp $       */
 
 /******************************************************************************
 
@@ -2502,7 +2502,7 @@ ixgbe_tx_offload(struct mbuf *mp, uint32_t *vlan_macip_lens,
        *vlan_macip_lens |= (ethlen << IXGBE_ADVTXD_MACLEN_SHIFT);
 
        if (ext.ip4) {
-               iphlen = ext.ip4->ip_hl << 2;
+               iphlen = ext.ip4hlen;
 
                if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) {
                        *olinfo_status |= IXGBE_TXD_POPTS_IXSM << 8;
@@ -2542,7 +2542,7 @@ ixgbe_tx_offload(struct mbuf *mp, uint32_t *vlan_macip_lens,
                if (ext.tcp) {
                        uint32_t hdrlen, thlen, paylen, outlen;
 
-                       thlen = ext.tcp->th_off << 2;
+                       thlen = ext.tcphlen;
 
                        outlen = mp->m_pkthdr.ph_mss;
                        *mss_l4len_idx |= outlen << IXGBE_ADVTXD_MSS_SHIFT;
@@ -3277,11 +3277,11 @@ ixgbe_rxeof(struct rx_ring *rxr)
                                        hdrlen += ETHER_VLAN_ENCAP_LEN;
 #endif
                                if (ext.ip4)
-                                       hdrlen += ext.ip4->ip_hl << 2;
+                                       hdrlen += ext.ip4hlen;
                                if (ext.ip6)
                                        hdrlen += sizeof(*ext.ip6);
                                if (ext.tcp) {
-                                       hdrlen += ext.tcp->th_off << 2;
+                                       hdrlen += ext.tcphlen;
                                        tcpstat_inc(tcps_inhwlro);
                                        tcpstat_add(tcps_inpktlro, pkts);
                                } else {
index 1f89dd7..153f367 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_ixl.c,v 1.95 2024/01/07 21:01:45 bluhm Exp $ */
+/*     $OpenBSD: if_ixl.c,v 1.96 2024/02/13 13:58:19 bluhm Exp $ */
 
 /*
  * Copyright (c) 2013-2015, Intel Corporation
@@ -2827,7 +2827,7 @@ ixl_tx_setup_offload(struct mbuf *m0, struct ixl_tx_ring *txr,
                    IXL_TX_DESC_CMD_IIPT_IPV4_CSUM :
                    IXL_TX_DESC_CMD_IIPT_IPV4;
  
-               hlen = ext.ip4->ip_hl << 2;
+               hlen = ext.ip4hlen;
 #ifdef INET6
        } else if (ext.ip6) {
                offload |= IXL_TX_DESC_CMD_IIPT_IPV6;
@@ -2844,10 +2844,12 @@ ixl_tx_setup_offload(struct mbuf *m0, struct ixl_tx_ring *txr,
 
        if (ext.tcp && ISSET(m0->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) {
                offload |= IXL_TX_DESC_CMD_L4T_EOFT_TCP;
-               offload |= (uint64_t)ext.tcp->th_off << IXL_TX_DESC_L4LEN_SHIFT;
+               offload |= (uint64_t)(ext.tcphlen >> 2)
+                   << IXL_TX_DESC_L4LEN_SHIFT;
        } else if (ext.udp && ISSET(m0->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) {
                offload |= IXL_TX_DESC_CMD_L4T_EOFT_UDP;
-               offload |= (sizeof(*ext.udp) >> 2) << IXL_TX_DESC_L4LEN_SHIFT;
+               offload |= (uint64_t)(sizeof(*ext.udp) >> 2)
+                   << IXL_TX_DESC_L4LEN_SHIFT;
        }
 
        if (ISSET(m0->m_pkthdr.csum_flags, M_TCP_TSO)) {
@@ -2855,7 +2857,7 @@ ixl_tx_setup_offload(struct mbuf *m0, struct ixl_tx_ring *txr,
                        struct ixl_tx_desc *ring, *txd;
                        uint64_t cmd = 0, paylen, outlen;
 
-                       hlen += ext.tcp->th_off << 2;
+                       hlen += ext.tcphlen;
 
                        outlen = m0->m_pkthdr.ph_mss;
                        paylen = m0->m_pkthdr.len - ETHER_HDR_LEN - hlen;
index 550c4de..3648e4a 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_vio.c,v 1.29 2023/12/20 09:51:06 jan Exp $ */
+/*     $OpenBSD: if_vio.c,v 1.30 2024/02/13 13:58:19 bluhm Exp $       */
 
 /*
  * Copyright (c) 2012 Stefan Fritsch, Alexander Fiveg.
@@ -765,7 +765,7 @@ again:
                                hdr->csum_offset = offsetof(struct udphdr, uh_sum);
 
                        if (ext.ip4)
-                               hdr->csum_start += ext.ip4->ip_hl << 2;
+                               hdr->csum_start += ext.ip4hlen;
 #ifdef INET6
                        else if (ext.ip6)
                                hdr->csum_start += sizeof(*ext.ip6);
index a77a0d6..2d73253 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_ethersubr.c,v 1.291 2023/07/27 20:21:25 jan Exp $  */
+/*     $OpenBSD: if_ethersubr.c,v 1.292 2024/02/13 13:58:19 bluhm Exp $        */
 /*     $NetBSD: if_ethersubr.c,v 1.19 1996/05/07 02:40:30 thorpej Exp $        */
 
 /*
@@ -140,6 +140,20 @@ didn't get a copy, you may request one from <license@ipv6.nrl.navy.mil>.
 #include <netmpls/mpls.h>
 #endif /* MPLS */
 
+/* #define ETHERDEBUG 1 */
+#ifdef ETHERDEBUG
+int etherdebug = ETHERDEBUG;
+#define DNPRINTF(level, fmt, args...)                                  \
+       do {                                                            \
+               if (etherdebug >= level)                                \
+                       printf("%s: " fmt "\n", __func__, ## args);     \
+       } while (0)
+#else
+#define DNPRINTF(level, fmt, args...)                                  \
+       do { } while (0)
+#endif
+#define DPRINTF(fmt, args...)  DNPRINTF(1, fmt, args)
+
 u_int8_t etherbroadcastaddr[ETHER_ADDR_LEN] =
     { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 u_int8_t etheranyaddr[ETHER_ADDR_LEN] =
@@ -1034,56 +1048,126 @@ ether_e64_to_addr(struct ether_addr *ea, uint64_t e64)
 
 /* Parse different TCP/IP protocol headers for a quick view inside an mbuf. */
 void
-ether_extract_headers(struct mbuf *mp, struct ether_extracted *ext)
+ether_extract_headers(struct mbuf *m0, struct ether_extracted *ext)
 {
        struct mbuf     *m;
-       uint64_t         hlen;
+       size_t           hlen;
        int              hoff;
        uint8_t          ipproto;
        uint16_t         ether_type;
+       /* gcc 4.2.1 on sparc64 may create 32 bit loads on unaligned mbuf */
+       union {
+               u_char  hc_data;
+#if _BYTE_ORDER == _LITTLE_ENDIAN
+               struct {
+                       u_int   hl:4,   /* header length */
+                               v:4;    /* version */
+               } hc_ip;
+               struct {
+                       u_int   x2:4,   /* (unused) */
+                               off:4;  /* data offset */
+               } hc_th;
+#endif
+#if _BYTE_ORDER == _BIG_ENDIAN
+               struct {
+                       u_int   v:4,    /* version */
+                               hl:4;   /* header length */
+               } hc_ip;
+               struct {
+                       u_int   off:4,  /* data offset */
+                               x2:4;   /* (unused) */
+               } hc_th;
+#endif
+       } hdrcpy;
 
        /* Return NULL if header was not recognized. */
        memset(ext, 0, sizeof(*ext));
 
-       if (mp->m_len < sizeof(*ext->eh))
-               return;
+       KASSERT(ISSET(m0->m_flags, M_PKTHDR));
+       ext->paylen = m0->m_pkthdr.len;
 
-       ext->eh = mtod(mp, struct ether_header *);
+       if (m0->m_len < sizeof(*ext->eh)) {
+               DPRINTF("m_len %d, eh %zu", m0->m_len, sizeof(*ext->eh));
+               return;
+       }
+       ext->eh = mtod(m0, struct ether_header *);
        ether_type = ntohs(ext->eh->ether_type);
        hlen = sizeof(*ext->eh);
+       if (ext->paylen < hlen) {
+               DPRINTF("paylen %u, ehlen %zu", ext->paylen, hlen);
+               ext->eh = NULL;
+               return;
+       }
+       ext->paylen -= hlen;
 
 #if NVLAN > 0
        if (ether_type == ETHERTYPE_VLAN) {
-               ext->evh = mtod(mp, struct ether_vlan_header *);
+               if (m0->m_len < sizeof(*ext->evh)) {
+                       DPRINTF("m_len %d, evh %zu",
+                           m0->m_len, sizeof(*ext->evh));
+                       return;
+               }
+               ext->evh = mtod(m0, struct ether_vlan_header *);
                ether_type = ntohs(ext->evh->evl_proto);
                hlen = sizeof(*ext->evh);
+               if (sizeof(*ext->eh) + ext->paylen < hlen) {
+                       DPRINTF("paylen %zu, evhlen %zu",
+                           sizeof(*ext->eh) + ext->paylen, hlen);
+                       ext->evh = NULL;
+                       return;
+               }
+               ext->paylen = sizeof(*ext->eh) + ext->paylen - hlen;
        }
 #endif
 
        switch (ether_type) {
        case ETHERTYPE_IP:
-               m = m_getptr(mp, hlen, &hoff);
-               if (m == NULL || m->m_len - hoff < sizeof(*ext->ip4))
+               m = m_getptr(m0, hlen, &hoff);
+               if (m == NULL || m->m_len - hoff < sizeof(*ext->ip4)) {
+                       DPRINTF("m_len %d, hoff %d, ip4 %zu",
+                           m ? m->m_len : -1, hoff, sizeof(*ext->ip4));
                        return;
+               }
                ext->ip4 = (struct ip *)(mtod(m, caddr_t) + hoff);
 
-               if (ISSET(ntohs(ext->ip4->ip_off), IP_MF|IP_OFFMASK))
+               memcpy(&hdrcpy.hc_data, ext->ip4, 1);
+               hlen = hdrcpy.hc_ip.hl << 2;
+               if (m->m_len - hoff < hlen) {
+                       DPRINTF("m_len %d, hoff %d, iphl %zu",
+                           m ? m->m_len : -1, hoff, hlen);
+                       ext->ip4 = NULL;
                        return;
-
-               hlen = ext->ip4->ip_hl << 2;
+               }
+               if (ext->paylen < hlen) {
+                       DPRINTF("paylen %u, ip4hlen %zu", ext->paylen, hlen);
+                       ext->ip4 = NULL;
+                       return;
+               }
+               ext->ip4hlen = hlen;
+               ext->paylen -= hlen;
                ipproto = ext->ip4->ip_p;
 
+               if (ISSET(ntohs(ext->ip4->ip_off), IP_MF|IP_OFFMASK))
+                       return;
                break;
 #ifdef INET6
        case ETHERTYPE_IPV6:
-               m = m_getptr(mp, hlen, &hoff);
-               if (m == NULL || m->m_len - hoff < sizeof(*ext->ip6))
+               m = m_getptr(m0, hlen, &hoff);
+               if (m == NULL || m->m_len - hoff < sizeof(*ext->ip6)) {
+                       DPRINTF("m_len %d, hoff %d, ip6 %zu",
+                           m ? m->m_len : -1, hoff, sizeof(*ext->ip6));
                        return;
+               }
                ext->ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff);
 
                hlen = sizeof(*ext->ip6);
+               if (ext->paylen < hlen) {
+                       DPRINTF("paylen %u, ip6hlen %zu", ext->paylen, hlen);
+                       ext->ip6 = NULL;
+                       return;
+               }
+               ext->paylen -= hlen;
                ipproto = ext->ip6->ip6_nxt;
-
                break;
 #endif
        default:
@@ -1093,16 +1177,51 @@ ether_extract_headers(struct mbuf *mp, struct ether_extracted *ext)
        switch (ipproto) {
        case IPPROTO_TCP:
                m = m_getptr(m, hoff + hlen, &hoff);
-               if (m == NULL || m->m_len - hoff < sizeof(*ext->tcp))
+               if (m == NULL || m->m_len - hoff < sizeof(*ext->tcp)) {
+                       DPRINTF("m_len %d, hoff %d, tcp %zu",
+                           m ? m->m_len : -1, hoff, sizeof(*ext->tcp));
                        return;
+               }
                ext->tcp = (struct tcphdr *)(mtod(m, caddr_t) + hoff);
+
+               memcpy(&hdrcpy.hc_data, &ext->tcp->th_flags - 1, 1);
+               hlen = hdrcpy.hc_th.off << 2;
+               if (m->m_len - hoff < hlen) {
+                       DPRINTF("m_len %d, hoff %d, thoff %zu",
+                           m ? m->m_len : -1, hoff, hlen);
+                       ext->tcp = NULL;
+                       return;
+               }
+               if (ext->paylen < hlen) {
+                       DPRINTF("paylen %u, tcphlen %zu", ext->paylen, hlen);
+                       ext->tcp = NULL;
+                       return;
+               }
+               ext->tcphlen = hlen;
+               ext->paylen -= hlen;
                break;
 
        case IPPROTO_UDP:
                m = m_getptr(m, hoff + hlen, &hoff);
-               if (m == NULL || m->m_len - hoff < sizeof(*ext->udp))
+               if (m == NULL || m->m_len - hoff < sizeof(*ext->udp)) {
+                       DPRINTF("m_len %d, hoff %d, tcp %zu",
+                           m ? m->m_len : -1, hoff, sizeof(*ext->tcp));
                        return;
+               }
                ext->udp = (struct udphdr *)(mtod(m, caddr_t) + hoff);
+
+               hlen = sizeof(*ext->udp);
+               if (ext->paylen < hlen) {
+                       DPRINTF("paylen %u, udphlen %zu", ext->paylen, hlen);
+                       ext->udp = NULL;
+                       return;
+               }
                break;
        }
+
+       DNPRINTF(2, "%s%s%s%s%s%s ip4h %u, tcph %u, payl %u",
+           ext->eh ? "eh," : "", ext->evh ? "evh," : "",
+           ext->ip4 ? "ip4," : "", ext->ip6 ? "ip6," : "",
+           ext->tcp ? "tcp," : "", ext->udp ? "udp," : "",
+           ext->ip4hlen, ext->tcphlen, ext->paylen);
 }
index ed28944..4f5edd3 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_ether.h,v 1.90 2023/07/27 20:21:25 jan Exp $       */
+/*     $OpenBSD: if_ether.h,v 1.91 2024/02/13 13:58:19 bluhm Exp $     */
 /*     $NetBSD: if_ether.h,v 1.22 1996/05/11 13:00:00 mycroft Exp $    */
 
 /*
@@ -307,6 +307,9 @@ struct ether_extracted {
        struct ip6_hdr                  *ip6;
        struct tcphdr                   *tcp;
        struct udphdr                   *udp;
+       u_int                            ip4hlen;
+       u_int                            tcphlen;
+       u_int                            paylen;
 };
 
 void ether_extract_headers(struct mbuf *, struct ether_extracted *);