patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/ring: fix unchecked return value
@ 2020-09-22 17:20 Kevin Laatz
  2020-09-23  8:06 ` [dpdk-stable] [dpdk-dev] " David Marchand
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Kevin Laatz @ 2020-09-22 17:20 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, bruce.richardson, Kevin Laatz, stable

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 <kevin.laatz@intel.com>
---
 drivers/net/ring/rte_eth_ring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 40fe1ca4ba..62060e46ce 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -539,7 +539,8 @@ parse_internal_args(const char *key __rte_unused, const char *value,
 	struct ring_internal_args **internal_args = data;
 	void *args;
 
-	sscanf(value, "%p", &args);
+	if (sscanf(value, "%p", &args) != 1)
+		return -1;
 
 	*internal_args = args;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ring: fix unchecked return value
  2020-09-22 17:20 [dpdk-stable] [PATCH] net/ring: fix unchecked return value Kevin Laatz
@ 2020-09-23  8:06 ` David Marchand
  2020-09-23  9:39   ` Bruce Richardson
  2020-09-25 12:43 ` [dpdk-stable] " Ferruh Yigit
  2020-10-01 17:09 ` [dpdk-stable] [PATCH v2] " Kevin Laatz
  2 siblings, 1 reply; 16+ messages in thread
From: David Marchand @ 2020-09-23  8:06 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, Yigit, Ferruh, Bruce Richardson, dpdk stable

On Tue, Sep 22, 2020 at 7:25 PM Kevin Laatz <kevin.laatz@intel.com> 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 <kevin.laatz@intel.com>
> ---
>  drivers/net/ring/rte_eth_ring.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> index 40fe1ca4ba..62060e46ce 100644
> --- a/drivers/net/ring/rte_eth_ring.c
> +++ b/drivers/net/ring/rte_eth_ring.c
> @@ -539,7 +539,8 @@ parse_internal_args(const char *key __rte_unused, const char *value,
>         struct ring_internal_args **internal_args = data;
>         void *args;
>
> -       sscanf(value, "%p", &args);
> +       if (sscanf(value, "%p", &args) != 1)
> +               return -1;

Not sure this really needs fixing, as I understood the internal option
is something only the driver uses.

On the patch itself, sscanf stops at the first character it deems
incorrect, meaning that you would not detect trailing chars, like for
0x1234Z.
You can detect this by adding a canary.

$ cat sscanf.c
#include <stdio.h>

int main(int argc, char *argv[])
{
    void *args;
    char c;

    if (sscanf(argv[1], "%p", &args) != 1)
        printf("'%%p' KO for %s\n", argv[1]);
    else
        printf("'%%p' ok for %s\n", argv[1]);

    if (sscanf(argv[1], "%p%c", &args, &c) != 1)
        printf("'%%p%%c' KO for %s\n", argv[1]);
    else
        printf("'%%p%%c' ok for %s\n", argv[1]);
    return 0;
}

$ gcc -o sscanf -Wall -Werror sscanf.c

$ ./sscanf 0x1234
'%p' ok for 0x1234
'%p%c' ok for 0x1234

$ ./sscanf 0x1234Z
'%p' ok for 0x1234Z
'%p%c' KO for 0x1234Z


-- 
David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ring: fix unchecked return value
  2020-09-23  8:06 ` [dpdk-stable] [dpdk-dev] " David Marchand
@ 2020-09-23  9:39   ` Bruce Richardson
  2020-09-23  9:43     ` David Marchand
  0 siblings, 1 reply; 16+ messages in thread
From: Bruce Richardson @ 2020-09-23  9:39 UTC (permalink / raw)
  To: David Marchand; +Cc: Kevin Laatz, dev, Yigit, Ferruh, dpdk stable

