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

[Rahul Bhansali <rbhansali@marvell.com>]

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