Ferruh Yigit <ferruh.yi...@intel.com> writes: > On 3/12/2019 2:44 PM, Aaron Conole wrote: >> "Parthasarathy, JananeeX M" <jananeex.m.parthasara...@intel.com> writes: >> >>> Hi >>> >>>> -----Original Message----- >>>> From: Parthasarathy, JananeeX M >>>> Sent: Tuesday, February 19, 2019 6:33 PM >>>> To: Aaron Conole <acon...@redhat.com>; Poornima, PallantlaX >>>> <pallantlax.poorn...@intel.com> >>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pat...@intel.com>; Rao, Nikhil >>>> <nikhil....@intel.com>; sta...@dpdk.org >>>> Subject: RE: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Aaron Conole >>>>> Sent: Saturday, February 09, 2019 2:50 AM >>>>> To: Poornima, PallantlaX <pallantlax.poorn...@intel.com> >>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pat...@intel.com>; Rao, Nikhil >>>>> <nikhil....@intel.com>; sta...@dpdk.org >>>>> Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with >>>>> snprintf >>>>> >>>>> Pallantla Poornima <pallantlax.poorn...@intel.com> 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: sta...@dpdk.org >>>>>> >>>>>> Signed-off-by: Pallantla Poornima <pallantlax.poorn...@intel.com> >>>>>> --- >>>>>> 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
