[PATCH] app/graph: fix destination buffer too small

[PATCH] app/graph: fix destination buffer too small

[Mahmoud Maatuq]

as sizeof(config.rx.mempool_name) is < sizeof(res->mempool), it's safer
to copy min size of them to avoide out of bound memory write.

Coverity issue: 415430
Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
Cc: skori@marvell.com

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
---
 app/graph/ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
index cfc1b18569..4b6009ee12 100644
--- a/app/graph/ethdev.c
+++ b/app/graph/ethdev.c
@@ -671,7 +671,8 @@ cmd_ethdev_parsed(void *parsed_result, __rte_unused struct cmdline *cl, void *da
 	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));
+	memcpy(config.rx.mempool_name, res->mempool,
+		RTE_MIN(sizeof(config.rx.mempool_name), strlen(res->mempool)));
 
 	config.tx.n_queues = res->nb_txq;
 	config.tx.queue_size = ETHDEV_TX_DESC_DEFAULT;
-- 
2.43.0

Re: [PATCH] app/graph: fix destination buffer too small

[Stephen Hemminger]

On Mon, 24 Jun 2024 00:09:21 +0400
Mahmoud Maatuq <mahmoudmatook.mm@gmail.com> wrote:

> as sizeof(config.rx.mempool_name) is < sizeof(res->mempool), it's safer
> to copy min size of them to avoide out of bound memory write.
> 
> Coverity issue: 415430
> Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
> Cc: skori@marvell.com
> 
> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>

NAK, this could create an invalid not null terminated string.

Since mempool name is a null terminated string this should have always
used strlcpy instead of memcpy.

[PATCH v2] app/graph: fix destination buffer too small

[Mahmoud Maatuq]

as sizeof(config.rx.mempool_name) is < sizeof(res->mempool) we should
copy at most sizeof(config.rx.mempool_name) and replace memcpy with
strlcpy as mempool name is a null terminated string

Coverity issue: 415430
Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
Cc: skori@marvell.com

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
---
v2:
* replaced memcpy with strlcpy
---
 app/graph/ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
index cfc1b18569..e7a02b40a9 100644
--- a/app/graph/ethdev.c
+++ b/app/graph/ethdev.c
@@ -16,6 +16,7 @@

 #include "ethdev_priv.h"
 #include "module_api.h"
+#include "rte_string_fns.h"

 static const char
 cmd_ethdev_mtu_help[] = "ethdev <ethdev_name> mtu <mtu_sz>";
@@ -671,7 +672,7 @@ cmd_ethdev_parsed(void *parsed_result, __rte_unused struct cmdline *cl, void *da
 	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));
+	strlcpy(config.rx.mempool_name, res->mempool, sizeof(config.rx.mempool_name));

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

RE: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small

[Kiran Kumar Kokkilagadda]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]



From: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
Sent: Tuesday, June 25, 2024 1:31 AM
To: Sunil Kumar Kori <skori@marvell.com>; Rakesh Kudurumalla <rkudurumalla@marvell.com>; Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Jerin Jacob <jerinj@marvell.com>
Cc: dev@dpdk.org; Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
Subject: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small

