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 <[email protected]>
> Reported-by: "freenerguo(郭大兴)" <[email protected]>
> Signed-off-by: Dan Carpenter <[email protected]>
>
> 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.
>
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
>> <[email protected]> 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
On Fri, Jul 07, 2017 at 11:40:26AM +0300, Kalle Valo wrote:
> Linus Torvalds <[email protected]> writes:
>
> > On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> > <[email protected]> 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
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
>>> <[email protected]> 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
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
On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
<[email protected]> 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
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 <[email protected]> writes:
>>
>>> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
>>> <[email protected]> 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
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
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
> <[email protected]> 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
Linus Torvalds <[email protected]> writes:
> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> <[email protected]> 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
On 7/7/2017 12:32 AM, Linus Torvalds wrote:
> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
> <[email protected]> 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
Arend van Spriel <[email protected]> writes:
> On 7/7/2017 12:32 AM, Linus Torvalds wrote:
>> On Thu, Jul 6, 2017 at 10:11 AM, Arend van Spriel
>> <[email protected]> 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