From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 83B2FA058B; Wed, 25 Mar 2020 17:31:46 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DFB711C029; Wed, 25 Mar 2020 17:31:45 +0100 (CET) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id 5D77A1BFD4 for ; Wed, 25 Mar 2020 17:31:44 +0100 (CET) Received: by mail-wr1-f68.google.com with SMTP id 65so4003464wrl.1 for ; Wed, 25 Mar 2020 09:31:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=R4LGr8G6tZ4LZGTtNNQqS8vGPqa9azWBF0HoSOk/bmM=; b=T20QjxuO+c0Emit47p4ecuqe2fPSotYqHrSbP0GfgbnYlQxdoPmOgP+bGK9m1S7PS+ p3UqNX5BkKxMdGgyOHGoKDEB0sAdhvcLlTo6fHUK87DY1cX4Yf8z4ocgI0Zh9JmtwtIn RnsV1/hOOChchU8zr4JnlizoUhRKnRWe7oZTPYCpNy5dJRCu+j6AMrud3dd2lvjTGpno oM0im9iMNEw24kgeFblBp/xGo6Q81XzZa8sxxUZJDNLDdzRfUqmTLvBD/5tjHqhsucnx Mgi+EXVdDMDvA7w1O7RRe/Me4YVwQitKDcw+qLs0TDL5/BTBMZOy4UiPRRVB2tk5Vc9P uc2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=R4LGr8G6tZ4LZGTtNNQqS8vGPqa9azWBF0HoSOk/bmM=; b=S7rv0Z2zXdn+IrckvVskQdZkHwgrhLqakJmW1GL2IkjtqMwZDm7z4IMXSXLQmDrA+O 5HMMkP9ZFnvgjWz66AEC8Mj6BZogyIEfXubSCeHE3y2mrhmEP0hq8S4m+G/huxm6hdj0 o6oz0ZmylQUP50QXlWeO9cZd2YMvO2cP9ulYyzXg9vj8vXt3IiGqXHripKHI1snVPs83 NAoeZy/GCHAWCE+wslrH89Zi2cI4XcAZX9v5F52qAiXdTtSh5/tmj2QMWcrGAbjooHpK jBPbeSctd81U8hGVcx1lyxRWNT94UnsjiODLo05HEs3c0baYKnKRB7WQljlIBtyrvMcu yIjA== X-Gm-Message-State: ANhLgQ2EagTOKxWe27+dAi0blFl/QXAawxDajV9vxIcRdYaaqWl4xhEL 7kMz46jZhHIIkMaKGZm4u9/sPw== X-Google-Smtp-Source: ADFU+vvjQD/88TLlI51P67CLDjfKWMapjlfz9NG0e3y9gBZ6R3Rskh+LTG+2fRZKmo1CZTRcXsIA9A== X-Received: by 2002:adf:d088:: with SMTP id y8mr4246392wrh.36.1585153904030; Wed, 25 Mar 2020 09:31:44 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id a2sm4279645wrp.13.2020.03.25.09.31.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Mar 2020 09:31:43 -0700 (PDT) Date: Wed, 25 Mar 2020 17:31:42 +0100 From: Olivier Matz To: wangyunjian Cc: dev@dpdk.org, jerry.lilijun@huawei.com, xudingke@huawei.com, stable@dpdk.org Message-ID: <20200325163142.GE17125@platinum> References: <1584592680-14000-1-git-send-email-wangyunjian@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1584592680-14000-1-git-send-email-wangyunjian@huawei.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, On Thu, Mar 19, 2020 at 12:38:00PM +0800, wangyunjian wrote: > From: Yunjian Wang > > When an input params'value is '[', leading to the 'str' over read > or heap-buffer-overflow. So we can check the 'ctx1' length to avoid > this problem. > > Fixes: cc0579f2339a ("kvargs: support list value") > Cc: stable@dpdk.org > > Signed-off-by: Yunjian Wang > --- > lib/librte_kvargs/rte_kvargs.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c > index d39332999..a1144b90b 100644 > --- a/lib/librte_kvargs/rte_kvargs.c > +++ b/lib/librte_kvargs/rte_kvargs.c > @@ -48,7 +48,8 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params) > str = kvlist->pairs[i].value; > if (str[0] == '[') { > /* Find the end of the list. */ > - while (str[strlen(str) - 1] != ']') { > + while ((str[strlen(str) - 1] != ']') && > + (strlen(ctx1) > 0)) { > /* Restore the comma erased by strtok_r(). */ > str[strlen(str)] = ','; > /* Parse until next comma. */ I would prefer to keep the while condition as is, like this: /* Restore the comma erased by strtok_r(). */ + if (ctx1[0] == '\0') + return -1; /* no closing bracket */ str[strlen(str)] = ','; It avoids an uneeded call to strlen(), and ensure we are returning an error in that case. I also wanted to add a test case, but I realized that kvargs unit tests are broken now. I have done 2 patches to fix them. Do you mind if I send a patchset with these 2 patches + your patch (keeping your signed-off and doing the modification described above), to ensure there is no implicit dependency? Thanks, Olivier