From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 6B6C15A0319 for ; Fri, 26 Jul 2024 09:20:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1721978433; bh=13dTaPleBZja7rEToyDWYbZrd9r7X2InJiEg8dF0rEg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=pJXAqs+VPb09eNoAaucVnur+3an/SIoAwVjSZESEl/sGubPxCEZCoIn7a5ELhzZFz pE3mCBGqBiwmrWRyl4O7Loob7Dml0Ab/wD3JMtcnLfNZlMtl6QOKIwtA9D07RiKsNd xRy2gsvhzSaXWJO1tYJF1eHQiVEfg6FVNbNazYUl7R0YbzaMNiPLhE+z87H6FC/dnX 3fZMXiNWbOLG41rgwKVgBVItuQoC3mIiAhXTq1AWvqWe24+OWk94vBRm7uinN2J1Ta 0c4W+luyiLRCPzW5ERrWTu+0kmQnFyRlz2VxN6CuOoPhbh7uJ7epBBM8T0qwlK+JUF HZEonGRjMRUeA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4WVfKs0jjlz4x3r; Fri, 26 Jul 2024 17:20:33 +1000 (AEST) From: David Gibson To: passt-dev@passt.top, Stefano Brivio Subject: [PATCH 2/5] tap: Don't attempt to carry on if we get a bad frame length from qemu Date: Fri, 26 Jul 2024 17:20:28 +1000 Message-ID: <20240726072031.3941305-3-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.45.2 In-Reply-To: <20240726072031.3941305-1-david@gibson.dropbear.id.au> References: <20240726072031.3941305-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: BD4X5KKEBATDLJRBCIM5ZT3PG7BB53UP X-Message-ID-Hash: BD4X5KKEBATDLJRBCIM5ZT3PG7BB53UP X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: If we receive a too-short or too-long frame from the QEMU socket, currently we try to skip it and carry on. That sounds sensible on first blush, but probably isn't wise in practice. If this happens, either (a) qemu has done something seriously unexpected, or (b) we've received corrupt data over a Unix socket. Or more likely (c), we have a bug elswhere which has put us out of sync with the stream, so we're trying to read something that's not a frame length as a frame length. Neither (b) nor (c) is really salvageable with the same stream. Case (a) might be ok, but we can no longer be confident qemu won't do something else we can't cope with. So, instead of just skipping the frame and trying to carry on, log an error and close the socket. As a bonus, establishing firm bounds on l2len early will allow simplifications to how we deal with the case where a partial frame is recv()ed. Signed-off-by: David Gibson --- tap.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tap.c b/tap.c index a2ae7c2a..08700f65 100644 --- a/tap.c +++ b/tap.c @@ -1013,7 +1013,13 @@ redo: } while (n > (ssize_t)sizeof(uint32_t)) { - ssize_t l2len = ntohl(*(uint32_t *)p); + uint32_t l2len = ntohl(*(uint32_t *)p); + + if (l2len < sizeof(struct ethhdr) || l2len > ETH_MAX_MTU) { + err("Invalid frame size from QEMU (corrupt stream?)"); + tap_sock_reset(c); + return; + } p += sizeof(uint32_t); n -= sizeof(uint32_t); @@ -1027,16 +1033,8 @@ redo: return; } - /* Complete the partial read above before discarding a malformed - * frame, otherwise the stream will be inconsistent. - */ - if (l2len < (ssize_t)sizeof(struct ethhdr) || - l2len > (ssize_t)ETH_MAX_MTU) - goto next; - tap_add_packet(c, l2len, p); -next: p += l2len; n -= l2len; } -- 2.45.2