2013-02-07 20:14:17

by Tim Gardner

Subject: [PATCH wireless-next] brcmsmac: avoid 512 byte stack variable

Dynamically allocate the probe response template which
avoids potential stack corruption. Observed with smatch:

drivers/net/wireless/brcm80211/brcmsmac/main.c:7412 brcms_c_bss_update_probe_resp()
warn: 'prb_resp' puts 512 bytes on stack

Cc: Brett Rudley <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: "Franky (Zhenhui) Lin" <[email protected]>
Cc: Hante Meuleman <[email protected]>
Cc: "John W. Linville" <[email protected]>
Cc: Seth Forshee <[email protected]>
Cc: Pieter-Paul Giesberts <[email protected]>
Cc: Hauke Mehrtens <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Tim Gardner <[email protected]>
---
drivers/net/wireless/brcm80211/brcmsmac/main.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index c26992a..e392e76 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -7408,9 +7408,16 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,
struct brcms_bss_cfg *cfg,
bool suspend)
{
- u16 prb_resp[BCN_TMPL_LEN / 2];
+ u16 *prb_resp;
int len = BCN_TMPL_LEN;

+ prb_resp = kmalloc(BCN_TMPL_LEN, GFP_ATOMIC);
+ if (!prb_resp) {
+ wiphy_err(wlc->wiphy, "wl: %s: failed to alloc %u bytes\n",
+ __func__, BCN_TMPL_LEN);
+ return;
+ }
+
/*
* write the probe response to hardware, or save in
* the config structure
@@ -7444,6 +7451,8 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,

if (suspend)
brcms_c_enable_mac(wlc);
+
+ kfree(prb_resp);
}

void brcms_c_update_probe_resp(struct brcms_c_info *wlc, bool suspend)
--
1.7.9.5

2013-02-07 20:24:08

by Arend van Spriel

Subject: Re: [PATCH wireless-next] brcmsmac: avoid 512 byte stack variable

On 02/07/2013 09:13 PM, Tim Gardner wrote:
> Dynamically allocate the probe response template which
> avoids potential stack corruption. Observed with smatch:
>
> drivers/net/wireless/brcm80211/brcmsmac/main.c:7412 brcms_c_bss_update_probe_resp()
> warn: 'prb_resp' puts 512 bytes on stack
>
> Cc: Brett Rudley <[email protected]>
> Cc: Arend van Spriel <[email protected]>
> Cc: "Franky (Zhenhui) Lin" <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: "John W. Linville" <[email protected]>
> Cc: Seth Forshee <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Hauke Mehrtens <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]

One comment below. When taken care of feel free to add:

Acked-by: Arend van Spriel <[email protected]>
> Signed-off-by: Tim Gardner <[email protected]>
> ---
> drivers/net/wireless/brcm80211/brcmsmac/main.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> index c26992a..e392e76 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> @@ -7408,9 +7408,16 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,
> struct brcms_bss_cfg *cfg,
> bool suspend)
> {
> - u16 prb_resp[BCN_TMPL_LEN / 2];
> + u16 *prb_resp;
> int len = BCN_TMPL_LEN;
>
> + prb_resp = kmalloc(BCN_TMPL_LEN, GFP_ATOMIC);
> + if (!prb_resp) {
> + wiphy_err(wlc->wiphy, "wl: %s: failed to alloc %u bytes\n",
> + __func__, BCN_TMPL_LEN);

I believe the kmalloc() call spews enough info upon allocation failure
so please remove the error message here.

> + return;
> + }
> +
> /*
> * write the probe response to hardware, or save in
> * the config structure
> @@ -7444,6 +7451,8 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,
>
> if (suspend)
> brcms_c_enable_mac(wlc);
> +
> + kfree(prb_resp);
> }
>
> void brcms_c_update_probe_resp(struct brcms_c_info *wlc, bool suspend)
>

2013-02-07 20:28:28

by Tim Gardner

Subject: [PATCH wireless-next V2] brcmsmac: avoid 512 byte stack variable

Dynamically allocate the probe response template which
avoids potential stack corruption. Observed with smatch:

drivers/net/wireless/brcm80211/brcmsmac/main.c:7412 brcms_c_bss_update_probe_resp()
warn: 'prb_resp' puts 512 bytes on stack

Cc: Brett Rudley <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: "Franky (Zhenhui) Lin" <[email protected]>
Cc: Hante Meuleman <[email protected]>
Cc: "John W. Linville" <[email protected]>
Cc: Seth Forshee <[email protected]>
Cc: Pieter-Paul Giesberts <[email protected]>
Cc: Hauke Mehrtens <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Tim Gardner <[email protected]>
---
drivers/net/wireless/brcm80211/brcmsmac/main.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index c26992a..ea88abe 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -7408,9 +7408,13 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,
struct brcms_bss_cfg *cfg,
bool suspend)
{
- u16 prb_resp[BCN_TMPL_LEN / 2];
+ u16 *prb_resp;
int len = BCN_TMPL_LEN;

+ prb_resp = kmalloc(BCN_TMPL_LEN, GFP_ATOMIC);
+ if (!prb_resp)
+ return;
+
/*
* write the probe response to hardware, or save in
* the config structure
@@ -7444,6 +7448,8 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,

if (suspend)
brcms_c_enable_mac(wlc);
+
+ kfree(prb_resp);
}

void brcms_c_update_probe_resp(struct brcms_c_info *wlc, bool suspend)
--
1.7.9.5

2013-02-07 20:19:47

by Joe Perches

Subject: Re: [PATCH wireless-next] brcmsmac: avoid 512 byte stack variable

On Thu, 2013-02-07 at 13:13 -0700, Tim Gardner wrote:
> Dynamically allocate the probe response template which
> avoids potential stack corruption. Observed with smatch:

trivial:

> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
[]
> @@ -7408,9 +7408,16 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,
[]
> + prb_resp = kmalloc(BCN_TMPL_LEN, GFP_ATOMIC);
> + if (!prb_resp) {
> + wiphy_err(wlc->wiphy, "wl: %s: failed to alloc %u bytes\n",
> + __func__, BCN_TMPL_LEN);

Please remove the error message.
alloc's don't need specific OOM messages.

The mm subsystem already provides a standardized
message with a dump_stack().

2013-02-07 20:24:08

by Arend van Spriel

Subject: Re: [PATCH wireless-next] brcmsmac: avoid 512 byte stack variable

On 02/07/2013 09:19 PM, Joe Perches wrote:
> On Thu, 2013-02-07 at 13:13 -0700, Tim Gardner wrote:
>> Dynamically allocate the probe response template which
>> avoids potential stack corruption. Observed with smatch:
>
> trivial:
>
>> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> []
>> @@ -7408,9 +7408,16 @@ brcms_c_bss_update_probe_resp(struct brcms_c_info *wlc,
> []
>> + prb_resp = kmalloc(BCN_TMPL_LEN, GFP_ATOMIC);
>> + if (!prb_resp) {
>> + wiphy_err(wlc->wiphy, "wl: %s: failed to alloc %u bytes\n",
>> + __func__, BCN_TMPL_LEN);
>
> Please remove the error message.
> alloc's don't need specific OOM messages.
>
> The mm subsystem already provides a standardized
> message with a dump_stack().

You beat me to the finish line :-)

Gr. AvS