From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top, Stefano Brivio <sbrivio@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH 2/4] packet: More cautious checks to avoid pointer arithmetic UB
Date: Thu, 13 Mar 2025 16:40:48 +1100 [thread overview]
Message-ID: <20250313054050.642978-3-david@gibson.dropbear.id.au> (raw)
In-Reply-To: <20250313054050.642978-1-david@gibson.dropbear.id.au>
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 when then 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.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
packet.c | 12 +++++++++---
vu_common.c | 10 +++++++---
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/packet.c b/packet.c
index bcac0375..d1a51a5b 100644
--- a/packet.c
+++ b/packet.c
@@ -52,9 +52,15 @@ static int packet_check_range(const struct pool *p, const char *ptr, size_t len,
return -1;
}
- if (ptr + len > p->buf + p->buf_size) {
- trace("packet range end %p after buffer end %p, %s:%i",
- (void *)(ptr + len), (void *)(p->buf + p->buf_size),
+ if (len > p->buf_size) {
+ trace("packet range length %zu larger than buffer %zu, %s:%i",
+ len, p->buf_size, func, line);
+ return -1;
+ }
+
+ if ((size_t)(ptr - p->buf) > p->buf_size - len) {
+ trace("packet range %p, len %zu after buffer end %p, %s:%i",
+ (void *)ptr, len, (void *)(p->buf + p->buf_size),
func, line);
return -1;
}
diff --git a/vu_common.c b/vu_common.c
index 9eea4f2f..cefe5e20 100644
--- a/vu_common.c
+++ b/vu_common.c
@@ -36,11 +36,15 @@ int 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++) {
- /* NOLINTNEXTLINE(performance-no-int-to-ptr) */
- char *m = (char *)(uintptr_t)dev_region->mmap_addr +
+ uintptr_t base_addr = dev_region->mmap_addr +
dev_region->mmap_offset;
+ /* NOLINTNEXTLINE(performance-no-int-to-ptr) */
+ const char *base = (const char *)base_addr;
+
+ ASSERT(base_addr >= dev_region->mmap_addr);
- if (m <= ptr && ptr + len <= m + dev_region->size)
+ if (len <= dev_region->size && base <= ptr &&
+ (size_t)(ptr - base) <= dev_region->size - len)
return 0;
}
--
@@ -36,11 +36,15 @@ int 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++) {
- /* NOLINTNEXTLINE(performance-no-int-to-ptr) */
- char *m = (char *)(uintptr_t)dev_region->mmap_addr +
+ uintptr_t base_addr = dev_region->mmap_addr +
dev_region->mmap_offset;
+ /* NOLINTNEXTLINE(performance-no-int-to-ptr) */
+ const char *base = (const char *)base_addr;
+
+ ASSERT(base_addr >= dev_region->mmap_addr);
- if (m <= ptr && ptr + len <= m + dev_region->size)
+ if (len <= dev_region->size && base <= ptr &&
+ (size_t)(ptr - base) <= dev_region->size - len)
return 0;
}
--
2.48.1
next prev parent reply other threads:[~2025-03-13 5:40 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-13 5:40 [PATCH 0/4] Improve robustness of calculations related to frame size limits David Gibson
2025-03-13 5:40 ` [PATCH 1/4] vu_common: Tighten vu_packet_check_range() David Gibson
2025-03-13 5:40 ` David Gibson [this message]
2025-03-13 5:40 ` [PATCH 3/4] tap: Make size of pool_tap[46] purely a tuning parameter David Gibson
2025-03-13 5:40 ` [PATCH 4/4] tap: Clarify calculation of TAP_MSGS David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250313054050.642978-3-david@gibson.dropbear.id.au \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).