[PATCH] app/graph: fix overrun error (destination buffer too small)

[PATCH] app/graph: fix overrun error (destination buffer too small)

[Khadem Ullah]

Some memory locations will be written with incorrect values,
possibly corrupting data structures or data integrity.
Size of destination buffer is smaller than the size
argument specified.

Coverity issue: 415430
Fixes: 3850cb06ab9c ('app/graph: add ethdev commands')
Cc: stable@dpdk.org

Signed-off-by: Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk>
---
 app/graph/ethdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
index 2f4cf65c96..f6d4bce9ab 100644
--- a/app/graph/ethdev.c
+++ b/app/graph/ethdev.c
@@ -663,11 +663,12 @@ cmd_ethdev_parsed(void *parsed_result, __rte_unused struct cmdline *cl, void *da
 	struct cmd_ethdev_result *res = parsed_result;
 	struct ethdev_config config;
 	int rc;
-
+	size_t len;
 	memset(&config, 0, sizeof(struct ethdev_config));
 	config.rx.n_queues = res->nb_rxq;
 	config.rx.queue_size = ETHDEV_RX_DESC_DEFAULT;
-	memcpy(config.rx.mempool_name, res->mempool, strlen(res->mempool));
+	len = strnlen(res->mempool, sizeof(config.rx.mempool_name));
+	memcpy(config.rx.mempool_name, res->mempool, len);
 
 	config.tx.n_queues = res->nb_txq;
 	config.tx.queue_size = ETHDEV_TX_DESC_DEFAULT;
-- 
2.43.0

RE: [PATCH] app/graph: fix overrun error (destination buffer too small)

[Varghese, Vipin]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Khadem,

Thank you for sharing, please find my comment below

Snipped

>
> Some memory locations will be written with incorrect values, possibly corrupting
> data structures or data integrity.
> Size of destination buffer is smaller than the size argument specified.
>
> Coverity issue: 415430
> Fixes: 3850cb06ab9c ('app/graph: add ethdev commands')
> Cc: stable@dpdk.org
>
> Signed-off-by: Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk>
> ---
>  app/graph/ethdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c index 2f4cf65c96..f6d4bce9ab
> 100644
> --- a/app/graph/ethdev.c
> +++ b/app/graph/ethdev.c
> @@ -663,11 +663,12 @@ cmd_ethdev_parsed(void *parsed_result, __rte_unused
> struct cmdline *cl, void *da
>         struct cmd_ethdev_result *res = parsed_result;
>         struct ethdev_config config;
>         int rc;
> -
> +       size_t len;
>         memset(&config, 0, sizeof(struct ethdev_config));
>         config.rx.n_queues = res->nb_rxq;
>         config.rx.queue_size = ETHDEV_RX_DESC_DEFAULT;
> -       memcpy(config.rx.mempool_name, res->mempool, strlen(res->mempool));
> +       len = strnlen(res->mempool, sizeof(config.rx.mempool_name));
> +       memcpy(config.rx.mempool_name, res->mempool, len);

Yes indeed the strnlen is good choice. What I assume you are doing here to get the exact size `config.rx_mempool_size` by seeking for first `\0`. But the api `strnlen` intention of use is not of the same. The second argument is treated as maximum seek size, that is either return string len less than max-size or if not found return second argument.

There will no difference in using ` strlen(res->mempool)` and ` strnlen(res->mempool, sizeof(config.rx.mempool_name));` in this code case.

Can you please rework and share again. Hence NACK

NACK: Vipin Varghese <Vipin.varghese@amd.com>

>
>         config.tx.n_queues = res->nb_txq;
>         config.tx.queue_size = ETHDEV_TX_DESC_DEFAULT;
> --
> 2.43.0