net/ring: fix unchecked return value
Checks
Commit Message
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
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
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 */
}
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.
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
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.
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 :)
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
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.
@@ -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;