Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 25 Aug 2022 17:32:01 GMT
From:      John Baldwin <jhb@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 2d349fd22c97 - stable/12 - bhyve e1000: Skip packets with a small header.
Message-ID:  <202208251732.27PHW1r8045388@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/12 has been updated by jhb:

URL: https://cgit.FreeBSD.org/src/commit/?id=2d349fd22c97e34e5cf4e8e2ac9454d062fb7a61

commit 2d349fd22c97e34e5cf4e8e2ac9454d062fb7a61
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2022-08-17 17:01:16 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2022-08-25 16:39:59 +0000

    bhyve e1000: Skip packets with a small header.
    
    Certain operations such as checksum insertion and VLAN insertion
    require the device model to rewrite the packet header.  The first step
    in rewriting the packet header is to copy the existing packet header
    from the source packet.  This copy is done by copying data from an
    iovec array that corresponds to the S/G entries described by transmit
    descriptors.  However, if the total packet length is smaller than the
    headers that need to be copied as the initial template, this copy can
    overflow the iovec array and use garbage values as the source pointer
    to memcpy.  The PR used a single descriptor with a length of 0 in its
    PoC.
    
    To fix, track the total packet length and drop requests to transmit
    packets whose payload is smaller than the required header length.
    
    While here, fix another issue where the final descriptor could have an
    invalid length (too short) that could underflow 'len' when stripping
    the checksum.  Skip those requests instead, too.
    
    PR:             264372
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Reviewed by:    grehan, markj
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D36182
    
    (cherry picked from commit fa46f3704b7618f9d9493c126df781faf59040a8)
---
 usr.sbin/bhyve/pci_e82545.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/usr.sbin/bhyve/pci_e82545.c b/usr.sbin/bhyve/pci_e82545.c
index aeef3f84ee5d..0b46ea5dced7 100644
--- a/usr.sbin/bhyve/pci_e82545.c
+++ b/usr.sbin/bhyve/pci_e82545.c
@@ -230,7 +230,7 @@ struct ck_info {
  * Debug printf
  */
 static int e82545_debug = 0;
-#define WPRINTF(msg,params...) PRINTLN("e82545: " msg, params)
+#define WPRINTF(msg,params...) PRINTLN("e82545: " msg, ##params)
 #define DPRINTF(msg,params...) if (e82545_debug) WPRINTF(msg, params)
 
 #define	MIN(a,b) (((a)<(b))?(a):(b))
@@ -1083,15 +1083,18 @@ e82545_transmit(struct e82545_softc *sc, uint16_t head, uint16_t tail,
 	union  e1000_tx_udesc *dsc;
 	int desc, dtype, len, ntype, iovcnt, tcp, tso;
 	int mss, paylen, seg, tiovcnt, left, now, nleft, nnow, pv, pvoff;
-	unsigned hdrlen, vlen;
+	unsigned hdrlen, vlen, pktlen;
 	uint32_t tcpsum, tcpseq;
 	uint16_t ipcs, tcpcs, ipid, ohead;
+	bool invalid;
 
 	ckinfo[0].ck_valid = ckinfo[1].ck_valid = 0;
 	iovcnt = 0;
 	ntype = 0;
 	tso = 0;
+	pktlen = 0;
 	ohead = head;
+	invalid = false;
 
 	/* iovb[0/1] may be used for writable copy of headers. */
 	iov = &iovb[2];
@@ -1141,17 +1144,23 @@ e82545_transmit(struct e82545_softc *sc, uint16_t head, uint16_t tail,
 		len = (dtype == E1000_TXD_TYP_L) ? dsc->td.lower.flags.length :
 		    dsc->dd.lower.data & 0xFFFFF;
 
-		if (len > 0) {
-			/* Strip checksum supplied by guest. */
-			if ((dsc->td.lower.data & E1000_TXD_CMD_EOP) != 0 &&
-			    (dsc->td.lower.data & E1000_TXD_CMD_IFCS) == 0)
+		/* Strip checksum supplied by guest. */
+		if ((dsc->td.lower.data & E1000_TXD_CMD_EOP) != 0 &&
+		    (dsc->td.lower.data & E1000_TXD_CMD_IFCS) == 0) {
+			if (len <= 2) {
+				WPRINTF("final descriptor too short (%d) -- dropped",
+				    len);
+				invalid = true;
+			} else
 				len -= 2;
-			if (iovcnt < I82545_MAX_TXSEGS) {
-				iov[iovcnt].iov_base = paddr_guest2host(
-				    sc->esc_ctx, dsc->td.buffer_addr, len);
-				iov[iovcnt].iov_len = len;
-			}
+		}
+
+		if (len > 0 && iovcnt < I82545_MAX_TXSEGS) {
+			iov[iovcnt].iov_base = paddr_guest2host(sc->esc_ctx,
+			    dsc->td.buffer_addr, len);
+			iov[iovcnt].iov_len = len;
 			iovcnt++;
+			pktlen += len;
 		}
 
 		/*
@@ -1199,6 +1208,9 @@ e82545_transmit(struct e82545_softc *sc, uint16_t head, uint16_t tail,
 		}
 	}
 
+	if (invalid)
+		goto done;
+
 	if (iovcnt > I82545_MAX_TXSEGS) {
 		WPRINTF("tx too many descriptors (%d > %d) -- dropped",
 		    iovcnt, I82545_MAX_TXSEGS);
@@ -1292,8 +1304,13 @@ e82545_transmit(struct e82545_softc *sc, uint16_t head, uint16_t tail,
 		}
 	}
 
+	if (pktlen < hdrlen + vlen) {
+		WPRINTF("packet too small for writable header");
+		goto done;
+	}
+
 	/* Allocate, fill and prepend writable header vector. */
-	if (hdrlen != 0) {
+	if (hdrlen + vlen != 0) {
 		hdr = __builtin_alloca(hdrlen + vlen);
 		hdr += vlen;
 		for (left = hdrlen, hdrp = hdr; left > 0;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202208251732.27PHW1r8045388>