[PATCH] examples/l3fwd: resolve stack buffer overflow issue

[PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Rahul Bhansali]

This patch fixes the stack buffer overflow error reported
from AddressSanitizer.
Function send_packetsx4() tries to access out of bound data
from rte_mbuf and fill it into TX buffer even in the case
where no pending packets (len = 0).
Performance impact:- No

ASAN error report:-
==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800
READ of size 8 at 0xffffe2c0dcf0 thread T0
 #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
 #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226

Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
---
 examples/l3fwd/l3fwd_common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/examples/l3fwd/l3fwd_common.h b/examples/l3fwd/l3fwd_common.h
index 7d83ff641a..de77711f88 100644
--- a/examples/l3fwd/l3fwd_common.h
+++ b/examples/l3fwd/l3fwd_common.h
@@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
 
 		/* copy rest of the packets into the TX buffer. */
 		len = num - n;
+		if (len == 0)
+			goto exit;
+
 		j = 0;
 		switch (len % FWDSTEP) {
 		while (j < len) {
@@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
 		}
 	}
 
+exit:
 	qconf->tx_mbufs[port].len = len;
 }
 
-- 
2.25.1

RE: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Rahul Bhansali]

Hi Thomas,

Below patch is pending for review and merge. Can you please check it ?
http://patches.dpdk.org/project/dpdk/list/?series=21124

Regards,
Rahul


-----Original Message-----
From: Rahul Bhansali <rbhansali@marvell.com> 
Sent: Tuesday, January 11, 2022 6:20 PM
To: dev@dpdk.org; conor.walsh@intel.com; david.marchand@redhat.com
Cc: Rahul Bhansali <rbhansali@marvell.com>
Subject: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

This patch fixes the stack buffer overflow error reported from AddressSanitizer.
Function send_packetsx4() tries to access out of bound data from rte_mbuf and fill it into TX buffer even in the case where no pending packets (len = 0).
Performance impact:- No

ASAN error report:-
==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800 READ of size 8 at 0xffffe2c0dcf0 thread T0
 #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
 #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226

Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
---
 examples/l3fwd/l3fwd_common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/examples/l3fwd/l3fwd_common.h b/examples/l3fwd/l3fwd_common.h index 7d83ff641a..de77711f88 100644
--- a/examples/l3fwd/l3fwd_common.h
+++ b/examples/l3fwd/l3fwd_common.h
@@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
 
 		/* copy rest of the packets into the TX buffer. */
 		len = num - n;
+		if (len == 0)
+			goto exit;
+
 		j = 0;
 		switch (len % FWDSTEP) {
 		while (j < len) {
@@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
 		}
 	}
 
+exit:
 	qconf->tx_mbufs[port].len = len;
 }
 
--
2.25.1

RE: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Rahul Bhansali]

Ping..

-----Original Message-----
From: Rahul Bhansali <rbhansali@marvell.com> 
Sent: Tuesday, January 11, 2022 6:20 PM
To: dev@dpdk.org; conor.walsh@intel.com; david.marchand@redhat.com
Cc: Rahul Bhansali <rbhansali@marvell.com>
Subject: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

This patch fixes the stack buffer overflow error reported from AddressSanitizer.
Function send_packetsx4() tries to access out of bound data from rte_mbuf and fill it into TX buffer even in the case where no pending packets (len = 0).
Performance impact:- No

ASAN error report:-
==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800 READ of size 8 at 0xffffe2c0dcf0 thread T0
 #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
 #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226

Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
---
 examples/l3fwd/l3fwd_common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/examples/l3fwd/l3fwd_common.h b/examples/l3fwd/l3fwd_common.h index 7d83ff641a..de77711f88 100644
--- a/examples/l3fwd/l3fwd_common.h
+++ b/examples/l3fwd/l3fwd_common.h
@@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
 
 		/* copy rest of the packets into the TX buffer. */
 		len = num - n;
+		if (len == 0)
+			goto exit;
+
 		j = 0;
 		switch (len % FWDSTEP) {
 		while (j < len) {
@@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
 		}
 	}
 
+exit:
 	qconf->tx_mbufs[port].len = len;
 }
 
