From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Arend van Spriel" Date: Wed, 26 Sep 2012 09:41:36 +0000 Subject: Re: [patch] brcmfmac: use kcalloc() to prevent integer overflow Message-Id: <5062CDD0.2000008@broadcom.com> List-Id: References: <20120926072148.GA3956@elgon.mountain> <20120926073143.GO13767@mwanda> In-Reply-To: <20120926073143.GO13767@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter Cc: Brett Rudley , Roland Vossen , "Franky (Zhenhui) Lin" , Kan Yan , "John W. Linville" , Hante Meuleman , linux-wireless@vger.kernel.org, brcm80211-dev-list@broadcom.com, kernel-janitors@vger.kernel.org On 09/26/2012 09:31 AM, Dan Carpenter wrote: > Speaking of integer overflows, I had a couple other concerns in this > file. > > drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c brcmf_enq_event() > 4144 total_len = sizeof(struct brcmf_cfg80211_event_q); > 4145 if (data) > 4146 data_len = be32_to_cpu(msg->datalen); > 4147 else > 4148 data_len = 0; > 4149 total_len += data_len; > ^^^^^^^^^^^^^^^^^^^^^ > This looks very suspicious like a remote exploitable overflow. Hi Dan, The event message is generated in our device so I believe there is little room for exploits. > 4150 e = kzalloc(total_len, GFP_ATOMIC); > > drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c brcmf_run_escan() > 882 if (request != NULL) { > 883 /* Allocate space for populating ssids in struct */ > 884 params_size += sizeof(u32) * ((request->n_channels + 1) / 2); > 885 > 886 /* Allocate space for populating ssids in struct */ > 887 params_size += sizeof(struct brcmf_ssid) * request->n_ssids; > 888 } > 889 > 890 params = kzalloc(params_size, GFP_KERNEL); > > I didn't track back where request comes from so I don't know if > that's a problem or not. I figured you would know better than I > would. This request comes from user-space, ie. nl80211. cfg80211 does make sure that amount channels and ssids are sane (see nl80211_trigger_scan() in net/wireless/nl80211.c). Gr. AvS