From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202412 header.b=mLwG70hB; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id D3ADE5A061C for ; Fri, 20 Dec 2024 09:58:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202412; t=1734685098; bh=bC2MgRq/qxScZBP875rBDe8JAq3gfD5Fcw9ST4gamFk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=mLwG70hBo3KRHl+/CHAAaoFnmuxBU4FPLnqiNdHl2VxRU1qTzArVpcBbLhU/OJI4w EkFnt16CcN8tmfDL+HVwxfCD/4x+K9VAJ+okpg9eo9bWK1B2zA1PYPjHV5OhosYMZ/ XCI8u5ZGLrbZVtNdbI/Ayupm1lSdvOB/ci7RMh3DTnj9DcvAX9qYeDpTvZ1B4KCFXB OxqIv7r8Oe2a9vX0lyXUMfggizUX/L+tginSYd8px5kJEjzmQ6BrWUg/u5/GYNbGxG hxoMsfdYffk8ypmFnPBGvIA0jGol+mPNIhGzpq9Z3vIlWVd+ksu6n8JMsta/Z/klme J/LVTI8KtIDEQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YF1Xp1gBBz4xfF; Fri, 20 Dec 2024 19:58:18 +1100 (AEDT) From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: [PATCH v2 12/12] packet: More cautious checks to avoid pointer arithmetic UB Date: Fri, 20 Dec 2024 19:35:35 +1100 Message-ID: <20241220083535.1372523-13-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.47.1 In-Reply-To: <20241220083535.1372523-1-david@gibson.dropbear.id.au> References: <20241220083535.1372523-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: PB4K5M7ZTVBLKRXJ445C2XTSHV7JTXUB X-Message-ID-Hash: PB4K5M7ZTVBLKRXJ445C2XTSHV7JTXUB 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: packet_check_range (and vu_packet_check_range()) verify that the packet or section of packet we're interested in lies in the packet buffer pool we expect it to. However, in doing so it doesn't avoid the possibility of an integer overflow while performing pointer arithmetic, with is UB. In fact, AFAICT it's UB even to use arbitrary pointer arithmetic to construct a pointer outside of a known valid buffer. To do this safely, we can't calculate the end of a memory region with pointer addition, at least if we're treating the length as untrusted. Instead we must work out the offset of one memory region within another using pointer subtraction, then do integer checks against the length of the outer region. We then need to be careful about the order of checks so that those integer checks can't themselves overflow. While addressing this, correct a flaw in vu_packet_check_range() where it only checked that the given packet piece was above the region's mmap address, not the base of the specific section of the mmap we expect to find it in (dev_region->mmap_addr + dev_region->mmap_offset). Signed-off-by: David Gibson --- packet.c | 9 ++++++--- vu_common.c | 15 +++++++++++---- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/packet.c b/packet.c index c3b80924..04a7a829 100644 --- a/packet.c +++ b/packet.c @@ -49,9 +49,12 @@ static void packet_check_range(const struct pool *p, ASSERT_WITH_MSG(ptr >= p->buf, "packet range start %p before buffer start %p, %s:%i", (void *)ptr, (void *)p->buf, func, line); - ASSERT_WITH_MSG(ptr + len <= p->buf + p->buf_size, - "packet range end %p after buffer end %p, %s:%i", - (void *)(ptr + len), (void *)(p->buf + p->buf_size), + ASSERT_WITH_MSG(len <= p->buf_size, + "packet range length %zu larger than buffer %zu, %s:%i", + len, p->buf_size, func, line); + ASSERT_WITH_MSG((size_t)(ptr - p->buf) <= p->buf_size - len, +"packet range %p, len %zu extends beyond buffer %p, len %zu, %s:%i", + (void *)ptr, len, (void *)p->buf, p->buf_size, func, line); } diff --git a/vu_common.c b/vu_common.c index 6f49f873..3f7fb29d 100644 --- a/vu_common.c +++ b/vu_common.c @@ -35,16 +35,23 @@ void vu_packet_check_range(void *buf, const char *ptr, size_t len, struct vu_dev_region *dev_region; for (dev_region = buf; dev_region->mmap_addr; dev_region++) { + uintptr_t base_addr = dev_region->mmap_addr + + dev_region->mmap_offset; /* NOLINTNEXTLINE(performance-no-int-to-ptr) */ - char *m = (char *)(uintptr_t)dev_region->mmap_addr; + const char *base = (const char *)base_addr; - if (m <= ptr && - ptr + len <= m + dev_region->mmap_offset + dev_region->size) + ASSERT(base_addr >= dev_region->mmap_addr); + + if (len > dev_region->size) + continue; + + if (base <= ptr && + (size_t)(ptr - base) <= dev_region->size - len) return; } abort_with_msg( - "package range at %p, length %zd not within dev region %s:%i", + "packet range at %p, length %zd not within dev region, %s:%i", (void *)ptr, len, func, line); } -- 2.47.1