diff mbox series

net/ring: fix unchecked return value

Message ID 20200922172015.266698-1-kevin.laatz@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/ring: fix unchecked return value |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-broadcom-Functional success Functional Testing PASS

Commit Message

Kevin Laatz Sept. 22, 2020, 5:20 p.m. UTC 
  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(-)

Comments

David Marchand Sept. 23, 2020, 8:06 a.m. UTC | #1 
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
Bruce Richardson Sept. 23, 2020, 9:39 a.m. UTC | #2 
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 */
}
David Marchand Sept. 23, 2020, 9:43 a.m. UTC | #3 
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.
Kevin Laatz Sept. 23, 2020, 10:04 a.m. UTC | #4 
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
Bruce Richardson Sept. 23, 2020, 10:25 a.m. UTC | #5 
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.
Ferruh Yigit Sept. 25, 2020, 12:43 p.m. UTC | #6 
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 :)
Kevin Laatz Oct. 1, 2020, 2:14 p.m. UTC | #7 
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
Ferruh Yigit Oct. 1, 2020, 2:51 p.m. UTC | #8 
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.
diff mbox series

Patch

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;