On Wed, Sep 23, 2020 at 10:06:25AM +0200, David Marchand wrote:
> On Tue, Sep 22, 2020 at 7:25 PM Kevin Laatz <kevin.laatz@intel.com> 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 <kevin.laatz@intel.com>
> > ---
> >  drivers/net/ring/rte_eth_ring.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> > index 40fe1ca4ba..62060e46ce 100644
> > --- a/drivers/net/ring/rte_eth_ring.c
> > +++ b/drivers/net/ring/rte_eth_ring.c
> > @@ -539,7 +539,8 @@ parse_internal_args(const char *key __rte_unused, const char *value,
> >         struct ring_internal_args **internal_args = data;
> >         void *args;
> >
> > -       sscanf(value, "%p", &args);
> > +       if (sscanf(value, "%p", &args) != 1)
> > +               return -1;
> 
> Not sure this really needs fixing, as I understood the internal option
> is something only the driver uses.
> 
> On the patch itself, sscanf stops at the first character it deems
> incorrect, meaning that you would not detect trailing chars, like for
> 0x1234Z.
> You can detect this by adding a canary.
> 
> $ cat sscanf.c
> #include <stdio.h>
> 
> int main(int argc, char *argv[])
> {
>     void *args;
>     char c;
> 
>     if (sscanf(argv[1], "%p", &args) != 1)
>         printf("'%%p' KO for %s\n", argv[1]);
>     else
>         printf("'%%p' ok for %s\n", argv[1]);
> 
>     if (sscanf(argv[1], "%p%c", &args, &c) != 1)
>         printf("'%%p%%c' KO for %s\n", argv[1]);
>     else
>         printf("'%%p%%c' ok for %s\n", argv[1]);
>     return 0;
> }
> 
> $ gcc -o sscanf -Wall -Werror sscanf.c
> 
> $ ./sscanf 0x1234
> '%p' ok for 0x1234
> '%p%c' ok for 0x1234
> 
> $ ./sscanf 0x1234Z
> '%p' ok for 0x1234Z
> '%p%c' KO for 0x1234Z
> 
I think a more standard way of checking for trailing chars is to use %n
which stores the number of chars processed. Then check that against
strlen.

For example something like:

if (sscanf(value, "%p%n", args, n) != 1 || n != strlen(value)) {
  /* do error handling */
}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ring: fix unchecked return value
  2020-09-23  9:39   ` Bruce Richardson
@ 2020-09-23  9:43     ` David Marchand
  2020-09-23 10:04       ` Kevin Laatz
  2020-09-23 10:25       ` Bruce Richardson
  0 siblings, 2 replies; 16+ messages in thread
From: David Marchand @ 2020-09-23  9:43 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Kevin Laatz, dev, Yigit, Ferruh, dpdk stable

On Wed, Sep 23, 2020 at 11:39 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> I think a more standard way of checking for trailing chars is to use %n
> which stores the number of chars processed. Then check that against
> strlen.
>
> For example something like:
>
> if (sscanf(value, "%p%n", args, n) != 1 || n != strlen(value)) {
>   /* do error handling */
> }
>

The man is a bit scary about %n:

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.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ring: fix unchecked return value
  2020-09-23  9:43     ` David Marchand
@ 2020-09-23 10:04       ` Kevin Laatz
  2020-09-23 10:25       ` Bruce Richardson
  1 sibling, 0 replies; 16+ messages in thread
From: Kevin Laatz @ 2020-09-23 10:04 UTC (permalink / raw)
  To: David Marchand, Bruce Richardson; +Cc: dev, Yigit, Ferruh, dpdk stable

On 23/09/2020 10:43, David Marchand wrote:
> On Wed, Sep 23, 2020 at 11:39 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
>> I think a more standard way of checking for trailing chars is to use %n
>> which stores the number of chars processed. Then check that against
>> strlen.
>>
>> For example something like:
>>
>> if (sscanf(value, "%p%n", args, n) != 1 || n != strlen(value)) {
>>    /* do error handling */
>> }
>>
> The man is a bit scary about %n:
>
> 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.

I still think the check is required.

As you rightly point out, the value will be truncated once sscanf deems 
a character incorrect. This will happen regardless of if we check the 
return or not. What the check catches, however, is the case where the 
incorrect character comes at the beginning, e.g. Z0x1234 - which would 
truncated the entire arg.

$ ./sscanf Z0x1234
'%p' KO for Z0x1234
'%p%c' KO for Z0x1234

---
Kevin


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ring: fix unchecked return value
  2020-09-23  9:43     ` David Marchand
  2020-09-23 10:04       ` Kevin Laatz
@ 2020-09-23 10:25       ` Bruce Richardson
  1 sibling, 0 replies; 16+ messages in thread
From: Bruce Richardson @ 2020-09-23 10:25 UTC (permalink / raw)
  To: David Marchand; +Cc: Kevin Laatz, dev, Yigit, Ferruh, dpdk stable

On Wed, Sep 23, 2020 at 11:43:31AM +0200, David Marchand wrote:
> On Wed, Sep 23, 2020 at 11:39 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > I think a more standard way of checking for trailing chars is to use %n
> > which stores the number of chars processed. Then check that against
> > strlen.
> >
> > For example something like:
> >
> > if (sscanf(value, "%p%n", args, n) != 1 || n != strlen(value)) {
> >   /* do error handling */
> > }
> >
> 
> The man is a bit scary about %n:
> 
> 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.
> 

That's not in the man page on my system (Ubuntu 20.04):

n     Nothing is expected; instead, the number of characters  consumed  thus  far
      from  the input is stored through the next pointer, which must be a pointer
      to int.  This is not a conversion and does not increase the count  returned
      by  the  function.  The assignment can be suppressed with the * assignment-
      suppression character, but the effect on the  return  value  is  undefined.
      Therefore %*n conversions should not be used.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH] net/ring: fix unchecked return value
  2020-09-22 17:20 [dpdk-stable] [PATCH] net/ring: fix unchecked return value Kevin Laatz
  2020-09-23  8:06 ` [dpdk-stable] [dpdk-dev] " David Marchand
@ 2020-09-25 12:43 ` Ferruh Yigit
  2020-10-01 14:14   ` Kevin Laatz
  2020-10-01 17:09 ` [dpdk-stable] [PATCH v2] " Kevin Laatz
  2 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2020-09-25 12:43 UTC (permalink / raw)
  To: Kevin Laatz, dev
  Cc: bruce.richardson, stable, David Marchand, Stephen Hemminger

