From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id F17965A004E for ; Fri, 19 Jul 2024 23:29:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1721424559; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wo91O2+YgMhqoOlg1cccGwF4uoc1c/D6jX15TelqqdQ=; b=fTEsNdj0NkNY4iqQ+2Ny8Gd1ulaR2wvbAjRrhSZgz46VkCNZnopeYPaenjrZ3UxVK5Ho3T nZBSJ2y9U4kVGsSsul4Ni3mmJ3HgGMks3MLMgdU1h+TjLLfoJpaHbnTOz/PK1gc2vap2pj 8hP97vzftDFHIIg6NlMXdCKKrRZtk10= Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-594-aAW_O6CbOQWx9GMoegx4nw-1; Fri, 19 Jul 2024 17:29:17 -0400 X-MC-Unique: aAW_O6CbOQWx9GMoegx4nw-1 Received: by mail-qt1-f198.google.com with SMTP id d75a77b69052e-44f594e5605so25733141cf.2 for ; Fri, 19 Jul 2024 14:29:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721424557; x=1722029357; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=wo91O2+YgMhqoOlg1cccGwF4uoc1c/D6jX15TelqqdQ=; b=jmn2e3yZoo4Uw6igdxWx43hrmTP9CzkLFRBzuhaykGQAU9Gk1f8ZhNvTAgfsXMNSnR HOZF5aWGCFBF9HbRcL8I78dGKJ92wwJtG8MaCETHc6m/iTS4FnBh9FCX7oOsHtZjHgUV MTHNDBAunaEiIKCvJ2yvXXXPuEr3xQ8kntREQvXwnfVD1PrXH9/eZq8XmcampxYyoPoD Ntf9OYnjF+QO8+BKu/3sjfasTBxaMVzpwtCkLGKgGDLChnbIie39xysCgsyHL2L/OacM G5ZKZH3u2JDZ9CxZvCbrN7N6fV+S1vjTg5rLveLSjd4Cxj/hnBSAyNWvSU9iUtjkjBPw Q+yA== X-Gm-Message-State: AOJu0YxoznAwjka6MbSvoYUefDy+/q14RResoCVzG0dTpoj+SjyU5Tag M1/YzS4S9K0+i+dhf5Zw+dS5hmoagyypZeBHOu//DJpX1Fk3/hvJKmDu3yzwshe7V8rp9v0yQk7 AmQXH02dasfCg/F6GCkzTwHF8cmRZdzovRc41fn4IWIzZ4EMyoA== X-Received: by 2002:ac8:5885:0:b0:447:f5d2:7c5f with SMTP id d75a77b69052e-44fa535eaa1mr15587921cf.53.1721424557106; Fri, 19 Jul 2024 14:29:17 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG7rjfcGivvNfEFb3vIXXiniu4wrjvovVVICTH2iWjxrflim1yEZc5VM/UG7HAS6n7E8upK5Q== X-Received: by 2002:ac8:5885:0:b0:447:f5d2:7c5f with SMTP id d75a77b69052e-44fa535eaa1mr15587701cf.53.1721424556641; Fri, 19 Jul 2024 14:29:16 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-44f9cda35d0sm11445561cf.60.2024.07.19.14.29.15 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 19 Jul 2024 14:29:15 -0700 (PDT) Date: Fri, 19 Jul 2024 23:28:40 +0200 From: Stefano Brivio To: David Gibson , Laurent Vivier Subject: Re: [PATCH v2 1/4] packet: replace struct desc by struct iovec Message-ID: <20240719232840.2ad9f8df@elisabeth> In-Reply-To: References: <20240712153244.831436-1-lvivier@redhat.com> <20240712153244.831436-2-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: QILATYJT7H7WBHMBWWGGQS5YX23WAJN6 X-Message-ID-Hash: QILATYJT7H7WBHMBWWGGQS5YX23WAJN6 X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top 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: On Mon, 15 Jul 2024 14:59:42 +1000 David Gibson wrote: > On Fri, Jul 12, 2024 at 05:32:41PM +0200, Laurent Vivier wrote: > > 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 > > --- > > 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 > ""), so I think you can just leave out this check. 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. > > + 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. > > + "%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, ...and that's packet_get_do()'s usage, passing a non-zero offset here (stricter check anyway), while: > 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. > > > + 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. 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. > > } > > 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