* [dpdk-stable] [PATCH] test/eventdev: fix sprintf with snprintf
@ 2019-02-06 10:43 Pallantla Poornima
2019-02-08 21:19 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
0 siblings, 1 reply; 11+ messages in thread
From: Pallantla Poornima @ 2019-02-06 10:43 UTC (permalink / raw)
To: dev; +Cc: reshma.pattan, nikhil.rao, Pallantla Poornima, stable
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 <pallantlax.poornima@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);
--
2.17.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-02-06 10:43 [dpdk-stable] [PATCH] test/eventdev: fix sprintf with snprintf Pallantla Poornima
@ 2019-02-08 21:19 ` Aaron Conole
[not found] ` <7AE31235A30B41498D1C31348DC858BD5B534A73@IRSMSX103.ger.corp.intel.com>
0 siblings, 1 reply; 11+ messages in thread
From: Aaron Conole @ 2019-02-08 21:19 UTC (permalink / raw)
To: Pallantla Poornima; +Cc: dev, reshma.pattan, nikhil.rao, stable
Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>
> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
[not found] ` <7AE31235A30B41498D1C31348DC858BD5B534A73@IRSMSX103.ger.corp.intel.com>
@ 2019-03-12 7:41 ` Parthasarathy, JananeeX M
2019-03-12 14:44 ` Aaron Conole
0 siblings, 1 reply; 11+ messages in thread
From: Parthasarathy, JananeeX M @ 2019-03-12 7:41 UTC (permalink / raw)
To: 'Aaron Conole'
Cc: 'dev@dpdk.org',
Pattan, Reshma, Rao, Nikhil, 'stable@dpdk.org',
Poornima, PallantlaX
Hi
>-----Original Message-----
>From: Parthasarathy, JananeeX M
>Sent: Tuesday, February 19, 2019 6:33 PM
>To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
><pallantlax.poornima@intel.com>
>Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
><nikhil.rao@intel.com>; 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 <pallantlax.poornima@intel.com>
>>Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>><nikhil.rao@intel.com>; stable@dpdk.org
>>Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>snprintf
>>
>>Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>>>
>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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.
Thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-12 7:41 ` Parthasarathy, JananeeX M
@ 2019-03-12 14:44 ` Aaron Conole
2019-03-13 11:04 ` Ferruh Yigit
0 siblings, 1 reply; 11+ messages in thread
From: Aaron Conole @ 2019-03-12 14:44 UTC (permalink / raw)
To: Parthasarathy, JananeeX M
Cc: 'dev@dpdk.org',
Pattan, Reshma, Rao, Nikhil, 'stable@dpdk.org',
Poornima, PallantlaX
"Parthasarathy, JananeeX M" <jananeex.m.parthasarathy@intel.com> writes:
> Hi
>
>>-----Original Message-----
>>From: Parthasarathy, JananeeX M
>>Sent: Tuesday, February 19, 2019 6:33 PM
>>To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
>><pallantlax.poornima@intel.com>
>>Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>><nikhil.rao@intel.com>; 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 <pallantlax.poornima@intel.com>
>>>Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>><nikhil.rao@intel.com>; stable@dpdk.org
>>>Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>>snprintf
>>>
>>>Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>>>>
>>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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?
> Thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-12 14:44 ` Aaron Conole
@ 2019-03-13 11:04 ` Ferruh Yigit
2019-03-13 13:43 ` Aaron Conole
0 siblings, 1 reply; 11+ messages in thread
From: Ferruh Yigit @ 2019-03-13 11:04 UTC (permalink / raw)
To: Aaron Conole, Parthasarathy, JananeeX M
Cc: 'dev@dpdk.org',
Pattan, Reshma, Rao, Nikhil, 'stable@dpdk.org',
Poornima, PallantlaX
On 3/12/2019 2:44 PM, Aaron Conole wrote:
> "Parthasarathy, JananeeX M" <jananeex.m.parthasarathy@intel.com> writes:
>
>> Hi
>>
>>> -----Original Message-----
>>> From: Parthasarathy, JananeeX M
>>> Sent: Tuesday, February 19, 2019 6:33 PM
>>> To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
>>> <pallantlax.poornima@intel.com>
>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>> <nikhil.rao@intel.com>; 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 <pallantlax.poornima@intel.com>
>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>> <nikhil.rao@intel.com>; stable@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>>> snprintf
>>>>
>>>> Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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.
Thanks,
ferruh
[1]
test/eventdev: fix possible buffer overflow
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-13 11:04 ` Ferruh Yigit
@ 2019-03-13 13:43 ` Aaron Conole
2019-03-13 14:07 ` Ferruh Yigit
0 siblings, 1 reply; 11+ messages in thread
From: Aaron Conole @ 2019-03-13 13:43 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Parthasarathy, JananeeX M, 'dev@dpdk.org',
Pattan, Reshma, Rao, Nikhil, 'stable@dpdk.org',
Poornima, PallantlaX
Ferruh Yigit <ferruh.yigit@intel.com> writes:
> On 3/12/2019 2:44 PM, Aaron Conole wrote:
>> "Parthasarathy, JananeeX M" <jananeex.m.parthasarathy@intel.com> writes:
>>
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Parthasarathy, JananeeX M
>>>> Sent: Tuesday, February 19, 2019 6:33 PM
>>>> To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
>>>> <pallantlax.poornima@intel.com>
>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>> <nikhil.rao@intel.com>; 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 <pallantlax.poornima@intel.com>
>>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>>> <nikhil.rao@intel.com>; stable@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>>>> snprintf
>>>>>
>>>>> Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-13 13:43 ` Aaron Conole
@ 2019-03-13 14:07 ` Ferruh Yigit
2019-03-13 14:35 ` Aaron Conole
0 siblings, 1 reply; 11+ messages in thread
From: Ferruh Yigit @ 2019-03-13 14:07 UTC (permalink / raw)
To: Aaron Conole
Cc: Parthasarathy, JananeeX M, 'dev@dpdk.org',
Pattan, Reshma, Rao, Nikhil, 'stable@dpdk.org',
Poornima, PallantlaX
On 3/13/2019 1:43 PM, Aaron Conole wrote:
> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>
>> On 3/12/2019 2:44 PM, Aaron Conole wrote:
>>> "Parthasarathy, JananeeX M" <jananeex.m.parthasarathy@intel.com> writes:
>>>
>>>> Hi
>>>>
>>>>> -----Original Message-----
>>>>> From: Parthasarathy, JananeeX M
>>>>> Sent: Tuesday, February 19, 2019 6:33 PM
>>>>> To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
>>>>> <pallantlax.poornima@intel.com>
>>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>>> <nikhil.rao@intel.com>; 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 <pallantlax.poornima@intel.com>
>>>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>>>> <nikhil.rao@intel.com>; stable@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>>>>> snprintf
>>>>>>
>>>>>> Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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?
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.
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-13 14:07 ` Ferruh Yigit
@ 2019-03-13 14:35 ` Aaron Conole
2019-03-30 14:15 ` Jerin Jacob Kollanukkaran
0 siblings, 1 reply; 11+ messages in thread
From: Aaron Conole @ 2019-03-13 14:35 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Parthasarathy, JananeeX M, 'dev@dpdk.org',
Pattan, Reshma, Rao, Nikhil, 'stable@dpdk.org',
Poornima, PallantlaX
Ferruh Yigit <ferruh.yigit@intel.com> writes:
> On 3/13/2019 1:43 PM, Aaron Conole wrote:
>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>>
>>> On 3/12/2019 2:44 PM, Aaron Conole wrote:
>>>> "Parthasarathy, JananeeX M" <jananeex.m.parthasarathy@intel.com> writes:
>>>>
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Parthasarathy, JananeeX M
>>>>>> Sent: Tuesday, February 19, 2019 6:33 PM
>>>>>> To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
>>>>>> <pallantlax.poornima@intel.com>
>>>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>>>> <nikhil.rao@intel.com>; 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 <pallantlax.poornima@intel.com>
>>>>>>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>>>>>> <nikhil.rao@intel.com>; stable@dpdk.org
>>>>>>> Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>>>>>> snprintf
>>>>>>>
>>>>>>> Pallantla Poornima <pallantlax.poornima@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: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@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?
>
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-13 14:35 ` Aaron Conole
@ 2019-03-30 14:15 ` Jerin Jacob Kollanukkaran
2019-04-01 20:37 ` Aaron Conole
0 siblings, 1 reply; 11+ messages in thread
From: Jerin Jacob Kollanukkaran @ 2019-03-30 14:15 UTC (permalink / raw)
To: aconole, ferruh.yigit
Cc: nikhil.rao, pallantlax.poornima, dev, jananeex.m.parthasarathy,
reshma.pattan, stable
On Wed, 2019-03-13 at 10:35 -0400, Aaron Conole wrote:
>
> > 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.
Aaron,
Are you expecting next version of this patch? Or I can merge this
patch?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-03-30 14:15 ` Jerin Jacob Kollanukkaran
@ 2019-04-01 20:37 ` Aaron Conole
2019-04-02 1:35 ` [dpdk-stable] [EXT] " Jerin Jacob Kollanukkaran
0 siblings, 1 reply; 11+ messages in thread
From: Aaron Conole @ 2019-04-01 20:37 UTC (permalink / raw)
To: Jerin Jacob Kollanukkaran
Cc: ferruh.yigit, nikhil.rao, pallantlax.poornima, dev,
jananeex.m.parthasarathy, reshma.pattan, stable
Jerin Jacob Kollanukkaran <jerinj@marvell.com> writes:
> On Wed, 2019-03-13 at 10:35 -0400, Aaron Conole wrote:
>>
>> > 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.
>
> Aaron,
>
> Are you expecting next version of this patch? Or I can merge this
> patch?
Go ahead and merge.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
2019-04-01 20:37 ` Aaron Conole
@ 2019-04-02 1:35 ` Jerin Jacob Kollanukkaran
0 siblings, 0 replies; 11+ messages in thread
From: Jerin Jacob Kollanukkaran @ 2019-04-02 1:35 UTC (permalink / raw)
To: aconole
Cc: stable, nikhil.rao, pallantlax.poornima, dev,
jananeex.m.parthasarathy, reshma.pattan, ferruh.yigit
On Mon, 2019-04-01 at 16:37 -0400, Aaron Conole wrote:
>
> Jerin Jacob Kollanukkaran <jerinj@marvell.com> writes:
>
> > On Wed, 2019-03-13 at 10:35 -0400, Aaron Conole wrote:
> > > > 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.
> >
> > Aaron,
> >
> > Are you expecting next version of this patch? Or I can merge this
> > patch?
>
> Go ahead and merge.
Applied to dpdk-next-eventdev/master. Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-02 1:35 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06 10:43 [dpdk-stable] [PATCH] test/eventdev: fix sprintf with snprintf Pallantla Poornima
2019-02-08 21:19 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
[not found] ` <7AE31235A30B41498D1C31348DC858BD5B534A73@IRSMSX103.ger.corp.intel.com>
2019-03-12 7:41 ` Parthasarathy, JananeeX M
2019-03-12 14:44 ` Aaron Conole
2019-03-13 11:04 ` Ferruh Yigit
2019-03-13 13:43 ` Aaron Conole
2019-03-13 14:07 ` Ferruh Yigit
2019-03-13 14:35 ` Aaron Conole
2019-03-30 14:15 ` Jerin Jacob Kollanukkaran
2019-04-01 20:37 ` Aaron Conole
2019-04-02 1:35 ` [dpdk-stable] [EXT] " Jerin Jacob Kollanukkaran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).