[2/6] net/bnxt: fix potential data race
diff mbox series

Message ID 20200303175938.14292-3-stephen@networkplumber.org
State New
Delegated to: Ajit Khaparde
Headers show
Series
  • net/bnxt: bounds checking patches
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger March 3, 2020, 5:59 p.m. UTC
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(-)

Comments

Christopher Ertl March 3, 2020, 6:13 p.m. UTC | #1
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
Stephen Hemminger March 3, 2020, 6:16 p.m. UTC | #2
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?

Patch
diff mbox series

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++) {