From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=c0KG+iOc; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 1D82F5A026F for ; Wed, 01 Jan 2025 22:54:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1735768490; 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=8V9Vhsk9ATSs1md7jm+q8SqKz7KSF8oWdPSe3wJGkRQ=; b=c0KG+iOc/yymVMJf3M3qDKrutnvq4uZxx6Euv//gdc24lDr6ZQHGcqZjsxlqsWUe2cO0Er BrOZDYtRXedb+ZK/JKZiMfQqCr25w/58Uc9DEnrfLUPKqdOHCrhiYzWnAxcPA7Fa5Hx5qs Hx7lIRF7kcwWe07o2UAD1NrsAMxEhqM= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-232-vFsPvDGkMCim3NWvaPwGAA-1; Wed, 01 Jan 2025 16:54:47 -0500 X-MC-Unique: vFsPvDGkMCim3NWvaPwGAA-1 X-Mimecast-MFC-AGG-ID: vFsPvDGkMCim3NWvaPwGAA Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-385e00ebb16so1846263f8f.3 for ; Wed, 01 Jan 2025 13:54:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735768485; x=1736373285; 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=8V9Vhsk9ATSs1md7jm+q8SqKz7KSF8oWdPSe3wJGkRQ=; b=KyPUdwViNG6FDTFYzLVr7t0jXtpI6KLGAWoyyLbsb+eECAwSw4TIDDuN5gIzFpJLIS 9PzAhHa1vPAj7FWAu/sR0C982/uBMkDCirqiyoT9+7mUSd6sVjMSRe0iXIPa0ElVCWv/ A6SxyaQUZXx4NKJhaznGoDdNnycOsI3g4iYX1HRNYCawHfqrGph8K+thrS1SjBx5xtIu QXYSikzSYeuFdZ1Ix3j0Oxpkyc4YcJfCuOPfWInpdFRei2uqfwIdXqZ29me2hoR9hR2D 5ZOYelCImblGqjVRp3o/c7kpIpbM+5XJyDo49Cntfyr22OUFOdkijjTxEIvUXtWfq3As B7fQ== X-Gm-Message-State: AOJu0YwVrIVanPM7kt/bbVvpPQmufwqy3/YTRCpBvkh5xNKZwlzbBPFi nQ32fc3URYdgyDL6DUl/+8YFlfH2XB/Gjg68KmGqi1xiBNXGeMehPck2KseJRCSD6dwHob0hRgn y2Zx54wDBZtenrLn9+tAE6ZaOScAdqlagCMMlEO+gEQ2YzuyTGneLm354qQ== X-Gm-Gg: ASbGnct7AU00HrEd8sho0yoAhDMyQxwQGxyw4j38BkKTsoylXhjxhxsVPUeydX4vzd7 GathWd6X2G046nEHERDNtirSRcGbWunSB9i5+ua9Rm2b4eBYwd1vOpXRBW7Gex92Xz0bm9boJ9C r2dRcZDYMJ6GO2Rlt1Y9NYe86eb22mrqQklxiBA2Bw9IilyWXwbKvpy+0OjT/ZKwpNEZ6zn5ILr KVHiH7VPv12nmr+tApu39ykopZYwIR9SuQJTYMkN974+xmMLzKN9Lnmffvq0//XyyxA X-Received: by 2002:a05:6000:2ae:b0:385:f560:7911 with SMTP id ffacd0b85a97d-38a221f300cmr37233760f8f.10.1735768485207; Wed, 01 Jan 2025 13:54:45 -0800 (PST) X-Google-Smtp-Source: AGHT+IFLyjMv082BySzKr9y5roxE7XgOjcoUwD1CdTBhLci9U5H+n1ueNfTDn4z3JaWvsFFx3QrcHw== X-Received: by 2002:a05:6000:2ae:b0:385:f560:7911 with SMTP id ffacd0b85a97d-38a221f300cmr37233750f8f.10.1735768484758; Wed, 01 Jan 2025 13:54:44 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a47c9b2efsm17035854f8f.91.2025.01.01.13.54.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Jan 2025 13:54:42 -0800 (PST) Date: Wed, 1 Jan 2025 22:54:41 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 09/12] packet: Distinguish severities of different packet_{add,git}_do() errors Message-ID: <20250101225441.00bdb632@elisabeth> In-Reply-To: <20241220083535.1372523-10-david@gibson.dropbear.id.au> References: <20241220083535.1372523-1-david@gibson.dropbear.id.au> <20241220083535.1372523-10-david@gibson.dropbear.id.au> 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-MFC-PROC-ID: zkLI3E54a9EruHY7VgUuNN6IO2gkPLKvhVXz9XRbrc4_1735768486 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: YNLWCTBF2EU5X3SUIWZZQK3UDDCWYA3Q X-Message-ID-Hash: YNLWCTBF2EU5X3SUIWZZQK3UDDCWYA3Q 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 Fri, 20 Dec 2024 19:35:32 +1100 David Gibson wrote: > packet_add() and packet_get() can fail for a number of reasons, and we > currently treat them all basically the same: we log a trace() level message > and for packet_get() we return NULL. However the different causes of > error are quite different and suggest different handling: > > 1) If we run out of space in the pool on add, that's (rightly) non-fatal, > but is an unusual situation which we might want to know about. Promote > it's message to debug() level. > > 2) All packet_check_range() errors and the checks on packet length indicate > a serious problem. Due to either a bug in calling code, or some sort > of memory-clobbering, we've been given addresses or sizes of packets > that are nonsensical. If things are this bad, we shouldn't try to carry > on replace these with asserts. > > 3) Requesting a packet index that doesn't exist in the pool in packet_get() > indicates a bug in the caller - it should always check the index first. > Replace this with an assert. The reasons for 2) and 3) being trace() messages (they should probably be debug() following the same reasoning as 1)) are graceful degradation and security (availability). If we have a packet triggering some sort of "memory clobbering", or an issue in the caller, that might very well be just one packet or a few of them. I think it's a disservice to users to crash in that case, and it has the potential to worsen a security flaw if this packet can be built on purpose. It's also a disservice to package maintainers because it has the potential to turn a harmless issue into an annoying situation with associated time pressure and everything. Those are not warn() calls, by the way, because we don't want to make it too easy, in that (perhaps unlikely) case, to flood logs, and possibly hide information due to rotation. I suppose that the argument for asserts here is so that we discover functional issues as soon as possible, which I understand of course, but it relies on two assumptions that might not hold: 1. the crash will be reported, while a debug() message will go unnoticed. Well, the crash is more noticeable, but that doesn't mean that it will be reported. Users might/will try to hack something around it or try to survive with slirp4netns and all its flaws. On the other hand, a malfunction (say, failed connections) might be equally likely to be reported, along with debug() messages. 2. the check and the assertion themselves are correct. This was not the case for the two most recent severe issues (commit 61c0b0d0f199, bug #105) we discovered through assertions. Issues such as https://github.com/containers/podman/issues/22925, on the other hand, are good examples of how asserts saved us a lot of time and effort later on (compared to, say, "connections stop working after three days"). But I would argue that those are substantially different cases where assertions are checking something that's much more inherent to the implementation. In this case, I would argue that there's no need to crash, so we shouldn't. We can presumably carry on after that, and we're not leaking any resource or sneakily leave behind some condition that will cause apparently unrelated issues at a later time. > 4) On packet_get() requesting a section of the packet beyond its bounds is > correct already. This happens routinely from some callers when they > probe to see if certain parts of the packet are present. At worst it > indicates that the guest has generate a malformed packet, which we > should quietly drop as we already do. > > Signed-off-by: David Gibson > --- > packet.c | 71 ++++++++++++++++++++--------------------------------- > packet.h | 3 ++- > vu_common.c | 13 +++++++--- > 3 files changed, 38 insertions(+), 49 deletions(-) > > diff --git a/packet.c b/packet.c > index c921aa15..24f12448 100644 > --- a/packet.c > +++ b/packet.c > @@ -30,37 +30,27 @@ > * @func: For tracing: name of calling function > * @line: For tracing: caller line of function call > * > - * Return: 0 if the range is valid, -1 otherwise > + * ASSERT()s if the given range isn't valid (within the expected buffer(s) for > + * the pool). > */ > -static int packet_check_range(const struct pool *p, const char *ptr, size_t len, > - const char *func, int line) > +static void packet_check_range(const struct pool *p, > + const char *ptr, size_t len, > + const char *func, int line) > { > if (p->buf_size == 0) { > - int ret; > - > - ret = vu_packet_check_range((void *)p->buf, ptr, len); > - > - if (ret == -1) > - trace("cannot find region, %s:%i", func, line); > - > - return ret; > - } > - > - if (ptr < p->buf) { > - trace("packet range start %p before buffer start %p, %s:%i", > - (void *)ptr, (void *)p->buf, func, line); > - 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), > - func, line); > - return -1; > + vu_packet_check_range((void *)p->buf, ptr, len, func, line); > + return; > } > > - return 0; > + 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), > + func, line); > } > + > /** > * packet_add_do() - Add data as packet descriptor to given pool > * @p: Existing pool > @@ -75,18 +65,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start, > size_t idx = p->count; > > if (idx >= p->size) { > - trace("add packet index %zu to pool with size %zu, %s:%i", > + debug("add packet index %zu to pool with size %zu, %s:%i", > idx, p->size, func, line); > return; > } > > - if (packet_check_range(p, start, len, func, line)) > - return; > + packet_check_range(p, start, len, func, line); > > - if (len > PACKET_MAX_LEN) { > - trace("add packet length %zu, %s:%i", len, func, line); > - return; > - } > + ASSERT_WITH_MSG(len <= PACKET_MAX_LEN, > + "add packet length %zu (max %zu), %s:%i", > + len, PACKET_MAX_LEN, func, line); > > p->pkt[idx].iov_base = (void *)start; > p->pkt[idx].iov_len = len; > @@ -111,16 +99,12 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset, > { > char *ptr; > > - if (idx >= p->size || idx >= p->count) { > - trace("packet %zu from pool size: %zu, count: %zu, %s:%i", > - idx, p->size, p->count, func, line); > - return NULL; > - } > - > - if (len > PACKET_MAX_LEN) { > - trace("packet data length %zu, %s:%i", len, func, line); > - return NULL; > - } > + ASSERT_WITH_MSG(idx < p->size && idx < p->count, > + "packet %zu from pool size: %zu, count: %zu, %s:%i", > + idx, p->size, p->count, func, line); > + ASSERT_WITH_MSG(len <= PACKET_MAX_LEN, > + "packet range length %zu (max %zu), %s:%i", > + len, PACKET_MAX_LEN, func, line); > > if (len + offset > p->pkt[idx].iov_len) { > trace("data length %zu, offset %zu from length %zu, %s:%i", > @@ -130,8 +114,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset, > > ptr = (char *)p->pkt[idx].iov_base + offset; > > - if (packet_check_range(p, ptr, len, func, line)) > - return NULL; > + packet_check_range(p, ptr, len, func, line); > > if (left) > *left = p->pkt[idx].iov_len - offset - len; > diff --git a/packet.h b/packet.h > index f95cda08..b164f77e 100644 > --- a/packet.h > +++ b/packet.h > @@ -31,7 +31,8 @@ struct pool { > struct iovec pkt[]; > }; > > -int vu_packet_check_range(void *buf, const char *ptr, size_t len); > +void vu_packet_check_range(void *buf, const char *ptr, size_t len, > + const char *func, int line); > void packet_add_do(struct pool *p, size_t len, const char *start, > const char *func, int line); > void *packet_get_do(const struct pool *p, const size_t idx, > diff --git a/vu_common.c b/vu_common.c > index 531f8786..528b9b08 100644 > --- a/vu_common.c > +++ b/vu_common.c > @@ -24,10 +24,13 @@ > * @buf: Array of the available memory regions > * @ptr: Start of desired data range > * @size: Length of desired data range > + * @func: For tracing: name of calling function > + * @line: For tracing: caller line of function call If those arguments are mandatory, I would drop the "For tracing" specification. > * > - * Return: 0 if the zone is in a mapped memory region, -1 otherwise > + * Aborts if the zone isn't in any of the device regions > */ > -int vu_packet_check_range(void *buf, const char *ptr, size_t len) > +void vu_packet_check_range(void *buf, const char *ptr, size_t len, > + const char *func, int line) > { > struct vu_dev_region *dev_region; > > @@ -37,10 +40,12 @@ int vu_packet_check_range(void *buf, const char *ptr, size_t len) > > if (m <= ptr && > ptr + len <= m + dev_region->mmap_offset + dev_region->size) > - return 0; > + return; > } > > - return -1; > + abort_with_msg( > + "package range at %p, length %zd not within dev region %s:%i", s/package/packet/ > + (void *)ptr, len, func, line); > } > > /** -- Stefano