[PATCH V3 1/3] brcmfmac: Avoid possible out-of-bounds read

[Kevin Cernekee]

In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before
the length of rxframe is validated.  This could lead to uninitialized
data being accessed (but not printed).  Since we already have a
perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec,
and ch.chspec is not modified by decchspec(), avoid the extra
assignment and use ch.chspec in the debug print.

Suggested-by: Mattias Nissler <mniss...@chromium.org>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


V2->V3: No change


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 2ce675ab40ef..1c450c0727cb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -1853,7 +1853,6 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if 
*ifp,
struct afx_hdl *afx_hdl = >afx_hdl;
struct brcmf_cfg80211_vif *vif = ifp->vif;
struct brcmf_rx_mgmt_data *rxframe = (struct brcmf_rx_mgmt_data *)data;
-   u16 chanspec = be16_to_cpu(rxframe->chanspec);
struct brcmu_chan ch;
u8 *mgmt_frame;
u32 mgmt_frame_len;
@@ -1906,7 +1905,7 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if 
*ifp,
cfg80211_rx_mgmt(>wdev, freq, 0, mgmt_frame, mgmt_frame_len, 0);
 
brcmf_dbg(INFO, "mgmt_frame_len (%d) , e->datalen (%d), chanspec 
(%04x), freq (%d)\n",
- mgmt_frame_len, e->datalen, chanspec, freq);
+ mgmt_frame_len, e->datalen, ch.chspec, freq);
 
return 0;
 }
-- 
2.14.1.690.gbb1197296e-goog

[PATCH V3 3/3] brcmfmac: Add check for short event packets

[Kevin Cernekee]

The length of the data in the received skb is currently passed into
brcmf_fweh_process_event() as packet_len, but this value is not checked.
event_packet should be followed by DATALEN bytes of additional event
data.  Ensure that the received packet actually contains at least
DATALEN bytes of additional data, to avoid copying uninitialized memory
into event->data.

Suggested-by: Mattias Nissler <mniss...@chromium.org>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


V2->V3: Change '<' to '>' and retest


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 27e661fa356f..e7eaa57d11d9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -424,7 +424,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
if (code != BRCMF_E_IF && !fweh->evt_handler[code])
return;
 
-   if (datalen > BRCMF_DCMD_MAXLEN)
+   if (datalen > BRCMF_DCMD_MAXLEN ||
+   datalen + sizeof(*event_packet) > packet_len)
return;
 
if (in_interrupt())
-- 
2.14.1.690.gbb1197296e-goog

[PATCH V3 2/3] brcmfmac: Delete redundant length check

[Kevin Cernekee]

brcmf_fweh_process_event() sets event->datalen to the
endian-swapped value of event_packet->msg.datalen, which is the
same as emsg.datalen.  This length is already validated in
brcmf_fweh_process_event(), so there is no need to check it
again upon dequeuing the event.

Suggested-by: Arend van Spriel <arend.vanspr...@broadcom.com>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 5 -
 1 file changed, 5 deletions(-)


V2->V3: No change


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 4eb1e1ce9ace..27e661fa356f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -257,11 +257,6 @@ static void brcmf_fweh_event_worker(struct work_struct 
*work)
brcmf_dbg_hex_dump(BRCMF_EVENT_ON(), event->data,
   min_t(u32, emsg.datalen, 64),
   "event payload, len=%d\n", emsg.datalen);
-   if (emsg.datalen > event->datalen) {
-   brcmf_err("event invalid length header=%d, msg=%d\n",
- event->datalen, emsg.datalen);
-   goto event_free;
-   }
 
