DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow
@ 2021-06-16 16:07 ohilyard
  2021-06-16 16:56 ` Stephen Hemminger
  2021-06-16 18:18 ` [dpdk-dev] [PATCH v2] " ohilyard
  0 siblings, 2 replies; 12+ messages in thread
From: ohilyard @ 2021-06-16 16:07 UTC (permalink / raw)
  To: vladimir.medvedkin; +Cc: dev, david.marchand, Owen Hilyard

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow
  2021-06-16 16:07 [dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow ohilyard
@ 2021-06-16 16:56 ` Stephen Hemminger
  2021-06-16 17:27   ` Medvedkin, Vladimir
  2021-06-16 18:18 ` [dpdk-dev] [PATCH v2] " ohilyard
  1 sibling, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2021-06-16 16:56 UTC (permalink / raw)
  To: ohilyard; +Cc: vladimir.medvedkin, dev, david.marchand

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 *


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow
  2021-06-16 16:56 ` Stephen Hemminger
@ 2021-06-16 17:27   ` Medvedkin, Vladimir
  0 siblings, 0 replies; 12+ messages in thread
From: Medvedkin, Vladimir @ 2021-06-16 17:27 UTC (permalink / raw)
  To: ohilyard; +Cc: dev, david.marchand, Stephen Hemminger

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

-- 
Regards,
Vladimir

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH v2] lib/rte_rib6: fix stack buffer overflow
  2021-06-16 16:07 [dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow ohilyard
  2021-06-16 16:56 ` Stephen Hemminger
@ 2021-06-16 18:18 ` ohilyard
  2021-06-18 16:22   ` Medvedkin, Vladimir
  2021-06-21 13:28   ` [dpdk-dev] [PATCH v3] " ohilyard
  1 sibling, 2 replies; 12+ messages in thread
From: ohilyard @ 2021-06-16 18:18 UTC (permalink / raw)
  To: vladimir.medvedkin; +Cc: dev, stephen, david.marchand, Owen Hilyard

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.

Fixes: f7e861e21c ("rib: support IPv6")

Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
---
 lib/rib/rte_rib6.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
index f6c55ee45..a4daf12ca 100644
--- a/lib/rib/rte_rib6.c
+++ b/lib/rib/rte_rib6.c
@@ -79,20 +79,31 @@ 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;
+	uint8_t 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 *
 get_nxt_node(struct rte_rib6_node *node,
 	const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE])
 {
+	if (node->depth == 128)
+		return NULL;
 	return (get_dir(ip, node->depth)) ? node->right : node->left;
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] lib/rte_rib6: fix stack buffer overflow
  2021-06-16 18:18 ` [dpdk-dev] [PATCH v2] " ohilyard
@ 2021-06-18 16:22   ` Medvedkin, Vladimir
  2021-06-18 16:27     ` Medvedkin, Vladimir
  2021-06-21 13:28   ` [dpdk-dev] [PATCH v3] " ohilyard
  1 sibling, 1 reply; 12+ messages in thread
From: Medvedkin, Vladimir @ 2021-06-18 16:22 UTC (permalink / raw)
  To: ohilyard; +Cc: dev, stephen, david.marchand

Hi Owen,

Just a few nits inlined below

On 16/06/2021 21:18, 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.
> 
> Fixes: f7e861e21c ("rib: support IPv6")
> 
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> ---
>   lib/rib/rte_rib6.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
> index f6c55ee45..a4daf12ca 100644
> --- a/lib/rib/rte_rib6.c
> +++ b/lib/rib/rte_rib6.c
> @@ -79,20 +79,31 @@ 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;
> +	uint8_t index, msk;
> +
> +	/* depth & 127 clamps depth to values that will not

Please be consistent with the coding style. Check 1.3.1 in 
https://doc.dpdk.org/guides/contributing/coding_style.html

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

(depth & INT8_MAX) / UINT8_MAX ?

> +
> +	/*
> +	 * 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 *
>   get_nxt_node(struct rte_rib6_node *node,
>   	const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE])
>   {
> +	if (node->depth == 128)
> +		return NULL;

please use RIB6_MAXDEPTH instead of 128.
Also I'd put a blank line before the final return.

>   	return (get_dir(ip, node->depth)) ? node->right : node->left;
>   }
>   
> 

while this is a bug fix, please add Cc: stable@dpdk.org on v3 to 
backport this patch.

Apart from that LGTM.

-- 
Regards,
Vladimir

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v2] lib/rte_rib6: fix stack buffer overflow
  2021-06-18 16:22   ` Medvedkin, Vladimir
@ 2021-06-18 16:27     ` Medvedkin, Vladimir
  0 siblings, 0 replies; 12+ messages in thread
From: Medvedkin, Vladimir @ 2021-06-18 16:27 UTC (permalink / raw)
  To: ohilyard; +Cc: dev, stephen, david.marchand



On 18/06/2021 19:22, Medvedkin, Vladimir wrote:
> Hi Owen,
> 
> Just a few nits inlined below
> 
> On 16/06/2021 21:18, 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.
>>
>> Fixes: f7e861e21c ("rib: support IPv6")
>>
>> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
>> ---
>>   lib/rib/rte_rib6.c | 27 +++++++++++++++++++--------
>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
>> index f6c55ee45..a4daf12ca 100644
>> --- a/lib/rib/rte_rib6.c
>> +++ b/lib/rib/rte_rib6.c
>> @@ -79,20 +79,31 @@ 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;
>> +    uint8_t index, msk;
>> +
>> +    /* depth & 127 clamps depth to values that will not
> 
> Please be consistent with the coding style. Check 1.3.1 in 
> https://doc.dpdk.org/guides/contributing/coding_style.html
> 
>> +     * 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;
> 
> (depth & INT8_MAX) / UINT8_MAX ?

Ugh...
sed 's/UINT8_MAX/CHAR_BIT'

> 
>> +
>> +    /*
>> +     * 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 *
>>   get_nxt_node(struct rte_rib6_node *node,
>>       const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE])
>>   {
>> +    if (node->depth == 128)
>> +        return NULL;
> 
> please use RIB6_MAXDEPTH instead of 128.
> Also I'd put a blank line before the final return.
> 
>>       return (get_dir(ip, node->depth)) ? node->right : node->left;
>>   }
>>
> 
> while this is a bug fix, please add Cc: stable@dpdk.org on v3 to 
> backport this patch.
> 
> Apart from that LGTM.
> 

-- 
Regards,
Vladimir

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow
  2021-06-16 18:18 ` [dpdk-dev] [PATCH v2] " ohilyard
  2021-06-18 16:22   ` Medvedkin, Vladimir
@ 2021-06-21 13:28   ` ohilyard
  2021-06-22  7:10     ` David Marchand
                       ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: ohilyard @ 2021-06-21 13:28 UTC (permalink / raw)
  To: vladimir.medvedkin; +Cc: dev, stephen, david.marchand, Owen Hilyard

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.

Fixes: f7e861e21c ("rib: support IPv6")

Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
---
 lib/rib/rte_rib6.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
index f6c55ee45..96424e9c9 100644
--- a/lib/rib/rte_rib6.c
+++ b/lib/rib/rte_rib6.c
@@ -79,20 +79,33 @@ 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;
+	uint8_t 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 & (UINT8_MAX - 1)) / CHAR_BIT;
+
+	/*
+	 * 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 *
 get_nxt_node(struct rte_rib6_node *node,
 	const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE])
 {
+	if (node->depth == RIB6_MAXDEPTH)
+		return NULL;
+
 	return (get_dir(ip, node->depth)) ? node->right : node->left;
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow
  2021-06-21 13:28   ` [dpdk-dev] [PATCH v3] " ohilyard
@ 2021-06-22  7:10     ` David Marchand
  2021-06-22 10:51       ` Medvedkin, Vladimir
  2021-06-23 15:17     ` [dpdk-dev] [PATCH v4] rib: fix max depth IPv6 lookup ohilyard
  2021-06-24  9:01     ` [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow Medvedkin, Vladimir
  2 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2021-06-22  7:10 UTC (permalink / raw)
  To: Owen Hilyard, Vladimir Medvedkin; +Cc: dev, Stephen Hemminger

On Mon, Jun 21, 2021 at 3:28 PM <ohilyard@iol.unh.edu> wrote:
>
> From: Owen Hilyard <ohilyard@iol.unh.edu>

Hi Owen, Vladimir,


Owen, two comments on the patch title.

- We (try to) never prefix with lib/, as it gives no additional info.
The prefix should be the library name.
There were some transgressions to this rule, but this was Thomas or me
being absent minded.

For other parts of the tree, it is a bit more complex, but if unsure,
the simplest is to look at the git history.
Here this is the rib library, so "rib: " is enough.


- The title purpose is to give a hint of the functional impact: people
looking for fixes for a type of bug can find it more easily.

Here, just indicating we are fixing a buffer overflow won't help judge
in which usecase the issue happenned.
How about: "rib: fix max depth IPv6 lookup"


>
> 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.
>
> Fixes: f7e861e21c ("rib: support IPv6")
Cc: stable@dpdk.org

>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>


Vladimir, can you review this fix?

Thanks!

-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow
  2021-06-22  7:10     ` David Marchand
@ 2021-06-22 10:51       ` Medvedkin, Vladimir
  0 siblings, 0 replies; 12+ messages in thread
From: Medvedkin, Vladimir @ 2021-06-22 10:51 UTC (permalink / raw)
  To: David Marchand, Owen Hilyard; +Cc: dev, Stephen Hemminger

Hi Owen, David,

Apart from David's comments looks good to me.


On 22/06/2021 10:10, David Marchand wrote:
> On Mon, Jun 21, 2021 at 3:28 PM <ohilyard@iol.unh.edu> wrote:
>>
>> From: Owen Hilyard <ohilyard@iol.unh.edu>
> 
> Hi Owen, Vladimir,
> 
> 
> Owen, two comments on the patch title.
> 
> - We (try to) never prefix with lib/, as it gives no additional info.
> The prefix should be the library name.
> There were some transgressions to this rule, but this was Thomas or me
> being absent minded.
> 
> For other parts of the tree, it is a bit more complex, but if unsure,
> the simplest is to look at the git history.
> Here this is the rib library, so "rib: " is enough.
> 
> 
> - The title purpose is to give a hint of the functional impact: people
> looking for fixes for a type of bug can find it more easily.
> 
> Here, just indicating we are fixing a buffer overflow won't help judge
> in which usecase the issue happenned.
> How about: "rib: fix max depth IPv6 lookup"
> 
> 
>>
>> 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.
>>
>> Fixes: f7e861e21c ("rib: support IPv6")
> Cc: stable@dpdk.org
> 
>>
>> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> 
> 
> Vladimir, can you review this fix?
> 
> Thanks!
> 

Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

-- 
Regards,
Vladimir

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH v4] rib: fix max depth IPv6 lookup
  2021-06-21 13:28   ` [dpdk-dev] [PATCH v3] " ohilyard
  2021-06-22  7:10     ` David Marchand
@ 2021-06-23 15:17     ` ohilyard
  2021-06-24 13:23       ` David Marchand
  2021-06-24  9:01     ` [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow Medvedkin, Vladimir
  2 siblings, 1 reply; 12+ messages in thread
From: ohilyard @ 2021-06-23 15:17 UTC (permalink / raw)
  To: vladimir.medvedkin; +Cc: dev, stable, stephen, david.marchand, Owen Hilyard

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.

Fixes: f7e861e21c ("rib: support IPv6")

Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
---
 lib/rib/rte_rib6.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
index f6c55ee45..96424e9c9 100644
--- a/lib/rib/rte_rib6.c
+++ b/lib/rib/rte_rib6.c
@@ -79,20 +79,33 @@ 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;
+	uint8_t 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 & (UINT8_MAX - 1)) / CHAR_BIT;
+
+	/*
+	 * 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 *
 get_nxt_node(struct rte_rib6_node *node,
 	const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE])
 {
+	if (node->depth == RIB6_MAXDEPTH)
+		return NULL;
+
 	return (get_dir(ip, node->depth)) ? node->right : node->left;
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow
  2021-06-21 13:28   ` [dpdk-dev] [PATCH v3] " ohilyard
  2021-06-22  7:10     ` David Marchand
  2021-06-23 15:17     ` [dpdk-dev] [PATCH v4] rib: fix max depth IPv6 lookup ohilyard
@ 2021-06-24  9:01     ` Medvedkin, Vladimir
  2 siblings, 0 replies; 12+ messages in thread
From: Medvedkin, Vladimir @ 2021-06-24  9:01 UTC (permalink / raw)
  To: ohilyard; +Cc: dev, stephen, david.marchand

Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

On 21/06/2021 16:28, 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.
> 
> Fixes: f7e861e21c ("rib: support IPv6")
> 
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> ---
>   lib/rib/rte_rib6.c | 29 +++++++++++++++++++++--------
>   1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
> index f6c55ee45..96424e9c9 100644
> --- a/lib/rib/rte_rib6.c
> +++ b/lib/rib/rte_rib6.c
> @@ -79,20 +79,33 @@ 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;
> +	uint8_t 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 & (UINT8_MAX - 1)) / CHAR_BIT;
> +
> +	/*
> +	 * 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 *
>   get_nxt_node(struct rte_rib6_node *node,
>   	const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE])
>   {
> +	if (node->depth == RIB6_MAXDEPTH)
> +		return NULL;
> +
>   	return (get_dir(ip, node->depth)) ? node->right : node->left;
>   }
>   
> 

-- 
Regards,
Vladimir

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v4] rib: fix max depth IPv6 lookup
  2021-06-23 15:17     ` [dpdk-dev] [PATCH v4] rib: fix max depth IPv6 lookup ohilyard
@ 2021-06-24 13:23       ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2021-06-24 13:23 UTC (permalink / raw)
  To: Owen Hilyard; +Cc: Vladimir Medvedkin, dev, dpdk stable, Stephen Hemminger

On Wed, Jun 23, 2021 at 5:17 PM <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.
>
> Fixes: f7e861e21c ("rib: support IPv6")
Updated sha1.
Cc: stable@dpdk.org

>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

Applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-06-24 13:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 16:07 [dpdk-dev] [PATCH] lib/rte_rib6: fix stack buffer overflow ohilyard
2021-06-16 16:56 ` Stephen Hemminger
2021-06-16 17:27   ` Medvedkin, Vladimir
2021-06-16 18:18 ` [dpdk-dev] [PATCH v2] " ohilyard
2021-06-18 16:22   ` Medvedkin, Vladimir
2021-06-18 16:27     ` Medvedkin, Vladimir
2021-06-21 13:28   ` [dpdk-dev] [PATCH v3] " ohilyard
2021-06-22  7:10     ` David Marchand
2021-06-22 10:51       ` Medvedkin, Vladimir
2021-06-23 15:17     ` [dpdk-dev] [PATCH v4] rib: fix max depth IPv6 lookup ohilyard
2021-06-24 13:23       ` David Marchand
2021-06-24  9:01     ` [dpdk-dev] [PATCH v3] lib/rte_rib6: fix stack buffer overflow Medvedkin, Vladimir

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).