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
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
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.
--
David Marchand
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.
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
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; > >
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.
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"?
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
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
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.