/* special handling of interface event */
if (event->code == BRCMF_E_IF) {
-- 
2.14.1.690.gbb1197296e-goog

Re: [PATCH 3/3] brcmfmac: Add check for short event packets

[Kevin Cernekee]

On Mon, Sep 11, 2017 at 12:09 PM, Arend van Spriel
 wrote:
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>>> index 5aabdc9ed7e0..4cad1f0d2a82 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>>> @@ -429,7 +429,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
>>>  if (code != BRCMF_E_IF && !fweh->evt_handler[code])
>>>  return;
>>>
>>> -   if (datalen > BRCMF_DCMD_MAXLEN)
>>> +   if (datalen > BRCMF_DCMD_MAXLEN ||
>>> +   datalen + sizeof(*event_packet) < packet_len)
>>
>>
>> Shouldn't this check be larger-than, i.e. we need the packet to be at
>> least sizeof(*event_packet) + its payload size?
>
> That depends on how you formulate the requirement. packet_len here is the
> length for the received skbuff. The event message (= sizeof(*event_packet))
> and its variable payload (= datalen) shall not exceed length of received
> skbuff (= packet_len).

Or should it be an exact match, i.e. datalen + sizeof(*event_packet)
!= packet_len

What did Franky's version of the check look like?

If Broadcom has a test suite that tries different event types and
notices if events are getting unexpectedly dropped, that would be
helpful in validating the change.  I would be wary of pushing this to
-stable until we know the check is 100% correct.

[PATCH V2 2/3] brcmfmac: Delete redundant length check

[Kevin Cernekee]

brcmf_fweh_process_event() sets event->datalen to the
endian-swapped value of event_packet->msg.datalen, which is the
same as emsg.datalen.  This length is already validated in
brcmf_fweh_process_event(), so there is no need to check it
again upon dequeuing the event.

Suggested-by: Arend van Spriel <arend.vanspr...@broadcom.com>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 5 -
 1 file changed, 5 deletions(-)


V1->V2: Delete the check instead of moving it.


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 4eb1e1ce9ace..27e661fa356f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -257,11 +257,6 @@ static void brcmf_fweh_event_worker(struct work_struct 
*work)
brcmf_dbg_hex_dump(BRCMF_EVENT_ON(), event->data,
   min_t(u32, emsg.datalen, 64),
   "event payload, len=%d\n", emsg.datalen);
-   if (emsg.datalen > event->datalen) {
-   brcmf_err("event invalid length header=%d, msg=%d\n",
- event->datalen, emsg.datalen);
-   goto event_free;
-   }
 
/* special handling of interface event */
if (event->code == BRCMF_E_IF) {
-- 
2.14.1.581.gf28d330327-goog

[PATCH V2 1/3] brcmfmac: Avoid possible out-of-bounds read

[Kevin Cernekee]

In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before
the length of rxframe is validated.  This could lead to uninitialized
data being accessed (but not printed).  Since we already have a
perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec,
and ch.chspec is not modified by decchspec(), avoid the extra
assignment and use ch.chspec in the debug print.

Suggested-by: Mattias Nissler <mniss...@chromium.org>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


V1->V2: Clarify changelog re: whether the uninitialized data is printed.


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 2ce675ab40ef..1c450c0727cb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -1853,7 +1853,6 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if 
*ifp,
struct afx_hdl *afx_hdl = >afx_hdl;
struct brcmf_cfg80211_vif *vif = ifp->vif;
struct brcmf_rx_mgmt_data *rxframe = (struct brcmf_rx_mgmt_data *)data;
-   u16 chanspec = be16_to_cpu(rxframe->chanspec);
struct brcmu_chan ch;
u8 *mgmt_frame;
u32 mgmt_frame_len;
@@ -1906,7 +1905,7 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if 
*ifp,
cfg80211_rx_mgmt(>wdev, freq, 0, mgmt_frame, mgmt_frame_len, 0);
 
brcmf_dbg(INFO, "mgmt_frame_len (%d) , e->datalen (%d), chanspec 
(%04x), freq (%d)\n",
- mgmt_frame_len, e->datalen, chanspec, freq);
+ mgmt_frame_len, e->datalen, ch.chspec, freq);
 
return 0;
 }
-- 
2.14.1.581.gf28d330327-goog

[PATCH V2 3/3] brcmfmac: Add check for short event packets

