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 0BDCEA04B6; Mon, 12 Oct 2020 13:57:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 860D51D6D6; Mon, 12 Oct 2020 13:57:20 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 113701D6D2; Mon, 12 Oct 2020 13:57:17 +0200 (CEST) IronPort-SDR: 07j6JuvRZ8TKcG1ZynW6kmFEUGWoScxlI6Yb0Kb6dd90YRR17MzdJsOFPNiuAnMjDISk+SxhPB AUm3vGZWH0yQ== X-IronPort-AV: E=McAfee;i="6000,8403,9771"; a="162260359" X-IronPort-AV: E=Sophos;i="5.77,366,1596524400"; d="scan'208";a="162260359" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2020 04:57:16 -0700 IronPort-SDR: SRhPLoyJYMnJqvdkvVMsZ04nwOsJAuj1eXJshYwdI6iTd1yRMob16XszcTg85bbD/jn1yyGcyp cL/zdRLvPqvQ== X-IronPort-AV: E=Sophos;i="5.77,366,1596524400"; d="scan'208";a="520670838" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.244.119]) ([10.213.244.119]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2020 04:57:15 -0700 To: Kevin Laatz , dev@dpdk.org Cc: bruce.richardson@intel.com, stephen@networkplumber.org, stable@dpdk.org References: <20200922172015.266698-1-kevin.laatz@intel.com> <20201001170902.487111-1-kevin.laatz@intel.com> From: Ferruh Yigit Message-ID: <307b8070-977d-187f-c9c0-4b97477eda98@intel.com> Date: Mon, 12 Oct 2020 12:57:11 +0100 MIME-Version: 1.0 In-Reply-To: <20201001170902.487111-1-kevin.laatz@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] net/ring: fix unchecked return value 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" On 10/1/2020 6:09 PM, Kevin Laatz wrote: > Add a check for the return value of the sscanf call in > parse_internal_args(), returning an error if we don't get the expected > result. > > Coverity issue: 362049 > Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API") > Cc: stable@dpdk.org > > Signed-off-by: Kevin Laatz > > --- > v2: added consumed characters count check > --- > drivers/net/ring/rte_eth_ring.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c > index 40fe1ca4ba..66367465fd 100644 > --- a/drivers/net/ring/rte_eth_ring.c > +++ b/drivers/net/ring/rte_eth_ring.c > @@ -538,8 +538,13 @@ parse_internal_args(const char *key __rte_unused, const char *value, > { > struct ring_internal_args **internal_args = data; > void *args; > + int n; > > - sscanf(value, "%p", &args); > + if (sscanf(value, "%p%n", &args, &n) != 1 || (size_t)n != strlen(value)) { two small details, 1- I see following note in the sscanf manual: https://linux.die.net/man/3/sscanf " The C standard says: "Execution of a %n directive does not increment the assignment count returned at the completion of execution" but the Corrigendum seems to contradict this. Probably it is wise not to make any assumptions on the effect of %n conversions on the return value. " So what do you think checking return value as " == 0" ? 2) If the 'value' is more than a pointer can hold, like "0xbbbbbbbbbbbbbbbbbb", the arg will be '0xffffffffffffffff', the 'n' will be 20. The "(size_t)n != strlen(value)" check doesn't catch this. What do you think adding another "strnlen(value, 18)", since 18 can be the largest pointer, even before 'sscanf()' ? This also protects against strlen with non-null terminated 'value'. > + PMD_LOG(ERR, "Error parsing internal args"); > + > + return -1; > + } > > *internal_args = args; > >