| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20171123132457.ttjnlqmi2j72nkac@mwanda>
Date: Thu, 23 Nov 2017 16:24:57 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: ishraq.i.ashraf@...il.com
Cc: gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
insafonov@...il.com, goudapatilk@...il.com,
linux-kernel@...r.kernel.org, himanshujha199640@...il.com,
Johannes Berg <johannes.berg@...el.com>
Subject: Re: [PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls
On Thu, Nov 23, 2017 at 02:29:06AM +0100, ishraq.i.ashraf@...il.com wrote:
> From: Ishraq Ibne Ashraf <ishraq.i.ashraf@...il.com>
>
> Commit 8bfb36766064 ("wireless: wext: remove ndo_do_ioctl fallback") breaks private WEXT
> IOCTL calls of this driver as these are not invoked through ndo_do_ioctl
> interface anymore. As a result hostapd stops working with this driver. In
> this patch this problem is solved by implementing equivalent private IOCTL
> functions of the existing ones which are accessed via iw_handler_def
> interface.
>
> Signed-off-by: Ishraq Ibne Ashraf <ishraq.i.ashraf@...il.com>
It's great to fix this, but new code should be at normal kernel quality.
> ---
> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1042 ++++++++++++++++++++++++
> 1 file changed, 1042 insertions(+)
>
> diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> index c0664dc..7503751 100644
> --- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> @@ -3061,6 +3061,1046 @@ static iw_handler rtw_handlers[] = {
> NULL, /*---hole---*/
> };
>
> +static int get_private_handler_ieee_param(struct adapter *padapter,
> + union iwreq_data *wrqu,
> + void *param)
Indent these more.
> +{
> + /*
> + * This function is expected to be called in master mode, which allows no
> + * power saving. So we just check hw_init_completed.
> + */
> +
> + if (!padapter->hw_init_completed)
> + return -EPERM;
> +
> + if (!wrqu->data.pointer)
> + return -EINVAL;
You could leave this out and it will return -EFAULT when copy_from_user()
fails. That's probably the right error code.
> +
> + /*
> + * Since we don't allocate memory for param in this function, we assume
> + * the caller of this function will properly allocate and deallocate memory
> + * for param.
> + */
This is an obvious comment. Remove it.
> + if (copy_from_user(param, wrqu->data.pointer, wrqu->data.length))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int rtw_hostapd_sta_flush_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> +
> + DBG_88E("%s\n", __func__);
Remove this line.
> +
> + flush_all_cam_entry(padapter); // Clear CAM.
Comment style.
> +
> + return rtw_sta_flush(padapter);
> +}
> +
> +static int rtw_add_sta_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret = 0;
> + struct sta_info *psta = NULL;
> + struct ieee_param *param = NULL;
Don't initialize these to useless values. It turns off the static
checker for finding uninitialized variables.
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> +
> + param = (struct ieee_param *)rtw_malloc(wrqu->data.length);
rtw_malloc() is buggy. It ignores locking. Let's not use it in new
code.
> +
> + if (!param) {
> + DBG_88E(" rtw_add_sta: ieee_param allocate fail !!!\n");
No need to print this debug message.
> +
> + return -ENOMEM;
> + }
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> +
> + if (ret != 0) {
if (ret). "ret" isn't a number zero which can be used for math like
"ret + 2", it's an error code. The != 0 is a double negative which
hurts readability. != 0 is appropriate for numbers and strcmp()
functions.
> + kfree(param);
Free this at the end of the function.
> + DBG_88E(" rtw_add_sta: ieee_param get fail !!!\n");
These messages are so ugly !!!
> +
> + return ret;
> + }
> +
> + DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, (param->sta_addr));
> +
> + if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)))
> + return -EINVAL;
ret = -EINVAL;
goto err_free_param;
> +
> + if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
> + param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
> + param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
> + return -EINVAL;
ret = -EINVAL;
goto err_free_param;
> +
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (psta) {
Always do failure handling. Never do success handling. So this becomes:
if (!psta)
goto err_free_param;
> + int flags = param->u.add_sta.flags;
> + psta->aid = param->u.add_sta.aid; // aid = 1~2007.
> +
> + memcpy(psta->bssrateset, param->u.add_sta.tx_supp_rates, 16);
> +
> + // Check WMM cap.
Comment style
> + if (WLAN_STA_WME&flags)
> + psta->qos_option = 1;
> + else
> + psta->qos_option = 0;
> +
> + if (pmlmepriv->qospriv.qos_option == 0)
> + psta->qos_option = 0;
> +
> + // Check 802.11n HT cap.
> + if (WLAN_STA_HT&flags) {
> + psta->htpriv.ht_option = true;
> + psta->qos_option = 1;
> + memcpy(&psta->htpriv.ht_cap,
> + ¶m->u.add_sta.ht_cap,
> + sizeof(struct ieee80211_ht_cap));
> + } else {
> + psta->htpriv.ht_option = false;
> + }
> +
> + if (pmlmepriv->htpriv.ht_option == false)
> + psta->htpriv.ht_option = false;
> +
> + update_sta_info_apmode(padapter, psta);
> + } else {
> + ret = -ENOMEM;
> + }
> +
> + if (ret == 0 && (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)))
> + ret = -EFAULT;
We need to free param.
> +
> + return ret;
The end of the function could look like this:
update_sta_info_apmode(padapter, psta);
if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
ret = -EFAULT;
err_free_param:
kfree(param);
return ret;
> +}
> +
> +static int rtw_del_sta_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret = 0;
> + struct sta_info *psta = NULL;
> + struct ieee_param *param = NULL;
Remove initialization.
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + int updated = 0;
> +
> + param = (struct ieee_param *)rtw_malloc(wrqu->data.length);
Use kmalloc(); Basically all the same stuff as the previous function.
regards,
dan carpenter
Powered by blists - more mailing lists