On 9/22/2020 6:20 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 <kevin.laatz@intel.com>
> ---
>   drivers/net/ring/rte_eth_ring.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> index 40fe1ca4ba..62060e46ce 100644
> --- a/drivers/net/ring/rte_eth_ring.c
> +++ b/drivers/net/ring/rte_eth_ring.c
> @@ -539,7 +539,8 @@ parse_internal_args(const char *key __rte_unused, const char *value,
>   	struct ring_internal_args **internal_args = data;
>   	void *args;
>   
> -	sscanf(value, "%p", &args);
> +	if (sscanf(value, "%p", &args) != 1)
> +		return -1;

I am aware that this is just to fix the coverity error to check the 
return value, BUT :)

The internal args is mainly to pass the information get by API 
('rte_eth_from_ring()') to ring probe. And the main information to pass 
is the ring.
It may be possible to pass the ring_name only and eliminate internal 
args completely, the driver already has a way to pass ring name:
"nodeaction=name:node:ATTACH" devargs.

If you have time, can you please check if it can be possible to fill and 
pass the nodeaction devargs in 'rte_eth_from_rings()' and eliminate the 
'internal' devargs completely?
If it works, as a bonus it will resolve above coverity issue by removing 
it :)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH] net/ring: fix unchecked return value
  2020-09-25 12:43 ` [dpdk-stable] " Ferruh Yigit
@ 2020-10-01 14:14   ` Kevin Laatz
  2020-10-01 14:51     ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Kevin Laatz @ 2020-10-01 14:14 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: bruce.richardson, stable, David Marchand, Stephen Hemminger