--
2.25.1

RE: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Walsh, Conor]

Hi Rahul,

I must have forgot to put my tag on this because I thought I reviewed it already.
I reviewed and tested it again this morning.

Thanks,
Reviewed-by: Conor Walsh <conor.walsh@intel.com>

> -----Original Message-----
> From: Rahul Bhansali <rbhansali@marvell.com>
> Sent: Monday 7 March 2022 06:45
> To: Rahul Bhansali <rbhansali@marvell.com>; dev@dpdk.org; Walsh, Conor
> <conor.walsh@intel.com>; david.marchand@redhat.com
> Subject: RE: [PATCH] examples/l3fwd: resolve stack buffer overflow issue
> 
> Ping..
> 
> -----Original Message-----
> From: Rahul Bhansali <rbhansali@marvell.com>
> Sent: Tuesday, January 11, 2022 6:20 PM
> To: dev@dpdk.org; conor.walsh@intel.com; david.marchand@redhat.com
> Cc: Rahul Bhansali <rbhansali@marvell.com>
> Subject: [PATCH] examples/l3fwd: resolve stack buffer overflow issue
> 
> This patch fixes the stack buffer overflow error reported from
> AddressSanitizer.
> Function send_packetsx4() tries to access out of bound data from rte_mbuf
> and fill it into TX buffer even in the case where no pending packets (len = 0).
> Performance impact:- No
> 
> ASAN error report:-
> ==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
> 0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800
> READ of size 8 at 0xffffe2c0dcf0 thread T0
>  #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
>  #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226
> 
> Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> ---
>  examples/l3fwd/l3fwd_common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/examples/l3fwd/l3fwd_common.h
> b/examples/l3fwd/l3fwd_common.h index 7d83ff641a..de77711f88 100644
> --- a/examples/l3fwd/l3fwd_common.h
> +++ b/examples/l3fwd/l3fwd_common.h
> @@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t
> port, struct rte_mbuf *m[],
> 
>  		/* copy rest of the packets into the TX buffer. */
>  		len = num - n;
> +		if (len == 0)
> +			goto exit;
> +
>  		j = 0;
>  		switch (len % FWDSTEP) {
>  		while (j < len) {
> @@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t
> port, struct rte_mbuf *m[],
>  		}
>  	}
> 
> +exit:
>  	qconf->tx_mbufs[port].len = len;
>  }
> 
> --
> 2.25.1

Re: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Thomas Monjalon]

11/01/2022 13:50, Rahul Bhansali:
> This patch fixes the stack buffer overflow error reported
> from AddressSanitizer.
> Function send_packetsx4() tries to access out of bound data
> from rte_mbuf and fill it into TX buffer even in the case
> where no pending packets (len = 0).
> Performance impact:- No
> 
> ASAN error report:-
> ==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
> 0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800
> READ of size 8 at 0xffffe2c0dcf0 thread T0
>  #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
>  #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226

This code comes from below commit, so these tags are missing:
Fixes: 96ff445371e0 ("examples/l3fwd: reorganise and optimize LPM code path")
Cc: stable@dpdk.org

> Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> ---
>  examples/l3fwd/l3fwd_common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/examples/l3fwd/l3fwd_common.h b/examples/l3fwd/l3fwd_common.h
> index 7d83ff641a..de77711f88 100644
> --- a/examples/l3fwd/l3fwd_common.h
> +++ b/examples/l3fwd/l3fwd_common.h
> @@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
>  
>  		/* copy rest of the packets into the TX buffer. */
>  		len = num - n;
> +		if (len == 0)
> +			goto exit;
> +

I don't understand how it can fix something.
There is already  "while (j < len)" with j and len being 0,
the loop should not be effective in this case.

>  		j = 0;
>  		switch (len % FWDSTEP) {
>  		while (j < len) {
> @@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
>  		}
>  	}
>  
> +exit:
>  	qconf->tx_mbufs[port].len = len;
>  }

