[PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Arend van Spriel]

The lower level nl80211 code in cfg80211 ensures that "len" is between
25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN (24) from
"len" so thats's max of 2280.  However, the action_frame->data[] buffer is
only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy() can
overflow.

	memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
	       le16_to_cpu(action_frame->len));

Reported-by: "freenerguo(郭大兴)" <freenerguo@tencent.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
Hi Kalle,

Here is the patch as Linus send it to us and security@kernel.org. I
removed the lower bound check as that is already done in cfg80211.
Now I signed off on the patch although formally I suppose Linus should
sign it off. Putting it out there so people can respond as deemed
necessary.

Now fingers crossed whether patchwork will properly deal with the UTF-8
characters :-p

Regards,
Arend
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index cd1d673..d182a00 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4851,6 +4851,11 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev)
 		cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, true,
 					GFP_KERNEL);
 	} else if (ieee80211_is_action(mgmt->frame_control)) {
+		if (len > BRCMF_FIL_ACTION_FRAME_SIZE + DOT11_MGMT_HDR_LEN) {
+			brcmf_err("invalid action frame length\n");
+			err = -EINVAL;
+			goto exit;
+		}
 		af_params = kzalloc(sizeof(*af_params), GFP_KERNEL);
 		if (af_params == NULL) {
 			brcmf_err("unable to allocate frame\n");
-- 
1.9.1

Re: [PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Sedat Dilek]

On Fri, Jul 7, 2017 at 2:01 PM, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
> The lower level nl80211 code in cfg80211 ensures that "len" is between
> 25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN (24) fr=
om
> "len" so thats's max of 2280.  However, the action_frame->data[] buffer i=
s
> only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy() can
> overflow.
>
>         memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
>                le16_to_cpu(action_frame->len));
>
> Reported-by: "freenerguo(=E9=83=AD=E5=A4=A7=E5=85=B4)" <freenerguo@tencen=
t.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
> Hi Kalle,
>
> Here is the patch as Linus send it to us and security@kernel.org. I
> removed the lower bound check as that is already done in cfg80211.
> Now I signed off on the patch although formally I suppose Linus should
> sign it off. Putting it out there so people can respond as deemed
> necessary.
>
> Now fingers crossed whether patchwork will properly deal with the UTF-8
> characters :-p
>

Somehow horrific to see - less in usage (no CC here).

- Sedat -

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/=
?qt=3Dgrep&q=3Dsecurity%40kernel.org

Re: [PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Arend van Spriel]

On 07-07-17 14:47, Sedat Dilek wrote:
> On Fri, Jul 7, 2017 at 2:01 PM, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> The lower level nl80211 code in cfg80211 ensures that "len" is between
>> 25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN (24) from
>> "len" so thats's max of 2280.  However, the action_frame->data[] buffer is
>> only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy() can
>> overflow.
>>
>>         memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
>>                le16_to_cpu(action_frame->len));
>>
>> Reported-by: "freenerguo(郭大兴)" <freenerguo@tencent.com>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> ---
>> Hi Kalle,
>>
>> Here is the patch as Linus send it to us and security@kernel.org. I
>> removed the lower bound check as that is already done in cfg80211.
>> Now I signed off on the patch although formally I suppose Linus should
>> sign it off. Putting it out there so people can respond as deemed
>> necessary.
>>
>> Now fingers crossed whether patchwork will properly deal with the UTF-8
>> characters :-p
>>
> 
> Somehow horrific to see - less in usage (no CC here).

Sorry, Sedat

What is horrific? It is a bit cryptic (for me) what you would like me to
do now if anything.

Regards,
Arend

> - Sedat -
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=security%40kernel.org
> 

Re: [PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Sedat Dilek]

On Fri, Jul 7, 2017 at 9:39 PM, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 07-07-17 14:47, Sedat Dilek wrote:
>> On Fri, Jul 7, 2017 at 2:01 PM, Arend van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> The lower level nl80211 code in cfg80211 ensures that "len" is between
>>> 25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN (24) =
from
>>> "len" so thats's max of 2280.  However, the action_frame->data[] buffer=
 is
>>> only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy() can
>>> overflow.
>>>
>>>         memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
>>>                le16_to_cpu(action_frame->len));
>>>
>>> Reported-by: "freenerguo(=E9=83=AD=E5=A4=A7=E5=85=B4)" <freenerguo@tenc=
ent.com>
>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>> ---
>>> Hi Kalle,
>>>
>>> Here is the patch as Linus send it to us and security@kernel.org. I
>>> removed the lower bound check as that is already done in cfg80211.
>>> Now I signed off on the patch although formally I suppose Linus should
>>> sign it off. Putting it out there so people can respond as deemed
>>> necessary.
>>>
>>> Now fingers crossed whether patchwork will properly deal with the UTF-8
>>> characters :-p
>>>
>>
>> Somehow horrific to see - less in usage (no CC here).
>
> Sorry, Sedat
>
> What is horrific? It is a bit cryptic (for me) what you would like me to
> do now if anything.
>

You did a CC <security@kernel.org>, thanks.
Looking at the sources, docs and (commit) logs, this email-address
seems "unknown" and less in usage.

- Sedat -

> Regards,
> Arend
>
>> - Sedat -
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/l=
og/?qt=3Dgrep&q=3Dsecurity%40kernel.org
>>

Re: [PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Johannes Berg]

On Fri, 2017-07-07 at 13:01 +0100, Arend van Spriel wrote:
> The lower level nl80211 code in cfg80211 ensures that "len" is
> between
> 25 and NL80211_ATTR_FRAME (2304).  We subtract DOT11_MGMT_HDR_LEN
> (24) from
> "len" so thats's max of 2280.  However, the action_frame->data[]
> buffer is
> only BRCMF_FIL_ACTION_FRAME_SIZE (1800) bytes long so this memcpy()
> can
> overflow.
> 
> 	memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
> 	       le16_to_cpu(action_frame->len));

Kalle is on vacation for the next 10 days or so.

Linus, since you were involved already, will you apply this directly?

Arend, otherwise please resend including netdev@, so we can ask davem
to pick it up (needs to land in his patchwork).

I guess it should also have a Cc: stable tag, and perhaps a Fixes?

Thanks,
johannes

Re: [PATCH] brcmfmac: fix possible buffer overflow in brcmf_cfg80211_mgmt_tx()

[Linus Torvalds]

On Fri, Jul 7, 2017 at 6:17 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Linus, since you were involved already, will you apply this directly?

I don't think it's _that_ urgent, since it's specific to one
particular driver anyway. I'd suggest just going through the normal
channels, and be cc'd to netdev.

> I guess it should also have a Cc: stable tag, and perhaps a Fixes?

The fixes tag would be

    Fixes: 18e2f61db3b70 ("brcmfmac: P2P action frame tx.")

which is 3.9 in case anybody cares. I assume that didn't get
backported any further.

                Linus