From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3699C43A3F; Thu, 1 Feb 2024 17:42:54 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0C82142E89; Thu, 1 Feb 2024 17:42:54 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 05FD540608 for ; Thu, 1 Feb 2024 17:42:53 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 5A8F720B2000; Thu, 1 Feb 2024 08:42:52 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 5A8F720B2000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1706805772; bh=P/AElz06B4OWZaMzz7whUGvOdt/WKq9erXzBK/yhQE4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IElMpx9VmcGgFC8y16MxFgG5jVezX/rJcODvgglLojwLwbSzgztyEf3iAKQhPT1PG kqndDXwP2ioR7cKBmFZ7y1HGKkRertAbUuIBNT1fcH++DXuTsQlqSeb/4iEC2A9ZuB KDVmaO59V3slrkO/vGF29W5+WF9fOknmhrtq/Uto= Date: Thu, 1 Feb 2024 08:42:52 -0800 From: Tyler Retzlaff To: David Marchand Cc: Bruce Richardson , "lihuisong (C)" , dev@dpdk.org, Ciara Power Subject: Re: [PATCH] telemetry: avoid truncation of strlcpy return before check Message-ID: <20240201164252.GA18071@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1691011261-5666-1-git-send-email-roretzla@linux.microsoft.com> <35199239-fac5-f7f2-6f80-5070b016d7d6@huawei.com> <20230808175937.GA13736@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Feb 01, 2024 at 12:45:43PM +0100, David Marchand wrote: > On Tue, Aug 8, 2023 at 8:35 PM Bruce Richardson > wrote: > > > > On Tue, Aug 08, 2023 at 10:59:37AM -0700, Tyler Retzlaff wrote: > > > On Tue, Aug 08, 2023 at 10:24:41AM +0800, lihuisong (C) wrote: > > > > > > > > 在 2023/8/3 5:21, Tyler Retzlaff 写道: > > > > >strlcpy returns type size_t when directly assigning to > > > > >struct rte_tel_data data_len field it may be truncated leading to > > > > >compromised length check that follows > > > > > > > > > >Since the limit in the check is < UINT_MAX the value returned is > > > > >safe to be cast to unsigned int (which may be narrower than size_t) > > > > >but only after being checked against RTE_TEL_MAX_SINGLE_STRING_LEN > > > > > > > > > >Signed-off-by: Tyler Retzlaff > > > > >--- > > > > > lib/telemetry/telemetry_data.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > >diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c > > > > >index 3b1a240..52307cb 100644 > > > > >--- a/lib/telemetry/telemetry_data.c > > > > >+++ b/lib/telemetry/telemetry_data.c > > > > >@@ -41,12 +41,13 @@ > > > > > int > > > > > rte_tel_data_string(struct rte_tel_data *d, const char *str) > > > > > { > > > > >+ const size_t len = strlcpy(d->data.str, str, sizeof(d->data.str)); > > > > sizeof(d->data.str) is equal to RTE_TEL_MAX_SINGLE_STRING_LEN(8192). > > > > So It seems that this truncation probably will not happen. > > > > > > agreed, regardless the data type choices permit a size that exceeds the > > > range of the narrower type and the assignment results in a warning being > > > generated on some targets. that's why the truncating cast is safe to > > > add. > > > > > > none of this would be necessary if data_len had been appropriately typed > > > as size_t. Bruce should we be changing the type instead since we are in > > > 23.11 merge window...? > > > > > I'm fine either way, to be honest. > > Can we conclude? > struct rte_tel_data seems internal (at least opaque from an > application pov), so I suppose the option of changing data_len to > size_t is still on the table. > > And we are missing a Fixes: tag too. there is actually a general pattern of this problem across dpdk tree and this fixes one instance. i've marked the patch as rejected for now and hope to come back with a more comprehensive series after msvc work is merged. ty > > Thanks. > > -- > David Marchand