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

[David Marchand <david.marchand@redhat.com>]

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