[dpdk-dev] [PATCH 0/6] net/bnxt: bounds checking patches

[dpdk-dev] [PATCH 0/6] net/bnxt: bounds checking patches

[Stephen Hemminger]

This set of patches came from security review of bnxt driver.
It introduces a set of overflow macros that could be more widely
used in other places in DPDK to check for math overflows.

Stephen Hemminger (6):
  eal: add portable way to check for math overflow
  net/bnxt: fix potential data race
  net/bnxt: avoid potential out of bounds read
  net/bnxt: check for integer overflow in buffer sizing
  net/bnxt: add integer underflow check
  net/bnxt: sanitize max_l2_ctx

 drivers/net/bnxt/bnxt_hwrm.c                 | 31 ++++++--
 lib/librte_eal/common/Makefile               |  2 +-
 lib/librte_eal/common/include/rte_overflow.h | 74 ++++++++++++++++++++
 3 files changed, 101 insertions(+), 6 deletions(-)
 create mode 100644 lib/librte_eal/common/include/rte_overflow.h

-- 
2.20.1

[dpdk-dev] [PATCH 1/6] eal: add portable way to check for math overflow

[Stephen Hemminger]

Clang and recent versions of GCC has builtin functions to do most math
operations and check for wraparound. On most architectures this is a
just a math operation followed by a branch on carry set.

But DPDK needs to be able to handle older GCC versions, and other
compilers so a wrapper macro is needed.

Chose to use a macro instead of inline functions to avoid having
to write lots of variants for each numeric type.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/common/Makefile               |  2 +-
 lib/librte_eal/common/include/rte_overflow.h | 74 ++++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_eal/common/include/rte_overflow.h

diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index c2c6d92cd377..3794128b75b7 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -13,7 +13,7 @@ INC += rte_tailq.h rte_interrupts.h rte_alarm.h
 INC += rte_string_fns.h rte_version.h
 INC += rte_eal_memconfig.h
 INC += rte_hexdump.h rte_devargs.h rte_bus.h rte_dev.h rte_class.h
-INC += rte_option.h
+INC += rte_option.h rte_overflow.h
 INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
 INC += rte_malloc.h rte_keepalive.h rte_time.h
 INC += rte_service.h rte_service_component.h
diff --git a/lib/librte_eal/common/include/rte_overflow.h b/lib/librte_eal/common/include/rte_overflow.h
new file mode 100644
index 000000000000..d61791371029
--- /dev/null
+++ b/lib/librte_eal/common/include/rte_overflow.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Microsoft Corp
+ */
+#ifndef RTE_OVERFLOW_H_
+#define RTE_OVERFLOW_H_
+/**
+ * @file
+ *
+ * Math functions with overflow checking.
+ * Wrappers for the __builtin_XXX_overflow functions that exist
+ * in recent versions of GCC and CLANG but may not exist
+ * in older compilers. They are macros to allow use with any
+ * size of unsigned number.
+ *
+ * See:
+ *  https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
+ *  https://github.com/nemequ/portable-snippets/tree/master/safe-math
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdbool.h>
+
+#if defined(__has_builtin)
+#    if __has_builtin(__builtin_add_overflow)
+#        define RTE_HAVE_BUILTIN_OVERFLOW
+#    endif
+#elif defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 5000)
+#    define RTE__HAVE_BUILTIN_OVERFLOW
+#endif
+
+/**
+ * Safely add two bit unsigned numbers
+ * @param a
+ *   One operand
+ * @param b
+ *   Other operand
+ * @param res
+ *   Pointer to the where result of a + b is stored.
+ *   Must not be NULL
+ * @return
+ *   return true if the result overflows and is therefore truncated.
+ */
+#ifdef RTE_HAVE_BUILTIN_OVERFLOW
+#define rte_add_overflow(a, b, res) __builtin_add_overflow(a, b, res)
+#else
+#define rte_add_overflow(a, b, res) ({ *res = a + b; *res < a; })
+#endif
+
+/**
+ * Safely multiply two unsigned numbers
+ * @param a
+ *   One operand
+ * @param b
+ *   Other operand
+ * @param res
+ *   Pointer to the where result of a + b is stored.
+ *   Must not be NULL
+ * @return
+ *   return true if the result overflows and is therefore truncated.
+ */
+#ifdef RTE_HAVE_BUILTIN_OVERFLOW
+#define rte_mul_overflow(a, b, res) __builtin_mul_overflow(a, b, res)
+#else
+#define rte_mul_overflow(a, b, res) ({ *res = a * b; *res < a; })
+#endif
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_OVERFLOW_H_ */
-- 
2.20.1