RE: [EXT] Re: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Rahul Bhansali]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, March 8, 2022 4:51 PM
> To: Rahul Bhansali <rbhansali@marvell.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; Conor Walsh
> <conor.walsh@intel.com>
> Subject: [EXT] Re: [PATCH] examples/l3fwd: resolve stack buffer overflow issue
> 
> External Email
> 
> ----------------------------------------------------------------------
> 11/01/2022 13:50, Rahul Bhansali:
> > This patch fixes the stack buffer overflow error reported from
> > AddressSanitizer.
> > Function send_packetsx4() tries to access out of bound data from
> > rte_mbuf and fill it into TX buffer even in the case where no pending
> > packets (len = 0).
> > Performance impact:- No
> >
> > ASAN error report:-
> > ==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
> > 0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp
> > 0xffffe2c0d800 READ of size 8 at 0xffffe2c0dcf0 thread T0
> >  #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
> >  #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226
> 
> This code comes from below commit, so these tags are missing:
> Fixes: 96ff445371e0 ("examples/l3fwd: reorganise and optimize LPM code
> path")
> Cc: stable@dpdk.org
> 
> > Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> > ---
> >  examples/l3fwd/l3fwd_common.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/examples/l3fwd/l3fwd_common.h
> > b/examples/l3fwd/l3fwd_common.h index 7d83ff641a..de77711f88 100644
> > --- a/examples/l3fwd/l3fwd_common.h
> > +++ b/examples/l3fwd/l3fwd_common.h
> > @@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t
> > port, struct rte_mbuf *m[],
> >
> >  		/* copy rest of the packets into the TX buffer. */
> >  		len = num - n;
> > +		if (len == 0)
> > +			goto exit;
> > +
> 
> I don't understand how it can fix something.
> There is already  "while (j < len)" with j and len being 0, the loop should not be
> effective in this case.

This Switch will execute Case statement first even before considering the while condition or anything else before case statement. While condition will be executed only after all switch cases are executed.
Hence in case of len = 0 and n > 28, it is throwing stack buffer overflow error.

Below is sample code to simulate the while loop behavior inside switch. Checked it for both x86 and arm64.
https://godbolt.org/z/4Kecqbsde 

> 
> >  		j = 0;
> >  		switch (len % FWDSTEP) {
> >  		while (j < len) {
> > @@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t
> port, struct rte_mbuf *m[],
> >  		}
> >  	}
> >
> > +exit:
> >  	qconf->tx_mbufs[port].len = len;
> >  }
> 
> 

Re: [EXT] Re: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Thomas Monjalon]

09/03/2022 16:24, Rahul Bhansali:
> Hi Thomas,
> 
> From: Thomas Monjalon <thomas@monjalon.net>
> > 11/01/2022 13:50, Rahul Bhansali:
> > >  		/* copy rest of the packets into the TX buffer. */
> > >  		len = num - n;
> > > +		if (len == 0)
> > > +			goto exit;
> > > +
> > 
> > I don't understand how it can fix something.
> > There is already  "while (j < len)" with j and len being 0, the loop should not be
> > effective in this case.
> 
> This Switch will execute Case statement first even before considering the while condition or anything else before case statement. While condition will be executed only after all switch cases are executed.

I don't know this construct. Is it part of the C standard?
We learn something everyday :)

> Hence in case of len = 0 and n > 28, it is throwing stack buffer overflow error.
> 
> Below is sample code to simulate the while loop behavior inside switch. Checked it for both x86 and arm64.
> https://godbolt.org/z/4Kecqbsde 
> 
> > 
> > >  		j = 0;
> > >  		switch (len % FWDSTEP) {
> > >  		while (j < len) {

RE: [EXT] Re: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Rahul Bhansali]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, March 10, 2022 12:37 AM
> To: Rahul Bhansali <rbhansali@marvell.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; Conor Walsh
> <conor.walsh@intel.com>
> Subject: Re: [EXT] Re: [PATCH] examples/l3fwd: resolve stack buffer overflow
> issue
> 
> 09/03/2022 16:24, Rahul Bhansali:
> > Hi Thomas,
> >
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 11/01/2022 13:50, Rahul Bhansali:
> > > >  		/* copy rest of the packets into the TX buffer. */
> > > >  		len = num - n;
> > > > +		if (len == 0)
> > > > +			goto exit;
> > > > +
> > >
> > > I don't understand how it can fix something.
> > > There is already  "while (j < len)" with j and len being 0, the loop
> > > should not be effective in this case.
> >
> > This Switch will execute Case statement first even before considering the while
> condition or anything else before case statement. While condition will be
> executed only after all switch cases are executed.
> 
> I don't know this construct. Is it part of the C standard?
> We learn something everyday :)
Yeah, this is new learning for me as well 😊
It’s the way switch works and make it faster than If-else conditions executions by directly transferring control to respective case first and then do rest of the things.
Ref document: https://docs.microsoft.com/en-us/cpp/c-language/switch-statement-c?view=msvc-170

