2020-06-01 07:21:40

by Wright Feng

Subject: [PATCH 1/5] brcmfmac: To fix kernel crash on out of boundary access

From: Raveendran Somu <[email protected]>

To trunkcate the addtional bytes, if extra bytes been received.
Current code only have a warning and proceed without handling it.
But in one of the crash reported by DVT, these causes the
crash intermittently. So the processing is limit to the skb->len.

Signed-off-by: Raveendran Somu <[email protected]>
Signed-off-by: Chi-hsien Lin <[email protected]>
Signed-off-by: Wright Feng <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index 09701262330d..531fe9be4025 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -1843,6 +1843,9 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb)

WARN_ON(siglen > skb->len);

+ if (siglen > skb->len)
+ siglen = skb->len;
+
if (!siglen)
return;
/* if flow control disabled, skip to packet data and leave */
--
2.25.0

2020-06-02 04:35:53

by Florian Fainelli

Subject: Re: [PATCH 1/5] brcmfmac: To fix kernel crash on out of boundary access

On 6/1/2020 12:19 AM, Wright Feng wrote:
> From: Raveendran Somu <[email protected]>
>
> To trunkcate the addtional bytes, if extra bytes been received.

typo: truncate. Missing "have been received".

> Current code only have a warning and proceed without handling it.
> But in one of the crash reported by DVT, these causes the
> crash intermittently. So the processing is limit to the skb->len.
>
> Signed-off-by: Raveendran Somu <[email protected]>
> Signed-off-by: Chi-hsien Lin <[email protected]>
> Signed-off-by: Wright Feng <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> index 09701262330d..531fe9be4025 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> @@ -1843,6 +1843,9 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb)
>
> WARN_ON(siglen > skb->len);
>
> + if (siglen > skb->len)
> + siglen = skb->len;

Does it make sense to keep the WARN_ON() one live above then?

> +
> if (!siglen)
> return;
> /* if flow control disabled, skip to packet data and leave */
>

--
Florian

2020-06-03 06:41:45

by Wright Feng

Subject: Re: [PATCH 1/5] brcmfmac: To fix kernel crash on out of boundary access

Florian Fainelli 於 6/2/2020 12:34 PM 寫道:
>
>
> On 6/1/2020 12:19 AM, Wright Feng wrote:
>> From: Raveendran Somu <[email protected]>
>>
>> To trunkcate the addtional bytes, if extra bytes been received.
>
> typo: truncate. Missing "have been received".
Thanks for catching this, Raveendran will correct it in V2.
>
>> Current code only have a warning and proceed without handling it.
>> But in one of the crash reported by DVT, these causes the
>> crash intermittently. So the processing is limit to the skb->len.
>>
>> Signed-off-by: Raveendran Somu <[email protected]>
>> Signed-off-by: Chi-hsien Lin <[email protected]>
>> Signed-off-by: Wright Feng <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> index 09701262330d..531fe9be4025 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> @@ -1843,6 +1843,9 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb)
>>
>> WARN_ON(siglen > skb->len);
>>
>> + if (siglen > skb->len)
>> + siglen = skb->len;
>
> Does it make sense to keep the WARN_ON() one live above then?
Regarding the change, I believe it is better to have the WARN_ON as
getting something more than expected is indicating the violation of
protocol.So it may be better to leave the warning, instead of quietly
correctly it, which can help in identifying the malicious operations.

If the warning is not complying with the rules, we can remove it, if not
we suggest to have it.
>> +
>> if (!siglen)
>> return;
>> /* if flow control disabled, skip to packet data and leave */
>>
>