Dan Carpenter <dan.carpenter@xxxxxxxxxx> writes: > Hello Larry, Jes, > > The rtw_report_sec_ie23a() is very buggy. > > 1) It uses GFP_KERNEL but the callers are holding a spinlock. > > rtw_select_and_join_from_scanned_queue23a() <- takes lock > -> rtw_joinbss_cmd23a() > -> rtw_restruct_sec_ie23a() > -> rtw_report_sec_ie23a() > > 2) The sprintf() can overflow because we're putting over 512 characters > into a IW_CUSTOM_MAX (256) character buffer. > > 3) It could actually be far worse than 512. It could be a forever > loop! :P The "i" variable is declared as u8 so it will always be > less than IW_CUSTOM_MAX (256). > > 4) What is the point of this function? It doesn't seem to store "buff" > anywhere or do anything with "wrqu". > > I have included my suggestion for how to start fixing the function but > it's not a complete solution because I don't know the answer to #4. > > regards, > dan carpenter Dan, Some of this code is a total nightmare :( The problem is we don't even know the intensions of the original author, and at times weird assumptions were made, so we have to analyse our way through it all to figure out what to do with it. I'll add this one to my list for further investigation. Note I already made modifications to it today, replacing _WPA_IE_ID_ with WLAN_EID_VENDOR_SPECIFIC. I am keeping a git tree with my current changes here: https://git.kernel.org/cgit/linux/kernel/git/jes/linux.git/log/?h=rtl8723au-next albeit these commits haven't been pushed yet. Cheers, Jes > > diff --git a/drivers/staging/rtl8723au/os_dep/mlme_linux.c b/drivers/staging/rtl8723au/os_dep/mlme_linux.c > index b30d4d3..6927227 100644 > --- a/drivers/staging/rtl8723au/os_dep/mlme_linux.c > +++ b/drivers/staging/rtl8723au/os_dep/mlme_linux.c > @@ -102,42 +102,30 @@ void rtw_os_indicate_disconnect23a(struct rtw_adapter *adapter) > > void rtw_report_sec_ie23a(struct rtw_adapter *adapter, u8 authmode, u8 *sec_ie) > { > - uint len; > - u8 *buff, *p, i; > + char buf[IW_CUSTOM_MAX]; > + int len; > + int i; > + int printed = 0; > union iwreq_data wrqu; > > RT_TRACE(_module_mlme_osdep_c_, _drv_info_, > ("+rtw_report_sec_ie23a, authmode =%d\n", authmode)); > > - buff = NULL; > - if (authmode == _WPA_IE_ID_) { > - RT_TRACE(_module_mlme_osdep_c_, _drv_info_, > - ("rtw_report_sec_ie23a, authmode =%d\n", authmode)); > - > - buff = kzalloc(IW_CUSTOM_MAX, GFP_KERNEL); > - if (!buff) > - return; > - p = buff; > - > - p += sprintf(p, "ASSOCINFO(ReqIEs ="); > - > - len = sec_ie[1]+2; > - len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX; > - > - for (i = 0; i < len; i++) > - p += sprintf(p, "%02x", sec_ie[i]); > - > - p += sprintf(p, ")"); > - > - memset(&wrqu, 0, sizeof(wrqu)); > - > - wrqu.data.length = p-buff; > - > - wrqu.data.length = (wrqu.data.length < IW_CUSTOM_MAX) ? > - wrqu.data.length : IW_CUSTOM_MAX; > + if (authmode != _WPA_IE_ID_) > + return; > > - kfree(buff); > + printed += sprintf(p, "ASSOCINFO(ReqIEs ="); > + len = min(sec_ie[1] + 2, IW_CUSTOM_MAX); > + for (i = 0; i < len; i++) { > + printed += scnprintf(buf + printed, sizeof(buf) - printed, > + "%02x", sec_ie[i]); > } > + printed += scnprintf(buf + printed, sizeof(buf) - printed, ")"); > + > + memset(&wrqu, 0, sizeof(wrqu)); > + /* FIXME: store the buf somewhere */ > + wrqu.data.length = printed; > + /* FIXME: do something with wrqu */ > } > > #ifdef CONFIG_8723AU_AP_MODE _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
