diff mbox series

[v3] lib/rte_rib6: fix stack buffer overflow

Message ID 20210621132834.21673-1-ohilyard@iol.unh.edu (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v3] lib/rte_rib6: fix stack buffer overflow |

Checks

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

Commit Message

Owen Hilyard June 21, 2021, 1:28 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.

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(-)

Comments

David Marchand June 22, 2021, 7:10 a.m. UTC | #1 
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!
Vladimir Medvedkin June 22, 2021, 10:51 a.m. UTC | #2 
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>
Vladimir Medvedkin June 24, 2021, 9:01 a.m. UTC | #3 
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;
>   }
>   
>
diff mbox series

Patch

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