[Kevin Cernekee]

The length of the data in the received skb is currently passed into
brcmf_fweh_process_event() as packet_len, but this value is not checked.
event_packet should be followed by DATALEN bytes of additional event
data.  Ensure that the received packet actually contains at least
DATALEN bytes of additional data, to avoid copying uninitialized memory
into event->data.

Suggested-by: Mattias Nissler <mniss...@chromium.org>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


V1->V2: No change.


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 27e661fa356f..28361bb865f3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -424,7 +424,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
if (code != BRCMF_E_IF && !fweh->evt_handler[code])
return;
 
-   if (datalen > BRCMF_DCMD_MAXLEN)
+   if (datalen > BRCMF_DCMD_MAXLEN ||
+   datalen + sizeof(*event_packet) < packet_len)
return;
 
if (in_interrupt())
-- 
2.14.1.581.gf28d330327-goog

[PATCH 1/3] brcmfmac: Avoid possible out-of-bounds read

[Kevin Cernekee]

In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before
the length of rxframe is validated.  This could lead to uninitialized
data being printed in a debug message.  Since we already have a
perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec,
and ch.chspec is not modified by decchspec(), avoid the extra
assignment and use ch.chspec in the debug print.

Suggested-by: Mattias Nissler <mniss...@chromium.org>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 2ce675ab40ef..1c450c0727cb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -1853,7 +1853,6 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if 
*ifp,
struct afx_hdl *afx_hdl = >afx_hdl;
struct brcmf_cfg80211_vif *vif = ifp->vif;
struct brcmf_rx_mgmt_data *rxframe = (struct brcmf_rx_mgmt_data *)data;
-   u16 chanspec = be16_to_cpu(rxframe->chanspec);
struct brcmu_chan ch;
u8 *mgmt_frame;
u32 mgmt_frame_len;
@@ -1906,7 +1905,7 @@ s32 brcmf_p2p_notify_rx_mgmt_p2p_probereq(struct brcmf_if 
*ifp,
cfg80211_rx_mgmt(>wdev, freq, 0, mgmt_frame, mgmt_frame_len, 0);
 
brcmf_dbg(INFO, "mgmt_frame_len (%d) , e->datalen (%d), chanspec 
(%04x), freq (%d)\n",
- mgmt_frame_len, e->datalen, chanspec, freq);
+ mgmt_frame_len, e->datalen, ch.chspec, freq);
 
return 0;
 }
-- 
2.14.1.581.gf28d330327-goog

[PATCH 3/3] brcmfmac: Add check for short event packets

[Kevin Cernekee]

The length of the data in the received skb is currently passed into
brcmf_fweh_process_event() as packet_len, but this value is not checked.
event_packet should be followed by DATALEN bytes of additional event
data.  Ensure that the received packet actually contains at least
DATALEN bytes of additional data, to avoid copying uninitialized memory
into event->data.

Suggested-by: Mattias Nissler <mniss...@chromium.org>
Signed-off-by: Kevin Cernekee <cerne...@chromium.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 5aabdc9ed7e0..4cad1f0d2a82 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -429,7 +429,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
if (code != BRCMF_E_IF && !fweh->evt_handler[code])
return;
 
-   if (datalen > BRCMF_DCMD_MAXLEN)
+   if (datalen > BRCMF_DCMD_MAXLEN ||
+   datalen + sizeof(*event_packet) < packet_len)
return;
 
if (in_interrupt())
-- 
2.14.1.581.gf28d330327-goog

[PATCH 0/3] New brcmfmac bounds checks

[Kevin Cernekee]

These were suggested by the Chrome OS security team[1].  Compile-tested
only.

[1] http://crosreview.com/656260

Kevin Cernekee (3):
  brcmfmac: Avoid possible out-of-bounds read
  brcmfmac: Don't print out-of-bounds event data
  brcmfmac: Add check for short event packets

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 13 +++--
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c  |  3 +--
 2 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.14.1.581.gf28d330327-goog