lib/rte_rib6: fix stack buffer overflow
Checks
Commit Message
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
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 *
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 *
>
@@ -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 *