| Message ID | 1360268032-52414-1-git-send-email-tim.gardner@canonical.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <brudley@broadcom.com> > Cc: Arend van Spriel <arend@broadcom.com> > Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com> > Cc: Hante Meuleman <meuleman@broadcom.com> > Cc: "John W. Linville" <linville@tuxdriver.com> > Cc: Seth Forshee <seth.forshee@canonical.com> > Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com> > Cc: Hauke Mehrtens <hauke@hauke-m.de> > Cc: linux-wireless@vger.kernel.org > Cc: brcm80211-dev-list@broadcom.com > Cc: netdev@vger.kernel.org One comment below. When taken care of feel free to add: Acked-by: Arend van Spriel <arend@broadcom.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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 <brudley@broadcom.com> Cc: Arend van Spriel <arend@broadcom.com> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com> Cc: Hante Meuleman <meuleman@broadcom.com> Cc: "John W. Linville" <linville@tuxdriver.com> Cc: Seth Forshee <seth.forshee@canonical.com> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com> Cc: Hauke Mehrtens <hauke@hauke-m.de> Cc: linux-wireless@vger.kernel.org Cc: brcm80211-dev-list@broadcom.com Cc: netdev@vger.kernel.org Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- drivers/net/wireless/brcm80211/brcmsmac/main.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)