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 7E7F42C28; Wed, 13 Mar 2019 15:35:56 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BE54131028D4; Wed, 13 Mar 2019 14:35:55 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DB72A5D77C; Wed, 13 Mar 2019 14:35:54 +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> <96fe0a31-215e-e6e6-96b0-540ca666951b@intel.com> Date: Wed, 13 Mar 2019 10:35:54 -0400 In-Reply-To: <96fe0a31-215e-e6e6-96b0-540ca666951b@intel.com> (Ferruh Yigit's message of "Wed, 13 Mar 2019 14:07:37 +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.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Wed, 13 Mar 2019 14:35:55 +0000 (UTC) Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] test/eventdev: fix sprintf with snprintf 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: , X-List-Received-Date: Wed, 13 Mar 2019 14:35:57 -0000 Ferruh Yigit writes: > On 3/13/2019 1:43 PM, Aaron Conole wrote: >> 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? > > These patches focus on preventing possible buffer overflow, the impact of > possible truncation changes case by case I think, like for this case I don't see > much benefit of adding return value check. > > For all cases I expect truncation trigger a functional error which should be > already handled properly, like in this case 'rte_vdev_init()' will fail in > second call if buffer is small. And give the user a bad error ("I said net_null1038123825, not net_null10 - bug in dpdk!"). > There may be cases to check the return value, but that should be the case with > 'sprintf' as well, changing API to 'snprintf' shouldn't require additional check > by default. I agree, that's true. I think it's the right thing to do here, though.