Re: [dpdk-dev] [PATCH v2] net/ring: fix unchecked return value

[Ferruh Yigit <ferruh.yigit@intel.com>]

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;
>   
>