| Message ID | 1499428893-30750-1-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
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). - Sedat - [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=security%40kernel.org
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
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
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 >
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(郭大兴)" <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. > 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/log/?qt=grep&q=security%40kernel.org >>
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");
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(+)