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

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

[Arend van Spriel]

On 06-07-17 10:41, Dan Carpenter wrote:
> The lower level networking code 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.  But 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));

Thanks, Dan

Looks fine to me so ...

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Reported-by: "freenerguo(郭大兴)" <freenerguo@tencent.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index dcde596c9eb9..bdae7b44a5ec 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -4934,6 +4934,11 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>                 cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, true,
>                                         GFP_KERNEL);
>         } else if (ieee80211_is_action(mgmt->frame_control)) {
> +               if (len > sizeof(action_frame->data) + DOT11_MGMT_HDR_LEN) {
> +                       err = -EINVAL;
> +                       goto exit;
> +               }
> +
>                 af_params = kzalloc(sizeof(*af_params), GFP_KERNEL);
>                 if (af_params == NULL) {
>                         brcmf_err("unable to allocate frame\n");
> 
> 
> You're receiving this message because you're a member of the brcm80211-dev-list group.
> 
> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
> 

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

[Linus Torvalds]

On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> Looks fine to me so ...

I really think that if we can't trust 'len', then we have to check
against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
we'll just have a big 16-bit number instead.

And we should do that brcmf_err() that I had in my version, which also
let's people know they are being attacked.

                    Linus

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

[Arend van Spriel]

On 7/7/2017 12:32 AM, Linus Torvalds wrote:
> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> Looks fine to me so ...
> 
> I really think that if we can't trust 'len', then we have to check
> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
> we'll just have a big 16-bit number instead.

Fair enough. The firmware on the device should have a check in place, 
but guess what... :-( Anyway, the lower bound depends on the type of 
management frames. So for action frames it is DOT11_MGMT_HDR_LEN + 1 /* 
Action Category */ + 1 /* Action */.

Might be better to place the lower bound check in net/wireless/nl80211.c 
and do that appropriate for the type of management frame. That way it is 
assured for all wireless drivers.

> And we should do that brcmf_err() that I had in my version, which also
> let's people know they are being attacked.

Ok.

Regards,
Arend

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

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 7/7/2017 12:32 AM, Linus Torvalds wrote:
>> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>>
>>> Looks fine to me so ...
>>
>> I really think that if we can't trust 'len', then we have to check
>> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
>> we'll just have a big 16-bit number instead.
>
> Fair enough. The firmware on the device should have a check in place,
> but guess what... :-( Anyway, the lower bound depends on the type of
> management frames. So for action frames it is DOT11_MGMT_HDR_LEN + 1
> /* Action Category */ + 1 /* Action */.
>
> Might be better to place the lower bound check in
> net/wireless/nl80211.c and do that appropriate for the type of
> management frame. That way it is assured for all wireless drivers.

So I drop this patch and wait for v2?

-- 
Kalle Valo

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

[Kalle Valo]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> Looks fine to me so ...
>
> I really think that if we can't trust 'len', then we have to check
> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
> we'll just have a big 16-bit number instead.
>
> And we should do that brcmf_err() that I had in my version, which also
> let's people know they are being attacked.

I hope brcmf_err() is ratelimited so that the attacker cannot spam the
logs too much. BTW I didn't see your version of the patch, I guess it
was not CCed to linux-wireless.

Just a side note, but this discussion is not stored in patchwork, I only
see the original patch. No idea why:

https://patchwork.kernel.org/patch/9827721/

-- 
Kalle Valo

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

[Dan Carpenter]

On Fri, Jul 07, 2017 at 11:40:26AM +0300, Kalle Valo wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> > <arend.vanspriel@broadcom.com> wrote:
> >>
> >> Looks fine to me so ...
> >
> > I really think that if we can't trust 'len', then we have to check
> > against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
> > we'll just have a big 16-bit number instead.
> >
> > And we should do that brcmf_err() that I had in my version, which also
> > let's people know they are being attacked.
> 
> I hope brcmf_err() is ratelimited so that the attacker cannot spam the
> logs too much.

The attacker already has CAP_NET_ADMIN here so you're probably already
toasted.

regards,
dan carpenter

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

[Arend van Spriel]

On 7/7/2017 10:49 AM, Dan Carpenter wrote:
> On Fri, Jul 07, 2017 at 11:40:26AM +0300, Kalle Valo wrote:
>> Linus Torvalds <torvalds@linux-foundation.org> writes:
>>
>>> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
>>> <arend.vanspriel@broadcom.com> wrote:
>>>>
>>>> Looks fine to me so ...
>>>
>>> I really think that if we can't trust 'len', then we have to check
>>> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
>>> we'll just have a big 16-bit number instead.
>>>
>>> And we should do that brcmf_err() that I had in my version, which also
>>> let's people know they are being attacked.
>>
>> I hope brcmf_err() is ratelimited so that the attacker cannot spam the
>> logs too much.
> 
> The attacker already has CAP_NET_ADMIN here so you're probably already
> toasted.

Indeed and brcmf_err() is ratelimited when build without CONFIG_BRCMDBG, 
which is what distros typically do.

Regards,
Arend

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

[Dan Carpenter]

On Thu, Jul 06, 2017 at 03:32:42PM -0700, Linus Torvalds wrote:
> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
> >
> > Looks fine to me so ...
> 
> I really think that if we can't trust 'len', then we have to check
> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
> we'll just have a big 16-bit number instead.

There is already a check in cfg80211_mlme_mgmt_tx().

	if (params->len < 24 + 1)
		return -EINVAL;

It probably should be using DOT11_MGMT_HDR_LEN instead of a magic 24.

regards,
dan carpenter

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

[Arend van Spriel]

On 7/7/2017 10:46 AM, Dan Carpenter wrote:
> On Thu, Jul 06, 2017 at 03:32:42PM -0700, Linus Torvalds wrote:
>> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>>
>>> Looks fine to me so ...
>>
>> I really think that if we can't trust 'len', then we have to check
>> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
>> we'll just have a big 16-bit number instead.
> 
> There is already a check in cfg80211_mlme_mgmt_tx().
> 
> 	if (params->len < 24 + 1)
> 		return -EINVAL;
> 
> It probably should be using DOT11_MGMT_HDR_LEN instead of a magic 24.

Missed that check when I looked yesterday evening. Must have been the 
time ;-)

Regards,
Arend

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

[Dan Carpenter]

Speaking of underflows:

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
  4913          if (ieee80211_is_probe_resp(mgmt->frame_control)) {
  4914                  /* Right now the only reason to get a probe response */
  4915                  /* is for p2p listen response or for p2p GO from     */
  4916                  /* wpa_supplicant. Unfortunately the probe is send   */
  4917                  /* on primary ndev, while dongle wants it on the p2p */
  4918                  /* vif. Since this is only reason for a probe        */
  4919                  /* response to be sent, the vif is taken from cfg.   */
  4920                  /* If ever desired to send proberesp for non p2p     */
  4921                  /* response then data should be checked for          */
  4922                  /* "DIRECT-". Note in future supplicant will take    */
  4923                  /* dedicated p2p wdev to do this and then this 'hack'*/
  4924                  /* is not needed anymore.                            */
  4925                  ie_offset =  DOT11_MGMT_HDR_LEN +
  4926                               DOT11_BCN_PRB_FIXED_LEN;
  4927                  ie_len = len - ie_offset;
                                 ^^^^^^^^^^^^^^^
This can underflow.  It's harmless, but it's annoying for me as a static
checker person because this is the line where I'd like to print a
warning but everyone will complain it's a "false positive".

  4928                  if (vif == cfg->p2p.bss_idx[P2PAPI_BSSCFG_PRIMARY].vif)
  4929                          vif = cfg->p2p.bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
  4930                  err = brcmf_vif_set_mgmt_ie(vif,
  4931                                              BRCMF_VNDR_IE_PRBRSP_FLAG,
  4932                                              &buf[ie_offset],
  4933                                              ie_len);
  4934                  cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, true,
  4935                                          GFP_KERNEL);

regards,
dan carpenter

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

[Arend van Spriel]

On 07-07-17 12:19, Dan Carpenter wrote:
> Speaking of underflows:
> 
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>   4913          if (ieee80211_is_probe_resp(mgmt->frame_control)) {
>   4914                  /* Right now the only reason to get a probe response */
>   4915                  /* is for p2p listen response or for p2p GO from     */
>   4916                  /* wpa_supplicant. Unfortunately the probe is send   */
>   4917                  /* on primary ndev, while dongle wants it on the p2p */
>   4918                  /* vif. Since this is only reason for a probe        */
>   4919                  /* response to be sent, the vif is taken from cfg.   */
>   4920                  /* If ever desired to send proberesp for non p2p     */
>   4921                  /* response then data should be checked for          */
>   4922                  /* "DIRECT-". Note in future supplicant will take    */
>   4923                  /* dedicated p2p wdev to do this and then this 'hack'*/
>   4924                  /* is not needed anymore.                            */
>   4925                  ie_offset =  DOT11_MGMT_HDR_LEN +
>   4926                               DOT11_BCN_PRB_FIXED_LEN;
>   4927                  ie_len = len - ie_offset;
>                                  ^^^^^^^^^^^^^^^
> This can underflow.  It's harmless, but it's annoying for me as a static
> checker person because this is the line where I'd like to print a
> warning but everyone will complain it's a "false positive".

Feel free to provide such a patch.

Regards,
Arend

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

[Arend van Spriel]

On 07-07-17 11:24, Arend van Spriel wrote:
> 
> 
> On 7/7/2017 10:46 AM, Dan Carpenter wrote:
>> On Thu, Jul 06, 2017 at 03:32:42PM -0700, Linus Torvalds wrote:
>>> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
>>> <arend.vanspriel@broadcom.com> wrote:
>>>>
>>>> Looks fine to me so ...
>>>
>>> I really think that if we can't trust 'len', then we have to check
>>> against the lower bound of DOT11_MGMT_HDR_LEN too, because otherwise
>>> we'll just have a big 16-bit number instead.
>>
>> There is already a check in cfg80211_mlme_mgmt_tx().
>>
>>     if (params->len < 24 + 1)
>>         return -EINVAL;
>>
>> It probably should be using DOT11_MGMT_HDR_LEN instead of a magic 24.
> 
> Missed that check when I looked yesterday evening. Must have been the
> time ;-)

Hi Dan,

This being said, are you going to send a V2 adding a brcmf_err() call as
Linus proposed? I think we can improve the length check above later if
deemed necessary.

Regards,
Arend

[PATCH] brcmfmac: buffer overflow in brcmf_cfg80211_mgmt_tx()

[Dan Carpenter]

The lower level networking code 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.  But 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: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index dcde596c9eb9..bdae7b44a5ec 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4934,6 +4934,11 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 		cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, true,
 					GFP_KERNEL);
 	} else if (ieee80211_is_action(mgmt->frame_control)) {
+		if (len > sizeof(action_frame->data) + DOT11_MGMT_HDR_LEN) {
+			err = -EINVAL;
+			goto exit;
+		}
+
 		af_params = kzalloc(sizeof(*af_params), GFP_KERNEL);
 		if (af_params == NULL) {
 			brcmf_err("unable to allocate frame\n");