diff mbox series

[v2] net/ring: fix unchecked return value

Message ID 20201001170902.487111-1-kevin.laatz@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] 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-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing warning Testing issues

Commit Message

Kevin Laatz Oct. 1, 2020, 5:09 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>

---
v2: added consumed characters count check
---
 drivers/net/ring/rte_eth_ring.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit Oct. 12, 2020, 11:57 a.m. UTC | #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;
>   
>
Bruce Richardson Oct. 12, 2020, 12:45 p.m. UTC | #2 
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.
Ferruh Yigit Oct. 12, 2020, 1:04 p.m. UTC | #3 
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"?
Bruce Richardson Oct. 12, 2020, 1:11 p.m. UTC | #4 
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
diff mbox series

Patch

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;