[dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow

[Stephen Hemminger]

On Wed, 16 Jun 2021 12:07:29 -0400
ohilyard at iol.unh.edu wrote:

> From: Owen Hilyard <ohilyard at iol.unh.edu>
> 
> ASAN found a stack buffer overflow in lib/rib/rte_rib6.c:get_dir.
> The fix for the stack buffer overflow was to make sure depth
> was always < 128, since when depth = 128 it caused the index
> into the ip address to be 16, which read off the end of the array.
> 
> While trying to solve the buffer overflow, I noticed that a few
> changes could be made to remove the for loop entirely.
> 
> Signed-off-by: Owen Hilyard <ohilyard at iol.unh.edu>
> ---
>  lib/rib/rte_rib6.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
> index f6c55ee45..2de50449d 100644
> --- a/lib/rib/rte_rib6.c
> +++ b/lib/rib/rte_rib6.c
> @@ -79,14 +79,20 @@ is_covered(const uint8_t ip1[RTE_RIB6_IPV6_ADDR_SIZE],
>  static inline int
>  get_dir(const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE], uint8_t depth)
>  {
> -	int i = 0;
> -	uint8_t p_depth, msk;
> -
> -	for (p_depth = depth; p_depth >= 8; p_depth -= 8)
> -		i++;
> -
> -	msk = 1 << (7 - p_depth);
> -	return (ip[i] & msk) != 0;
> +	int index, msk;
> +	/* depth & 127 clamps depth to values that will not

Please put blank line after declarations for clarity.
Since index and mask are not signed values, please make them unsigned.
Better yet, make them sized to the appropriate number of bits.

> +	 * read off the end of ip.
> +	 * depth is the number of bits deep into ip to traverse, and
> +	 * is incremented in blocks of 8 (1 byte). This means the last
> +	 * 3 bits are irrelevant to what the index of ip should be.
> +	 */
> +	index = (depth & 127) >> 3;
> +	/*
> +	 * msk is the bitmask used to extract the bit used to decide the
> +	 * direction of the next step of the binary search.
> +	 */
> +	msk = 1 << (7 - (depth & 7));
> +	return (ip[index] & msk) != 0;
>  }
>  
>  static inline struct rte_rib6_node *