From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id BB8F22BF5; Wed, 13 Mar 2019 14:43:03 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EAB4A330271; Wed, 13 Mar 2019 13:43:02 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 282CD60CD7; Wed, 13 Mar 2019 13:43:02 +0000 (UTC) From: Aaron Conole To: Ferruh Yigit Cc: "Parthasarathy\, JananeeX M" , "'dev\@dpdk.org'" , "Pattan\, Reshma" , "Rao\, Nikhil" , "'stable\@dpdk.org'" , "Poornima\, PallantlaX" References: <1549449822-412-1-git-send-email-pallantlax.poornima@intel.com> <7AE31235A30B41498D1C31348DC858BD5B534A73@IRSMSX103.ger.corp.intel.com> <7AE31235A30B41498D1C31348DC858BD5B54DCD4@IRSMSX103.ger.corp.intel.com> Date: Wed, 13 Mar 2019 09:43:01 -0400 In-Reply-To: (Ferruh Yigit's message of "Wed, 13 Mar 2019 11:04:59 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 13 Mar 2019 13:43:03 +0000 (UTC) Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Mar 2019 13:43:04 -0000 Ferruh Yigit writes: > On 3/12/2019 2:44 PM, Aaron Conole wrote: >> "Parthasarathy, JananeeX M" writes: >> >>> Hi >>> >>>> -----Original Message----- >>>> From: Parthasarathy, JananeeX M >>>> Sent: Tuesday, February 19, 2019 6:33 PM >>>> To: Aaron Conole ; Poornima, PallantlaX >>>> >>>> Cc: dev@dpdk.org; Pattan, Reshma ; Rao, Nikhil >>>> ; stable@dpdk.org >>>> Subject: RE: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Aaron Conole >>>>> Sent: Saturday, February 09, 2019 2:50 AM >>>>> To: Poornima, PallantlaX >>>>> Cc: dev@dpdk.org; Pattan, Reshma ; Rao, Nikhil >>>>> ; stable@dpdk.org >>>>> Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with >>>>> snprintf >>>>> >>>>> Pallantla Poornima writes: >>>>> >>>>>> sprintf function is not secure as it doesn't check the length of string. >>>>>> More secure function snprintf is used. >>>>>> >>>>>> Fixes: 2a9c83ae3b ("test/eventdev: add multi-ports test") >>>>>> Cc: stable@dpdk.org >>>>>> >>>>>> Signed-off-by: Pallantla Poornima >>>>>> --- >>>>>> test/test/test_event_eth_rx_adapter.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/test/test/test_event_eth_rx_adapter.c >>>>>> b/test/test/test_event_eth_rx_adapter.c >>>>>> index 1d3be82b5..38f5c039f 100644 >>>>>> --- a/test/test/test_event_eth_rx_adapter.c >>>>>> +++ b/test/test/test_event_eth_rx_adapter.c >>>>>> @@ -479,7 +479,8 @@ adapter_multi_eth_add_del(void) >>>>>> /* add the max port for rx_adapter */ >>>>>> port_index = rte_eth_dev_count_total(); >>>>>> for (; port_index < RTE_MAX_ETHPORTS; port_index += 1) { >>>>>> - sprintf(driver_name, "%s%u", "net_null", drv_id); >>>>>> + snprintf(driver_name, sizeof(driver_name), "%s%u", "net_null", >>>>>> + drv_id); >>>>>> err = rte_vdev_init(driver_name, NULL); >>>>>> TEST_ASSERT(err == 0, "Failed driver %s got %d", >>>>>> driver_name, err); >>>>> >>>>> You call this a fix, but it's not possible for the value of drv_id to >>>>> exceed '32' and the buffer size is plenty accommodating for that. Did >>>>> I miss something? What is this fixing? >>>> >>>> It is better practice to use snprintf although in this case buffer will not overflow >>>> as size is big enough to accommodate. The changes were done mainly to >>>> replace sprintf to snprintf. Probably we can remove "fix" line as it is not issue in >>>> this scenario. >>>> >>>> Thanks >>>> M.P.Jananee >>> >>> Please suggest if we can remove "fix" line. >> >> This is a stylistic change, I don't think it's appropriate to call it a >> fix, so I think you can remove the "Fixes" line. >> >> On further reflection, I actually think it will still be wrong. If the >> size buffer is ever changed, what will happen on truncation? We don't >> get an overflow any longer, but we still pass an invalid argument, so I >> don't think this 'fix' is really even a fix. It still has a bug - >> albeit not one that immediately triggers SSP exception or stack >> overflow. >> >> Makes sense? > > Hi Aaron, > > I see your point and I agree that existing code is not broken, it is functioning > well as it is. > > But we are fixing a possible issue, or lets say fixing using less secure API > although it doesn't cause any problem right now. Perhaps we can update the patch > title slightly [1] but I am for keeping the fix and I think it makes sense to > keep "Fixes" tag so that this update can be backported to stable trees. I can get behind changing the sprintf to snprintf, since it is a better API - but it needs to handle the return value properly (otherwise, in this case we will specify an incorrect device). I can even understanding calling it a fix, it's metadata and is probably needed from some kind of compliance anyway. I also understand that this is in test suite, but people usually copy code from test suites and that means the flaw at some point will be propagated. So I still think it should be a version which checks the return code. Otherwise in production if this is copied, and if I can figure out how to overflow the counter knowing the buffer boundaries, then there is a fixed device that will always be chosen. I think it goes for all the other 's/sprintf\(/snprintf\)' replacements, too. Maybe I misunderstand something? > Thanks, > ferruh > > [1] > test/eventdev: fix possible buffer overflow