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

2017-09-12 Thread Arend van Spriel 

On 12-09-17 17:04, Kevin Cernekee wrote:

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


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

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

2017-09-12 Thread 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.

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

2017-09-11 Thread Arend van Spriel 

On 11-09-17 11:19, Mattias Nissler wrote:

On Fri, Sep 8, 2017 at 9:13 PM, 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.

Suggested-by: Mattias Nissler 
Signed-off-by: Kevin Cernekee 
---
  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

2017-09-11 Thread Mattias Nissler 
On Fri, Sep 8, 2017 at 9:13 PM, 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.
>
> Suggested-by: Mattias Nissler 
> Signed-off-by: Kevin Cernekee 
> ---
>  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

2017-09-09 Thread 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 

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

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

2017-09-08 Thread 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 
Signed-off-by: Kevin Cernekee 
---
 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