[dpdk-dev] [PATCH 2/6] net/bnxt: fix potential data race

[Stephen Hemminger]

The response from the firmware is accessed multiple times.
This is a potential TOCTOU error.

Reported-by: Christopher Ertl <Christopher.Ertl@microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_hwrm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index a9c9c7297cab..20e2f6a36713 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -3746,6 +3746,7 @@ int bnxt_hwrm_port_led_qcaps(struct bnxt *bp)
 {
 	struct hwrm_port_led_qcaps_output *resp = bp->hwrm_cmd_resp_addr;
 	struct hwrm_port_led_qcaps_input req = {0};
+	uint8_t num_leds;
 	int rc;
 
 	if (BNXT_VF(bp))
@@ -3757,10 +3758,11 @@ int bnxt_hwrm_port_led_qcaps(struct bnxt *bp)
 
 	HWRM_CHECK_RESULT();
 
-	if (resp->num_leds > 0 && resp->num_leds < BNXT_MAX_LED) {
+	num_leds = resp->num_leds;
+	if (num_leds > 0 && num_leds < BNXT_MAX_LED) {
 		unsigned int i;
 
-		bp->num_leds = resp->num_leds;
+		bp->num_leds = num_leds;
 		memcpy(bp->leds, &resp->led0_id,
 			sizeof(bp->leds[0]) * bp->num_leds);
 		for (i = 0; i < bp->num_leds; i++) {
-- 
2.20.1

[dpdk-dev] [PATCH 3/6] net/bnxt: avoid potential out of bounds read

[Stephen Hemminger]

If hardware returned a bogus number of vnic's  from the
query it could cause an out of bounds read into vnic table.

Reported-by: Christopher Ertl <Christopher.Ertl@microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_hwrm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 20e2f6a36713..ad8bdb1c2913 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -4029,6 +4029,12 @@ static int bnxt_hwrm_func_vf_vnic_query(struct bnxt *bp, uint16_t vf,
 
 	HWRM_UNLOCK();
 
+	if (rc > bp->pf.total_vnics) {
+		PMD_DRV_LOG(ERR,
+			    "Vnic id %d is out of range\n", rc);
+		return -EINVAL;
+	}
+
 	return rc;
 }
 
-- 
2.20.1

[dpdk-dev] [PATCH 4/6] net/bnxt: check for integer overflow in buffer sizing

[Stephen Hemminger]

If the hardware returns invalid values, the buffer size calculation
could overflow.  Check for this by using the GCC/Clang builtin
that checks.

Reported-by: Christopher Ertl <Christopher.Ertl@microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_hwrm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index ad8bdb1c2913..6beb215d604f 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -11,6 +11,7 @@
 #include <rte_malloc.h>
 #include <rte_memzone.h>
 #include <rte_version.h>
+#include <rte_overflow.h>
 #include <rte_io.h>
 
 #include "bnxt.h"
@@ -3861,7 +3862,9 @@ int bnxt_get_nvram_directory(struct bnxt *bp, uint32_t len, uint8_t *data)
 	len -= 2;
 	memset(data, 0xff, len);
 
-	buflen = dir_entries * entry_length;
+	if (rte_mul_overflow(dir_entries, entry_length, &buflen))
+		return -EINVAL;
+
 	buf = rte_malloc("nvm_dir", buflen, 0);
 	if (buf == NULL)
 		return -ENOMEM;
-- 
2.20.1

[dpdk-dev] [PATCH 5/6] net/bnxt: add integer underflow check

[Stephen Hemminger]

If a request to read a small value is passed to nvram reader
it would underflow.

Reported-by: Christopher Ertl <Christopher.Ertl@microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_hwrm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 6beb215d604f..d878320aa110 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -3859,6 +3859,9 @@ int bnxt_get_nvram_directory(struct bnxt *bp, uint32_t len, uint8_t *data)
 
 	*data++ = dir_entries;
 	*data++ = entry_length;
+	if (len <= 2)
+		return -EINVAL;
+
 	len -= 2;
 	memset(data, 0xff, len);
 
-- 
2.20.1

[dpdk-dev] [PATCH 6/6] net/bnxt: sanitize max_l2_ctx

[Stephen Hemminger]

If max_l2_ctx is very large, then adding the additional value
could cause wraparound.

Reported-by: Christopher Ertl <Christopher.Ertl@microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_hwrm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index d878320aa110..099e04fa550a 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -651,8 +651,15 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	bp->first_vf_id = rte_le_to_cpu_16(resp->first_vf_id);
 	bp->max_rx_em_flows = rte_le_to_cpu_16(resp->max_rx_em_flows);
 	bp->max_l2_ctx = rte_le_to_cpu_16(resp->max_l2_ctxs);
-	if (!BNXT_CHIP_THOR(bp))
-		bp->max_l2_ctx += bp->max_rx_em_flows;
+	if (!BNXT_CHIP_THOR(bp)) {
+		uint16_t max_l2_ctx;
+
+		if (rte_add_overflow(bp->max_l2_ctx, bp->max_rx_em_flows,
+				     &max_l2_ctx))
+			return -EINVAL;
+		bp->max_l2_ctx = max_l2_ctx;
+	}
+
 	/* TODO: For now, do not support VMDq/RFS on VFs. */
 	if (BNXT_PF(bp)) {
 		if (bp->pf.max_vfs)
-- 
2.20.1

Re: [dpdk-dev] [EXTERNAL] [PATCH 2/6] net/bnxt: fix potential data race

[Christopher Ertl]

Can we add the `volatile` qualifier to the `hwrm_cmd_resp_addr` member (in drivers/net/bnxt/bnxt.h) to get a stronger guarantee that the compiler won't insert TOCTOU races in the output?

Christopher Ertl | MSRC Vulnerabilities & Mitigations | Microsoft Limited | +44 7773976589 | Christopher.Ertl@microsoft.com
 
Microsoft Limited (company number 01624297) is a company registered in England and Wales whose registered office is at Microsoft Campus, Thames Valley Park, Reading. RG6 1WG

-----Original Message-----
From: Stephen Hemminger <stephen@networkplumber.org> 
Sent: Tuesday, March 3, 2020 6:00 PM
To: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>; somnath.kotur@broadcom.com
Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org>; Christopher Ertl <Christopher.Ertl@microsoft.com>
Subject: [EXTERNAL] [PATCH 2/6] net/bnxt: fix potential data race

The response from the firmware is accessed multiple times.
This is a potential TOCTOU error.

Reported-by: Christopher Ertl <Christopher.Ertl@microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_hwrm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c index a9c9c7297cab..20e2f6a36713 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -3746,6 +3746,7 @@ int bnxt_hwrm_port_led_qcaps(struct bnxt *bp)  {
 	struct hwrm_port_led_qcaps_output *resp = bp->hwrm_cmd_resp_addr;
 	struct hwrm_port_led_qcaps_input req = {0};
+	uint8_t num_leds;
 	int rc;
 
 	if (BNXT_VF(bp))
@@ -3757,10 +3758,11 @@ int bnxt_hwrm_port_led_qcaps(struct bnxt *bp)
 
 	HWRM_CHECK_RESULT();
 
-	if (resp->num_leds > 0 && resp->num_leds < BNXT_MAX_LED) {
+	num_leds = resp->num_leds;
+	if (num_leds > 0 && num_leds < BNXT_MAX_LED) {
 		unsigned int i;
 
-		bp->num_leds = resp->num_leds;
+		bp->num_leds = num_leds;
 		memcpy(bp->leds, &resp->led0_id,
 			sizeof(bp->leds[0]) * bp->num_leds);
 		for (i = 0; i < bp->num_leds; i++) {
--
2.20.1

Re: [dpdk-dev] [EXTERNAL] [PATCH 2/6] net/bnxt: fix potential data race

[Stephen Hemminger]

On Tue, 3 Mar 2020 18:13:22 +0000
Christopher Ertl <Christopher.Ertl@microsoft.com> wrote:

> Can we add the `volatile` qualifier to the `hwrm_cmd_resp_addr` member (in drivers/net/bnxt/bnxt.h) to get a stronger guarantee that the compiler won't insert TOCTOU races in the output?
> 
> Christopher Ertl | MSRC Vulnerabilities & Mitigations | Microsoft Limited | +44 7773976589 | Christopher.Ertl@microsoft.com
>  
> Microsoft Limited (company number 01624297) is a company registered in England and Wales whose registered office is at Microsoft Campus, Thames Valley Park, Reading. RG6 1WG

Should this be done for all the registers in that structure?

Re: [dpdk-dev] [PATCH 1/6] eal: add portable way to check for math overflow

[Dmitry Kozlyuk]

> +#if defined(__has_builtin)
> +#    if __has_builtin(__builtin_add_overflow)
> +#        define RTE_HAVE_BUILTIN_OVERFLOW
> +#    endif
> +#elif defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 5000)
> +#    define RTE__HAVE_BUILTIN_OVERFLOW

Excessive underline after RTE results in RTE_HAVE_BUILTIN_OVERFLOW not being
defined.

-- 
Dmitry Kozlyuk

Re: [dpdk-dev] [PATCH 0/6] net/bnxt: bounds checking patches

[Ferruh Yigit]

On 3/3/2020 5:59 PM, Stephen Hemminger wrote:
> This set of patches came from security review of bnxt driver.
> It introduces a set of overflow macros that could be more widely
> used in other places in DPDK to check for math overflows.
> 
> Stephen Hemminger (6):
>   eal: add portable way to check for math overflow
>   net/bnxt: fix potential data race
>   net/bnxt: avoid potential out of bounds read
>   net/bnxt: check for integer overflow in buffer sizing
>   net/bnxt: add integer underflow check
>   net/bnxt: sanitize max_l2_ctx
> 

Hi Ajit,

I can see this patchset has been merged into your tree, although the note in the
mail list is missing. Since it has eal changes, I believe they should be
reviewed first before merging into brcm tree, can you separate the eal and
dependent patch for review, we can proceed with rest?

Regards,
ferruh

Re: [dpdk-dev] [PATCH 0/6] net/bnxt: bounds checking patches

[Ajit Khaparde]

On Tue, Mar 31, 2020 at 5:23 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/3/2020 5:59 PM, Stephen Hemminger wrote:
> > This set of patches came from security review of bnxt driver.
> > It introduces a set of overflow macros that could be more widely
> > used in other places in DPDK to check for math overflows.
> >
> > Stephen Hemminger (6):
> >   eal: add portable way to check for math overflow
> >   net/bnxt: fix potential data race
> >   net/bnxt: avoid potential out of bounds read
> >   net/bnxt: check for integer overflow in buffer sizing
> >   net/bnxt: add integer underflow check
> >   net/bnxt: sanitize max_l2_ctx
> >
>
> Hi Ajit,
>
> I can see this patchset has been merged into your tree, although the note
> in the
> mail list is missing. Since it has eal changes, I believe they should be
> reviewed first before merging into brcm tree, can you separate the eal and
> dependent patch for review, we can proceed with rest?
>
I don't mind.
But being original owner - Stephen, do you want to go ahead?

Thanks
Ajit

>
> Regards,
> ferruh
>

Re: [dpdk-dev] [PATCH 0/6] net/bnxt: bounds checking patches

[Stephen Hemminger]

On Tue, 31 Mar 2020 10:52:47 -0700
Ajit Khaparde <ajit.khaparde@broadcom.com> wrote:

> On Tue, Mar 31, 2020 at 5:23 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 3/3/2020 5:59 PM, Stephen Hemminger wrote:  
> > > This set of patches came from security review of bnxt driver.
> > > It introduces a set of overflow macros that could be more widely
> > > used in other places in DPDK to check for math overflows.
> > >
> > > Stephen Hemminger (6):
> > >   eal: add portable way to check for math overflow
> > >   net/bnxt: fix potential data race
> > >   net/bnxt: avoid potential out of bounds read
> > >   net/bnxt: check for integer overflow in buffer sizing
> > >   net/bnxt: add integer underflow check
> > >   net/bnxt: sanitize max_l2_ctx
> > >  
> >
> > Hi Ajit,
> >
> > I can see this patchset has been merged into your tree, although the note
> > in the
> > mail list is missing. Since it has eal changes, I believe they should be
> > reviewed first before merging into brcm tree, can you separate the eal and
> > dependent patch for review, we can proceed with rest?
> >  
> I don't mind.
> But being original owner - Stephen, do you want to go ahead?
> 
> Thanks
> Ajit
> 
> >
> > Regards,
> > ferruh
> >  

Sure, I expected normal review cycle on this.
Hoped that other drivers and eal core would also add overflow checks

Re: [dpdk-dev] [PATCH 0/6] net/bnxt: bounds checking patches

[Thomas Monjalon]

Why there was no progress on this during 6 months?

Ajit, it was out of my radar because delegated to you.

Please restart fresh with a separate patch for EAL,
addressing comments. Thanks


31/03/2020 20:04, Stephen Hemminger:
> On Tue, 31 Mar 2020 10:52:47 -0700
> Ajit Khaparde <ajit.khaparde@broadcom.com> wrote:
> 
> > On Tue, Mar 31, 2020 at 5:23 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > 
> > > On 3/3/2020 5:59 PM, Stephen Hemminger wrote:  
> > > > This set of patches came from security review of bnxt driver.
> > > > It introduces a set of overflow macros that could be more widely
> > > > used in other places in DPDK to check for math overflows.
> > > >
> > > > Stephen Hemminger (6):
> > > >   eal: add portable way to check for math overflow
> > > >   net/bnxt: fix potential data race
> > > >   net/bnxt: avoid potential out of bounds read
> > > >   net/bnxt: check for integer overflow in buffer sizing
> > > >   net/bnxt: add integer underflow check
> > > >   net/bnxt: sanitize max_l2_ctx
> > > >  
> > >
> > > Hi Ajit,
> > >
> > > I can see this patchset has been merged into your tree, although the note
> > > in the
> > > mail list is missing. Since it has eal changes, I believe they should be
> > > reviewed first before merging into brcm tree, can you separate the eal and
> > > dependent patch for review, we can proceed with rest?
> > >  
> > I don't mind.
> > But being original owner - Stephen, do you want to go ahead?
> > 
> > Thanks
> > Ajit
> > 
> > >
> > > Regards,
> > > ferruh
> > >  
> 
> Sure, I expected normal review cycle on this.
> Hoped that other drivers and eal core would also add overflow checks
>