| Message ID | 20200303175938.14292-3-stephen@networkplumber.org |
|---|---|
| State | New |
| Delegated to: | Ajit Khaparde |
| Headers | show |
| Series |
|
| Related | show |
| Context | Check | Description |
|---|---|---|
| ci/Intel-compilation | success | Compilation OK |
| ci/checkpatch | success | coding style OK |
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
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 --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++) {
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(-)