On Mon, 15 Jul 2024 14:59:42 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Fri, Jul 12, 2024 at 05:32:41PM +0200, Laurent Vivier wrote:That intention is actually pre-existing: look at the function comment (coming from packet_add_do()). Originally, I wanted to implement --trace like that: if no function name was given, no messages would be printed. Then I realised it wasn't really practical and changed to a static logging flag, but I still accidentally left this in commit bb708111833e ("treewide: Packet abstraction with mandatory boundary checks"). Anyway, yes, func is always passed, so there's no need for this check (and sure, there would be no _need_ anyway). We just need to fix the function comments.To be able to manage buffers inside a shared memory provided by a VM via a vhost-user interface, we cannot rely on the fact that buffers are located in a pre-defined memory area and use a base address and a 32bit offset to address them. We need a 64bit address, so replace struct desc by struct iovec and update range checking. Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- packet.c | 84 +++++++++++++++++++++++++++++++------------------------- packet.h | 14 ++-------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/packet.c b/packet.c index ccfc84607709..f7bb523c4ffa 100644 --- a/packet.c +++ b/packet.c @@ -22,6 +22,39 @@ #include "util.h" #include "log.h" +/** + * packet_check_range() - Check if a packet memory range is valid + * @p: Packet pool + * @offset: Offset of data range in packet descriptor + * @len: Length of desired data range + * @start: Start of the packet descriptor + * @func: For tracing: name of calling function, NULL means no trace() + * @line: For tracing: caller line of function call + * + * Return: 0 if the range is valid, -1 otherwise + */ +static int packet_check_range(const struct pool *p, size_t offset, size_t len, + const char *start, const char *func, int line) +{ + if (start < p->buf) { + if (func) {Omitting the message entirely if func is not set doesn't seem correct. I believe printf() should format NULL pointers sanely (typically as "<null>"), so I think you can just leave out this check.> + trace("add packet start %p before buffer start %p, "It's not "add" if it's called from packet_get_do(). As we print the function name anyway, we could drop "add " from this altogether, it should be clear enough....and that's packet_get_do()'s usage, passing a non-zero offset here (stricter check anyway), while:+ "%s:%i", (void *)start, (void *)p->buf, func, line); + } + return -1; + } + + if (start + len + offset > p->buf + p->buf_size) {It's not really clear to me why offset is needed in here. AIUI, offset is used when we want to talk about some piece of a larger packet/frame that's in the buffer. That's useful when we're dissecting packets,but surely we always want the whole frame/whatever to be within the buffer,packet_add_do() calls this with a zero offset, because the whole packet should fit. That is, this check replaces: if (start + len > p->buf + p->buf_size) { from packet_add_do(), and: if (p->pkt[idx].offset + len + offset > p->buf_size) { from packet_get_do(). It looks equivalent to me.so I don't know we need the extra complexity in this helper. I also think we should check for overflow on the LHS here, but that's pre-existing, so it doesn't need to go in this patch.Maybe something is wrong in the caller, but these are sanity checks for security's sake, so if somebody finds out how to reach here with a ludicrously long packet, I think it's preferable to discard the packet rather than crashing and turning whatever issue into a vulnerability.+ if (func) { + trace("packet offset plus length %lu from size %lu, " + "%s:%i", start - p->buf + len + offset, + p->buf_size, func, line); + } + return -1; + } + + return 0; +} /** * packet_add_do() - Add data as packet descriptor to given pool * @p: Existing pool @@ -41,34 +74,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start, return; } - if (start < p->buf) { - trace("add packet start %p before buffer start %p, %s:%i", - (void *)start, (void *)p->buf, func, line); + if (packet_check_range(p, 0, len, start, func, line)) return; - } - - if (start + len > p->buf + p->buf_size) { - trace("add packet start %p, length: %zu, buffer end %p, %s:%i", - (void *)start, len, (void *)(p->buf + p->buf_size), - func, line); - return; - } if (len > UINT16_MAX) { trace("add packet length %zu, %s:%i", len, func, line); return; } -#if UINTPTR_MAX == UINT64_MAX - if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { - trace("add packet start %p, buffer start %p, %s:%i", - (void *)start, (void *)p->buf, func, line); - return; - } -#endif - - p->pkt[idx].offset = start - p->buf; - p->pkt[idx].len = len; + p->pkt[idx].iov_base = (void *)start; + p->pkt[idx].iov_len = len; p->count++; } @@ -96,36 +111,31 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset, return NULL; } - if (len > UINT16_MAX || len + offset > UINT32_MAX) { + if (len > UINT16_MAX) { if (func) { - trace("packet data length %zu, offset %zu, %s:%i", - len, offset, func, line); + trace("packet data length %zu, %s:%i", + len, func, line);Should this be an assert? Seems like something is wrong in the caller, if they're trying to pass in a ludicrously long packet.> } > return NULL; > } > > - if (p->pkt[idx].offset + len + offset > p->buf_size) { > + if (len + offset > p->pkt[idx].iov_len) { > if (func) { > - trace("packet offset plus length %zu from size %zu, " > - "%s:%i", p->pkt[idx].offset + len + offset, > - p->buf_size, func, line); > + trace("data length %zu, offset %zu from length %zu, " > + "%s:%i", len, offset, p->pkt[idx].iov_len, > + func, line); > } > return NULL; > } > > - if (len + offset > p->pkt[idx].len) { > - if (func) { > - trace("data length %zu, offset %zu from length %u, " > - "%s:%i", len, offset, p->pkt[idx].len, > - func, line); > - } > + if (packet_check_range(p, offset, len, p->pkt[idx].iov_base, > + func, line)) > return NULL; > - } > > if (left) > - *left = p->pkt[idx].len - offset - len; > + *left = p->pkt[idx].iov_len - offset - len; > > - return p->buf + p->pkt[idx].offset + offset; > + return (char *)p->pkt[idx].iov_base + offset; > } > > /** > diff --git a/packet.h b/packet.h > index a784b07bbed5..8377dcf678bb 100644 > --- a/packet.h > +++ b/packet.h > @@ -6,16 +6,6 @@ > #ifndef PACKET_H > #define PACKET_H > > -/** > - * struct desc - Generic offset-based descriptor within buffer > - * @offset: Offset of descriptor relative to buffer start, 32-bit limit > - * @len: Length of descriptor, host order, 16-bit limit > - */ > -struct desc { > - uint32_t offset; > - uint16_t len; > -}; > - > /** > * struct pool - Generic pool of packets stored in a buffer > * @buf: Buffer storing packet descriptors > @@ -29,7 +19,7 @@ struct pool { > size_t buf_size; > size_t size; > size_t count; > - struct desc pkt[1]; > + struct iovec pkt[1]; > }; > > void packet_add_do(struct pool *p, size_t len, const char *start, > @@ -54,7 +44,7 @@ struct _name ## _t { \ > size_t buf_size; \ > size_t size; \ > size_t count; \ > - struct desc pkt[_size]; \ > + struct iovec pkt[_size]; \ > } > > #define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size) \-- Stefano