[PATCH 0/3] New brcmfmac bounds checks

[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

[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 <mnissler@chromium.org>
Signed-off-by: Kevin Cernekee <cernekee@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 = &p2p->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(&vif->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

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

[Arend van Spriel]

On 08-09-17 21:13, Kevin Cernekee wrote:
> 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

The debug message is after the length validation so there is not 
unintialized data being printed.

> 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.

However, there is no real for the chanspec variable for the given reason 
so...

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

[PATCH 2/3] brcmfmac: Don't print out-of-bounds event data

[Kevin Cernekee]

The debug print that dumps out newly-dequeued events uses emsg.datalen
before that field has been validated, which may lead to an out-of-bounds
read.  Assume that any properly-formed event message has a valid length
field, and move the debug print below the length check.

Suggested-by: Mattias Nissler <mnissler@chromium.org>
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 4eb1e1ce9ace..5aabdc9ed7e0 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -252,17 +252,17 @@ static void brcmf_fweh_event_worker(struct work_struct *work)
 		emsg.ifidx = emsg_be->ifidx;
 		emsg.bsscfgidx = emsg_be->bsscfgidx;
 
-		brcmf_dbg(EVENT, "  version %u flags %u status %u reason %u\n",
-			  emsg.version, emsg.flags, emsg.status, emsg.reason);
-		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;
 		}
 
+		brcmf_dbg(EVENT, "  version %u flags %u status %u reason %u\n",
+			  emsg.version, emsg.flags, emsg.status, emsg.reason);
+		brcmf_dbg_hex_dump(BRCMF_EVENT_ON(), event->data,
+				   min_t(u32, emsg.datalen, 64),
+				   "event payload, len=%d\n", emsg.datalen);
 		/* special handling of interface event */
 		if (event->code == BRCMF_E_IF) {
 			brcmf_fweh_handle_if_event(drvr, &emsg, event->data);
-- 
2.14.1.581.gf28d330327-goog

Re: [PATCH 2/3] brcmfmac: Don't print out-of-bounds event data

[Arend van Spriel]

On 08-09-17 21:13, Kevin Cernekee wrote:
> The debug print that dumps out newly-dequeued events uses emsg.datalen
> before that field has been validated, which may lead to an out-of-bounds
> read.  Assume that any properly-formed event message has a valid length
> field, and move the debug print below the length check.

The length check is a bit redundant as event->datalen is assigned to 
emsg.datalen upon queuing the event which also does validation. So I 
would propose to just remove the length check here.

Regards,
Arend

> Suggested-by: Mattias Nissler <mnissler@chromium.org>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

[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 <mnissler@chromium.org>
Signed-off-by: Kevin Cernekee <cernekee@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

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

[Arend van Spriel]

On 08-09-17 21:13, Kevin Cernekee wrote:
> 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.

Franky made an almost identical change which I had queued up for 
submission. So you beat us to it ;-)

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

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

[Mattias Nissler]

On Fri, Sep 8, 2017 at 9:13 PM, Kevin Cernekee <cernekee@chromium.org> wrote:
>
> 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 <mnissler@chromium.org>
> Signed-off-by: Kevin Cernekee <cernekee@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)

Shouldn't this check be larger-than, i.e. we need the packet to be at
least sizeof(*event_packet) + its payload size?

>                 return;
>
>         if (in_interrupt())
> --
> 2.14.1.581.gf28d330327-goog
>

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

[Arend van Spriel]

On 11-09-17 11:19, Mattias Nissler wrote:
> On Fri, Sep 8, 2017 at 9:13 PM, Kevin Cernekee <cernekee@chromium.org> wrote:
>>
>> 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 <mnissler@chromium.org>
>> Signed-off-by: Kevin Cernekee <cernekee@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)
> 
> 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).

Regards,
Arend

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
<arend.vanspriel@broadcom.com> 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.

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

[Arend van Spriel]

On 12-09-17 17:04, Kevin Cernekee wrote:
> On Mon, Sep 11, 2017 at 12:09 PM, Arend van Spriel
> <arend.vanspriel@broadcom.com> 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

Checking for exact match might not work, because the skbuff length could 
differ because of host interface alignment requirements.

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

the check Franky had was:

	datalen > packet_len - sizeof(*event_packet)

> 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.

Agree. I quickly browsed through our collection of tests in our test 
framework, but found none covering this.

Regards,
Arend