diff mbox series

lib/rte_rib6: fix stack buffer overflow

Message ID 20210616160730.348523-1-ohilyard@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: David Marchand
Headers show
Series lib/rte_rib6: fix stack buffer overflow | expand

Checks

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

Commit Message

Owen Hilyard June 16, 2021, 4:07 p.m. UTC
From: Owen Hilyard <ohilyard@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@iol.unh.edu>
---
 lib/rib/rte_rib6.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Stephen Hemminger June 16, 2021, 4:56 p.m. UTC | #1
On Wed, 16 Jun 2021 12:07:29 -0400
ohilyard@iol.unh.edu wrote:

> From: Owen Hilyard <ohilyard@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@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 *
Medvedkin, Vladimir June 16, 2021, 5:27 p.m. UTC | #2
Hi Owen,

Thanks for the fix.

I like your solution with removing the loop. However, while this fixes 
the buffer overflow, IMO it is not complete, because get_dir() shouldn't 
be called in cases where depth = 128. In this case checking the MSB of 
the ip is not quite right thing.
The only place where it is possible (depth == 128) is on calling 
get_nxt_node() from rte_rib6_lookup(), so I would suggest adding 
something like this:

if (depth == 128)
	return NULL;

to get_nxt_node() along with your changes.

Also, apart from Stephen's comments, please add the corresponding 
fixline to the v2.

Thanks!


On 16/06/2021 19:56, Stephen Hemminger wrote:
> On Wed, 16 Jun 2021 12:07:29 -0400
> ohilyard@iol.unh.edu wrote:
> 
>> From: Owen Hilyard <ohilyard@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@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 *
>
diff mbox series

Patch

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
+	 * 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 *