> 
> > Hence in case of len = 0 and n > 28, it is throwing stack buffer overflow error.
> >
> > Below is sample code to simulate the while loop behavior inside switch.
> Checked it for both x86 and arm64.
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__godbolt.org_z_4Ke
> > cqbsde&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=bm7kwlFq6L5uO69sS-
> 08RKSWPEU
> > tAQMUQjqHDFDtmpY&m=3GLnWHKqJB7pB5Jc36-gFYv-q-
> 3lyEtAFIK3Zt_TMRHhsAGJPIM
> > sAYTAunXt-TCf&s=L93OhPE9w3nl-Tf16rsvJ_OIC9Jar3Q7Be6vX9KfKfc&e=
> >
> > >
> > > >  		j = 0;
> > > >  		switch (len % FWDSTEP) {
> > > >  		while (j < len) {
> 
> 
> 
> 

RE: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Ananyev, Konstantin]

> This patch fixes the stack buffer overflow error reported
> from AddressSanitizer.
> Function send_packetsx4() tries to access out of bound data
> from rte_mbuf and fill it into TX buffer even in the case
> where no pending packets (len = 0).
> Performance impact:- No
> 
> ASAN error report:-
> ==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
> 0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800
> READ of size 8 at 0xffffe2c0dcf0 thread T0
>  #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
>  #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226
> 
> Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> ---
>  examples/l3fwd/l3fwd_common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/examples/l3fwd/l3fwd_common.h b/examples/l3fwd/l3fwd_common.h
> index 7d83ff641a..de77711f88 100644
> --- a/examples/l3fwd/l3fwd_common.h
> +++ b/examples/l3fwd/l3fwd_common.h
> @@ -236,6 +236,9 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
> 
>  		/* copy rest of the packets into the TX buffer. */
>  		len = num - n;
> +		if (len == 0)
> +			goto exit;
> +
>  		j = 0;
>  		switch (len % FWDSTEP) {
>  		while (j < len) {
> @@ -258,6 +261,7 @@ send_packetsx4(struct lcore_conf *qconf, uint16_t port, struct rte_mbuf *m[],
>  		}
>  	}
> 
> +exit:
>  	qconf->tx_mbufs[port].len = len;
>  }
> 
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.25.1

Re: [PATCH] examples/l3fwd: resolve stack buffer overflow issue

[Thomas Monjalon]

> > This patch fixes the stack buffer overflow error reported
> > from AddressSanitizer.
> > Function send_packetsx4() tries to access out of bound data
> > from rte_mbuf and fill it into TX buffer even in the case
> > where no pending packets (len = 0).
> > Performance impact:- No
> > 
> > ASAN error report:-
> > ==819==ERROR: AddressSanitizer: stack-buffer-overflow on address
> > 0xffffe2c0dcf0 at pc 0x0000005e791c bp 0xffffe2c0d7e0 sp 0xffffe2c0d800
> > READ of size 8 at 0xffffe2c0dcf0 thread T0
> >  #0 0x5e7918 in send_packetsx4 ../examples/l3fwd/l3fwd_common.h:251
> >  #1 0x5e7918 in send_packets_multi ../examples/l3fwd/l3fwd_neon.h:226
> > 
> > Signed-off-by: Rahul Bhansali <rbhansali@marvell.com>
> Reviewed-by: Conor Walsh <conor.walsh@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks.