[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

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

[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

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?

[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] [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
>