diff mbox series

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

Message ID 20200303175938.14292-3-stephen@networkplumber.org (mailing list archive)
State Changes Requested, archived
Delegated to: Ajit Khaparde
Headers
Series net/bnxt: bounds checking patches |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation 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?
diff mbox series

Patch

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