as sizeof(config. rx. mempool_name) is < sizeof(res->mempool) we should copy at most sizeof(config. rx. mempool_name) and replace memcpy with strlcpy as mempool name is a null terminated string Coverity issue: 415430 Fixes: 3850cb06ab9c ("app/graph: 


as sizeof(config.rx.mempool_name) is < sizeof(res->mempool) we should

copy at most sizeof(config.rx.mempool_name) and replace memcpy with

strlcpy as mempool name is a null terminated string



Coverity issue: 415430

Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")

Cc: skori@marvell.com<mailto:skori@marvell.com>



Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com<mailto:mahmoudmatook.mm@gmail.com>>

---

v2:

* replaced memcpy with strlcpy

---

 app/graph/ethdev.c | 3 ++-

 1 file changed, 2 insertions(+), 1 deletion(-)



diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c

index cfc1b18569..e7a02b40a9 100644

--- a/app/graph/ethdev.c

+++ b/app/graph/ethdev.c

@@ -16,6 +16,7 @@



 #include "ethdev_priv.h"

 #include "module_api.h"

+#include "rte_string_fns.h"



 static const char

 cmd_ethdev_mtu_help[] = "ethdev <ethdev_name> mtu <mtu_sz>";

@@ -671,7 +672,7 @@ cmd_ethdev_parsed(void *parsed_result, __rte_unused struct cmdline *cl, void *da

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

+           strlcpy(config.rx.mempool_name, res->mempool, sizeof(config.rx.mempool_name));



Can be changed to strlcpy(config.rx.mempool_name, res->mempool->name, sizeof(config.rx.mempool_name)); ?



              config.tx.n_queues = res->nb_txq;

              config.tx.queue_size = ETHDEV_TX_DESC_DEFAULT;

--

2.43.0



[-- Attachment #2: Type: text/html, Size: 10044 bytes --]

Re: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small

[Mahmoud Matook]

On 06/25, Kiran Kumar Kokkilagadda wrote:

> 
> 
> From: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> Sent: Tuesday, June 25, 2024 1:31 AM
> To: Sunil Kumar Kori <skori@marvell.com>; Rakesh Kudurumalla <rkudurumalla@marvell.com>; Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Jerin Jacob <jerinj@marvell.com>
> Cc: dev@dpdk.org; Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> Subject: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small
> 
> as sizeof(config. rx. mempool_name) is < sizeof(res->mempool) we should copy at most sizeof(config. rx. mempool_name) and replace memcpy with strlcpy as mempool name is a null terminated string Coverity issue: 415430 Fixes: 3850cb06ab9c ("app/graph: 
> 
> 
> as sizeof(config.rx.mempool_name) is < sizeof(res->mempool) we should
> 
> copy at most sizeof(config.rx.mempool_name) and replace memcpy with
> 
> strlcpy as mempool name is a null terminated string
> 
> 
> 
> Coverity issue: 415430
> 
> Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
> 
> Cc: skori@marvell.com<mailto:skori@marvell.com>
> 
> 
> 
> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com<mailto:mahmoudmatook.mm@gmail.com>>
> 
> ---
> 
> v2:
> 
> * replaced memcpy with strlcpy
> 
> ---
> 
>  app/graph/ethdev.c | 3 ++-
> 
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> 
> diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
> 
> index cfc1b18569..e7a02b40a9 100644
> 
> --- a/app/graph/ethdev.c
> 
> +++ b/app/graph/ethdev.c
> 
> @@ -16,6 +16,7 @@
> 
> 
> 
>  #include "ethdev_priv.h"
> 
>  #include "module_api.h"
> 
> +#include "rte_string_fns.h"
> 
> 
> 
>  static const char
> 
>  cmd_ethdev_mtu_help[] = "ethdev <ethdev_name> mtu <mtu_sz>";
> 
> @@ -671,7 +672,7 @@ cmd_ethdev_parsed(void *parsed_result, __rte_unused struct cmdline *cl, void *da
> 
>               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));
> 
> +           strlcpy(config.rx.mempool_name, res->mempool, sizeof(config.rx.mempool_name));
> 
> 
> 
> Can be changed to strlcpy(config.rx.mempool_name, res->mempool->name, sizeof(config.rx.mempool_name)); ?

mempool field is of type cmdline_fixed_string_t (array of char)

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

Re: [PATCH] app/graph: fix destination buffer too small

[Mahmoud Matook]

On 06/23, Stephen Hemminger wrote:

> On Mon, 24 Jun 2024 00:09:21 +0400
> Mahmoud Maatuq <mahmoudmatook.mm@gmail.com> wrote:
> 
> > as sizeof(config.rx.mempool_name) is < sizeof(res->mempool), it's safer
> > to copy min size of them to avoide out of bound memory write.
> > 
> > Coverity issue: 415430
> > Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
> > Cc: skori@marvell.com
> > 
> > Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> 
> NAK, this could create an invalid not null terminated string.
> 
> Since mempool name is a null terminated string this should have always
> used strlcpy instead of memcpy.

in patch v2 of this it's changed to strlcpy but we got truncation
warning as source buffer size is 128 and the dest buffer size is 26 
can we incrase the size of the source buffer ? 

RE: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small

[Kiran Kumar Kokkilagadda]

> -----Original Message-----
> From: Mahmoud Matook <mahmoudmatook.mm@gmail.com>
> Sent: Thursday, June 27, 2024 1:49 AM
> To: Kiran Kumar Kokkilagadda <kirankumark@marvell.com>
> Cc: Sunil Kumar Kori <skori@marvell.com>; Rakesh Kudurumalla
> <rkudurumalla@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Jerin Jacob <jerinj@marvell.com>;
> dev@dpdk.org
> Subject: Re: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too
> small
> 
> On 06/25, Kiran Kumar Kokkilagadda wrote: > > > From: Mahmoud Maatuq
> <mahmoudmatook. mm@ gmail. com> > Sent: Tuesday, June 25, 2024 1: 31
> AM > To: Sunil Kumar Kori <skori@ marvell. com>; Rakesh Kudurumalla
> <rkudurumalla@ marvell. com>; 
> On 06/25, Kiran Kumar Kokkilagadda wrote:
> 
> >
> >
> > From: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> > Sent: Tuesday, June 25, 2024 1:31 AM
> > To: Sunil Kumar Kori <skori@marvell.com>; Rakesh Kudurumalla
> > <rkudurumalla@marvell.com>; Nithin Kumar Dabilpuram
> > <ndabilpuram@marvell.com>; Jerin Jacob <jerinj@marvell.com>
> > Cc: dev@dpdk.org; Mahmoud Maatuq
> <mahmoudmatook.mm@gmail.com>
> > Subject: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too
> > small
> >
> > as sizeof(config. rx. mempool_name) is < sizeof(res->mempool) we
> > should copy at most sizeof(config. rx. mempool_name) and replace memcpy
> with strlcpy as mempool name is a null terminated string Coverity issue:
> 415430 Fixes: 3850cb06ab9c ("app/graph:
> >
> >
> > as sizeof(config.rx.mempool_name) is < sizeof(res->mempool) we should
> >
> > copy at most sizeof(config.rx.mempool_name) and replace memcpy with
> >
> > strlcpy as mempool name is a null terminated string
> >
> >
> >
> > Coverity issue: 415430
> >
> > Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
> >
> > Cc: skori@marvell.com<mailto:skori@marvell.com>
> >
> >
> >
> > Signed-off-by: Mahmoud Maatuq
> >
> <mahmoudmatook.mm@gmail.com<mailto:mahmoudmatook.mm@gmail.co
> m>>
> >
> > ---

Acked-by: Kiran Kumar Kokkilagadda <kirankumark@marvell.com>


> >
> > v2:
> >
> > * replaced memcpy with strlcpy
> >
> > ---
> >
> >  app/graph/ethdev.c | 3 ++-
> >
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >
> >
> > diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
> >
> > index cfc1b18569..e7a02b40a9 100644
> >
> > --- a/app/graph/ethdev.c
> >
> > +++ b/app/graph/ethdev.c
> >
> > @@ -16,6 +16,7 @@
> >
> >
> >
> >  #include "ethdev_priv.h"
> >
> >  #include "module_api.h"
> >
> > +#include "rte_string_fns.h"
> >
> >
> >
> >  static const char
> >
> >  cmd_ethdev_mtu_help[] = "ethdev <ethdev_name> mtu <mtu_sz>";
> >
> > @@ -671,7 +672,7 @@ cmd_ethdev_parsed(void *parsed_result,
> > __rte_unused struct cmdline *cl, void *da
> >
> >               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));
> >
> > +           strlcpy(config.rx.mempool_name, res->mempool,
> > + sizeof(config.rx.mempool_name));
> >
> >
> >
> > Can be changed to strlcpy(config.rx.mempool_name, res->mempool-
> >name, sizeof(config.rx.mempool_name)); ?
> 
> mempool field is of type cmdline_fixed_string_t (array of char)
> 

Ok, I over looked the "res" type. 

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

Re: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small

[David Marchand]

On Thu, Jun 27, 2024 at 2:44 AM Kiran Kumar Kokkilagadda
<kirankumark@marvell.com> wrote:
> > On 06/25, Kiran Kumar Kokkilagadda wrote: > > > From: Mahmoud Maatuq
> > <mahmoudmatook. mm@ gmail. com> > Sent: Tuesday, June 25, 2024 1: 31
> > AM > To: Sunil Kumar Kori <skori@ marvell. com>; Rakesh Kudurumalla
> > <rkudurumalla@ marvell. com>;
> > On 06/25, Kiran Kumar Kokkilagadda wrote:
> > > Subject: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too
> > > small
> > >
> > > as sizeof(config. rx. mempool_name) is < sizeof(res->mempool) we
> > > should copy at most sizeof(config. rx. mempool_name) and replace memcpy
> > with strlcpy as mempool name is a null terminated string Coverity issue:
> > 415430 Fixes: 3850cb06ab9c ("app/graph:
> > >
> > >
> > > as sizeof(config.rx.mempool_name) is < sizeof(res->mempool) we should
> > >
> > > copy at most sizeof(config.rx.mempool_name) and replace memcpy with
> > >
> > > strlcpy as mempool name is a null terminated string
> > >
> > >
> > >
> > > Coverity issue: 415430
> > >
> > > Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
> > >
> > > Cc: skori@marvell.com<mailto:skori@marvell.com>
> > >
> > >
> > >
> > > Signed-off-by: Mahmoud Maatuq
> > >
> > <mahmoudmatook.mm@gmail.com<mailto:mahmoudmatook.mm@gmail.co
> > m>>
> > >
> > > ---
>
> Acked-by: Kiran Kumar Kokkilagadda <kirankumark@marvell.com>

This patch does not pass the CI, please check.


-- 
David Marchand

Re: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too small

[Mahmoud Matook]

On 07/04, David Marchand wrote:

> On Thu, Jun 27, 2024 at 2:44 AM Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com> wrote:
> > > On 06/25, Kiran Kumar Kokkilagadda wrote: > > > From: Mahmoud Maatuq
> > > <mahmoudmatook. mm@ gmail. com> > Sent: Tuesday, June 25, 2024 1: 31
> > > AM > To: Sunil Kumar Kori <skori@ marvell. com>; Rakesh Kudurumalla
> > > <rkudurumalla@ marvell. com>;
> > > On 06/25, Kiran Kumar Kokkilagadda wrote:
> > > > Subject: [EXTERNAL] [PATCH v2] app/graph: fix destination buffer too
> > > > small
> > > >
> > > > as sizeof(config. rx. mempool_name) is < sizeof(res->mempool) we
> > > > should copy at most sizeof(config. rx. mempool_name) and replace memcpy
> > > with strlcpy as mempool name is a null terminated string Coverity issue:
> > > 415430 Fixes: 3850cb06ab9c ("app/graph:
> > > >
> > > >
> > > > as sizeof(config.rx.mempool_name) is < sizeof(res->mempool) we should
> > > >
> > > > copy at most sizeof(config.rx.mempool_name) and replace memcpy with
> > > >
> > > > strlcpy as mempool name is a null terminated string
> > > >
> > > >
> > > >
> > > > Coverity issue: 415430
> > > >
> > > > Fixes: 3850cb06ab9c ("app/graph: add ethdev commands")
> > > >
> > > > Cc: skori@marvell.com<mailto:skori@marvell.com>
> > > >
> > > >
> > > >
> > > > Signed-off-by: Mahmoud Maatuq
> > > >
> > > <mahmoudmatook.mm@gmail.com<mailto:mahmoudmatook.mm@gmail.co
> > > m>>
> > > >
> > > > ---
> >
> > Acked-by: Kiran Kumar Kokkilagadda <kirankumark@marvell.com>
> 
> This patch does not pass the CI, please check.

The warning in CI is clear, (truncation warning) as the size of
destination buffer: config.rx.mempool_name is around 26 bytes but the
size of of source buffer: res->mempool is 128 bytes

Some possible solutions: 
  * increase the destination buffer size: by tracking it we need to
      increase RTE_MEMZONE_NAMESIZE, (more memory consumption). 

  * if the mem pool name is usually around the same size as destination
      we could decrease the size of source buffer, but res->mempool of
      type cmdline_fixed_string_t and maybe used somewhere else that
      could also produce truncation.

  *  change the type of source buffer and use size as destination.

what do you think?

> 
> 
> -- 
> David Marchand
>