diff mbox series

examples/l3fwd: resolve stack buffer overflow issue

Message ID 20220111125005.554635-1-rbhansali@marvell.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series examples/l3fwd: resolve stack buffer overflow issue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS

Commit Message

Rahul Bhansali Jan. 11, 2022, 12:50 p.m. UTC 
  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(+)

Comments

Rahul Bhansali March 7, 2022, 4:27 a.m. UTC | #1 
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
Rahul Bhansali March 7, 2022, 6:45 a.m. UTC | #2 
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
Conor Walsh March 7, 2022, 10:46 a.m. UTC | #3 
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
Thomas Monjalon March 8, 2022, 11:20 a.m. UTC | #4 
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;
>  }
Rahul Bhansali March 9, 2022, 3:24 p.m. UTC | #5 
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;
> >  }
> 
>
Ananyev, Konstantin March 9, 2022, 3:57 p.m. UTC | #6 
> 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
Thomas Monjalon March 9, 2022, 7:07 p.m. UTC | #7 
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) {
Rahul Bhansali March 10, 2022, 9:38 a.m. UTC | #8 
> -----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) {
> 
> 
> 
>
Thomas Monjalon March 14, 2022, 10:16 p.m. UTC | #9 
> > 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.
diff mbox series

Patch

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