On 25/09/2020 13:43, Ferruh Yigit wrote:
> On 9/22/2020 6:20 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 <kevin.laatz@intel.com>
>> ---
>>   drivers/net/ring/rte_eth_ring.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ring/rte_eth_ring.c 
>> b/drivers/net/ring/rte_eth_ring.c
>> index 40fe1ca4ba..62060e46ce 100644
>> --- a/drivers/net/ring/rte_eth_ring.c
>> +++ b/drivers/net/ring/rte_eth_ring.c
>> @@ -539,7 +539,8 @@ parse_internal_args(const char *key __rte_unused, 
>> const char *value,
>>       struct ring_internal_args **internal_args = data;
>>       void *args;
>>   -    sscanf(value, "%p", &args);
>> +    if (sscanf(value, "%p", &args) != 1)
>> +        return -1;
>
> I am aware that this is just to fix the coverity error to check the 
> return value, BUT :)
>
> The internal args is mainly to pass the information get by API 
> ('rte_eth_from_ring()') to ring probe. And the main information to 
> pass is the ring.
> It may be possible to pass the ring_name only and eliminate internal 
> args completely, the driver already has a way to pass ring name:
> "nodeaction=name:node:ATTACH" devargs.
>
> If you have time, can you please check if it can be possible to fill 
> and pass the nodeaction devargs in 'rte_eth_from_rings()' and 
> eliminate the 'internal' devargs completely?
> If it works, as a bonus it will resolve above coverity issue by 
> removing it :)
>
Hi Ferruh,

It seems to be used for more than just that - after the parsing, the 
internal args are passed to do_eth_dev_ring_create() as multiple parameters.

- Kevin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH] net/ring: fix unchecked return value
  2020-10-01 14:14   ` Kevin Laatz
@ 2020-10-01 14:51     ` Ferruh Yigit
  0 siblings, 0 replies; 16+ messages in thread
From: Ferruh Yigit @ 2020-10-01 14:51 UTC (permalink / raw)
  To: Kevin Laatz, dev
  Cc: bruce.richardson, stable, David Marchand, Stephen Hemminger

On 10/1/2020 3:14 PM, Kevin Laatz wrote:
> 
> On 25/09/2020 13:43, Ferruh Yigit wrote:
>> On 9/22/2020 6:20 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 <kevin.laatz@intel.com>
>>> ---
>>>   drivers/net/ring/rte_eth_ring.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
>>> index 40fe1ca4ba..62060e46ce 100644
>>> --- a/drivers/net/ring/rte_eth_ring.c
>>> +++ b/drivers/net/ring/rte_eth_ring.c
>>> @@ -539,7 +539,8 @@ parse_internal_args(const char *key __rte_unused, const 
>>> char *value,
>>>       struct ring_internal_args **internal_args = data;
>>>       void *args;
>>>   -    sscanf(value, "%p", &args);
>>> +    if (sscanf(value, "%p", &args) != 1)
>>> +        return -1;
>>
>> I am aware that this is just to fix the coverity error to check the return 
>> value, BUT :)
>>
>> The internal args is mainly to pass the information get by API 
>> ('rte_eth_from_ring()') to ring probe. And the main information to pass is the 
>> ring.
>> It may be possible to pass the ring_name only and eliminate internal args 
>> completely, the driver already has a way to pass ring name:
>> "nodeaction=name:node:ATTACH" devargs.
>>
>> If you have time, can you please check if it can be possible to fill and pass 
>> the nodeaction devargs in 'rte_eth_from_rings()' and eliminate the 'internal' 
>> devargs completely?
>> If it works, as a bonus it will resolve above coverity issue by removing it :)
>>
> Hi Ferruh,
> 
> It seems to be used for more than just that - after the parsing, the internal 
> args are passed to do_eth_dev_ring_create() as multiple parameters.
> 

Yes multiple args passed to 'do_eth_dev_ring_create()' from internal arg, and 
they may be provided via existing nodeaction arg too, needs to be checked.

Anyway, the option to eliminate the internal args is just a good to have, we can 
check it later instead making the this coverity fix more complex.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-stable] [PATCH v2] net/ring: fix unchecked return value
  2020-09-22 17:20 [dpdk-stable] [PATCH] net/ring: fix unchecked return value Kevin Laatz
  2020-09-23  8:06 ` [dpdk-stable] [dpdk-dev] " David Marchand
  2020-09-25 12:43 ` [dpdk-stable] " Ferruh Yigit
@ 2020-10-01 17:09 ` Kevin Laatz
  2020-10-12 11:57   ` Ferruh Yigit
  2020-10-13 13:07   ` [dpdk-stable] [PATCH v3] " Kevin Laatz
  2 siblings, 2 replies; 16+ messages in thread
From: Kevin Laatz @ 2020-10-01 17:09 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, bruce.richardson, stephen, Kevin Laatz, stable

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 <kevin.laatz@intel.com>

---
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)) {
+		PMD_LOG(ERR, "Error parsing internal args");
+
+		return -1;
+	}
 
 	*internal_args = args;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/ring: fix unchecked return value
  2020-10-01 17:09 ` [dpdk-stable] [PATCH v2] " Kevin Laatz
@ 2020-10-12 11:57   ` Ferruh Yigit
  2020-10-12 12:45     ` Bruce Richardson
  2020-10-13 13:07   ` [dpdk-stable] [PATCH v3] " Kevin Laatz
  1 sibling, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2020-10-12 11:57 UTC (permalink / raw)
  To: Kevin Laatz, dev; +Cc: bruce.richardson, stephen, stable

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 <kevin.laatz@intel.com>
> 
> ---
> 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;
>   
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/ring: fix unchecked return value
  2020-10-12 11:57   ` Ferruh Yigit
@ 2020-10-12 12:45     ` Bruce Richardson
  2020-10-12 13:04       ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Bruce Richardson @ 2020-10-12 12:45 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Kevin Laatz, dev, stephen, stable

On Mon, Oct 12, 2020 at 12:57:11PM +0100, Ferruh Yigit wrote:
> 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 <kevin.laatz@intel.com>
> > 
> > ---
> > 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" ?
> 

Maybe in that copy of the man page but on my Ubuntu system there is no such
disclaimer, and I don't see it either on the kernel.org man page reference:

https://man7.org/linux/man-pages/man3/sscanf.3.html

That official man page reference clearly states that the behaviour is that
%n does not increase the reference count.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/ring: fix unchecked return value
  2020-10-12 12:45     ` Bruce Richardson
@ 2020-10-12 13:04       ` Ferruh Yigit
  2020-10-12 13:11         ` Bruce Richardson
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2020-10-12 13:04 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Kevin Laatz, dev, stephen, stable

On 10/12/2020 1:45 PM, Bruce Richardson wrote:
> On Mon, Oct 12, 2020 at 12:57:11PM +0100, Ferruh Yigit wrote:
>> 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 <kevin.laatz@intel.com>
>>>
>>> ---
>>> 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" ?
>>
> 
> Maybe in that copy of the man page but on my Ubuntu system there is no such
> disclaimer, and I don't see it either on the kernel.org man page reference:
> 
> https://man7.org/linux/man-pages/man3/sscanf.3.html
> 
> That official man page reference clearly states that the behaviour is that
> %n does not increase the reference count.
> 

My Linux box also doesn't have that note, but just to prevent the PMD fails for 
something like this.

Do you see any downside of checking as "sscanf() == 0"?


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/ring: fix unchecked return value
  2020-10-12 13:04       ` Ferruh Yigit
@ 2020-10-12 13:11         ` Bruce Richardson
  0 siblings, 0 replies; 16+ messages in thread
From: Bruce Richardson @ 2020-10-12 13:11 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Kevin Laatz, dev, stephen, stable

On Mon, Oct 12, 2020 at 02:04:26PM +0100, Ferruh Yigit wrote:
> On 10/12/2020 1:45 PM, Bruce Richardson wrote:
> > On Mon, Oct 12, 2020 at 12:57:11PM +0100, Ferruh Yigit wrote:
> > > 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 <kevin.laatz@intel.com>
> > > > 
> > > > ---
> > > > 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" ?
> > > 
> > 
> > Maybe in that copy of the man page but on my Ubuntu system there is no such
> > disclaimer, and I don't see it either on the kernel.org man page reference:
> > 
> > https://man7.org/linux/man-pages/man3/sscanf.3.html
> > 
> > That official man page reference clearly states that the behaviour is that
> > %n does not increase the reference count.
> > 
> 
> My Linux box also doesn't have that note, but just to prevent the PMD fails
> for something like this.
> 
> Do you see any downside of checking as "sscanf() == 0"?
>

Nope, no issue with checking that too.

/Bruce 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-stable] [PATCH v3] net/ring: fix unchecked return value
  2020-10-01 17:09 ` [dpdk-stable] [PATCH v2] " Kevin Laatz
  2020-10-12 11:57   ` Ferruh Yigit
@ 2020-10-13 13:07   ` Kevin Laatz
  2020-10-13 17:23     ` Ferruh Yigit
  1 sibling, 1 reply; 16+ messages in thread
From: Kevin Laatz @ 2020-10-13 13:07 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, bruce.richardson, stephen, Kevin Laatz, stable

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 <kevin.laatz@intel.com>

---
v2: added consumed characters count check

v3: add more improved checks
---
 drivers/net/ring/rte_eth_ring.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 40fe1ca4ba..41692305e7 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -16,6 +16,7 @@
 #define ETH_RING_ACTION_CREATE		"CREATE"
 #define ETH_RING_ACTION_ATTACH		"ATTACH"
 #define ETH_RING_INTERNAL_ARG		"internal"
+#define ETH_RING_INTERNAL_ARG_MAX_LEN	19
 
 static const char *valid_arguments[] = {
 	ETH_RING_NUMA_NODE_ACTION_ARG,
@@ -538,8 +539,21 @@ parse_internal_args(const char *key __rte_unused, const char *value,
 {
 	struct ring_internal_args **internal_args = data;
 	void *args;
+	int ret, n;
 
-	sscanf(value, "%p", &args);
+	/* make sure 'value' is valid pointer length */
+	if (strnlen(value, ETH_RING_INTERNAL_ARG_MAX_LEN) >=
+			ETH_RING_INTERNAL_ARG_MAX_LEN) {
+		PMD_LOG(ERR, "Error parsing internal args, 'value' too long");
+		return -1;
+	}
+
+	ret = sscanf(value, "%p%n", &args, &n);
+	if (ret == 0 || (size_t)n != strlen(value)) {
+		PMD_LOG(ERR, "Error parsing internal args");
+
+		return -1;
+	}
 
 	*internal_args = args;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v3] net/ring: fix unchecked return value
  2020-10-13 13:07   ` [dpdk-stable] [PATCH v3] " Kevin Laatz
@ 2020-10-13 17:23     ` Ferruh Yigit
  0 siblings, 0 replies; 16+ messages in thread
From: Ferruh Yigit @ 2020-10-13 17:23 UTC (permalink / raw)
  To: Kevin Laatz, dev; +Cc: bruce.richardson, stephen, stable

On 10/13/2020 2:07 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 <kevin.laatz@intel.com>

<...>

> +#define ETH_RING_INTERNAL_ARG_MAX_LEN	19

Added following comment while merging: /* "0x..16chars..\0" */

<...>

> -	sscanf(value, "%p", &args);
> +	/* make sure 'value' is valid pointer length */
> +	if (strnlen(value, ETH_RING_INTERNAL_ARG_MAX_LEN) >=
> +			ETH_RING_INTERNAL_ARG_MAX_LEN) {
> +		PMD_LOG(ERR, "Error parsing internal args, 'value' too long");

'value' is variable name and may not fit to the debug log.
Replaced with "..., argument is too long" while merging.

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied to dpdk-next-net/main, thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-10-13 17:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 17:20 [dpdk-stable] [PATCH] net/ring: fix unchecked return value Kevin Laatz
2020-09-23  8:06 ` [dpdk-stable] [dpdk-dev] " David Marchand
2020-09-23  9:39   ` Bruce Richardson
2020-09-23  9:43     ` David Marchand
2020-09-23 10:04       ` Kevin Laatz
2020-09-23 10:25       ` Bruce Richardson
2020-09-25 12:43 ` [dpdk-stable] " Ferruh Yigit
2020-10-01 14:14   ` Kevin Laatz
2020-10-01 14:51     ` Ferruh Yigit
2020-10-01 17:09 ` [dpdk-stable] [PATCH v2] " Kevin Laatz
2020-10-12 11:57   ` Ferruh Yigit
2020-10-12 12:45     ` Bruce Richardson
2020-10-12 13:04       ` Ferruh Yigit
2020-10-12 13:11         ` Bruce Richardson
2020-10-13 13:07   ` [dpdk-stable] [PATCH v3] " Kevin Laatz
2020-10-13 17:23     ` Ferruh Yigit

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).