Re: [PATCH v3] staging: rtl8188eu: Fix private WEXT IOCTL calls

From: Dan Carpenter
Date: Sat Nov 25 2017 - 16:26:25 EST

Next message: Solio Sarabia: "[PATCH RFC] veth: make veth aware of gso buffer size"
Previous message: Willem de Bruijn: "Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets"
In reply to: ishraq . i . ashraf: "[PATCH v3] staging: rtl8188eu: Fix private WEXT IOCTL calls"
Next in thread: Johannes Berg: "Re: [PATCH v3] staging: rtl8188eu: Fix private WEXT IOCTL calls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Thanks. This is looking *much* better...

On Sat, Nov 25, 2017 at 07:29:41PM +0100, ishraq.i.ashraf@xxxxxxxxx wrote:
> From: Ishraq Ibne Ashraf <ishraq.i.ashraf@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1077 ++++++++++++++++++++++++
> 1 file changed, 1077 insertions(+)
>
> diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> index c0664dc..8d99f99 100644
> --- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> @@ -3061,6 +3061,1081 @@ static iw_handler rtw_handlers[] = {
> NULL, /*---hole---*/
> };
>
> +static int get_private_handler_ieee_param(struct adapter *padapter,
> + union iwreq_data *wrqu,
> + void *param)
> +{
> + /*
> + * 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;

This check doesn't belong here. ->hw_init_completed is set to true in
rtw_hal_init() and false in rtw_hal_deinit(). Can it ever be false
here? I really doubt it and if I'm wrong then we are pretty screwed
already... Anyway, move the check or delete it. Then this function and
the kmalloc() can be replaced with memdup_user().

> +
> + 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);

Add a blank line after declarations.

> + flush_all_cam_entry(padapter); /* Clear CAM. */
> + 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;
> + int flags;
> + struct sta_info *psta;
> + struct ieee_param *param;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;

Then these lines become:

param = memdup_user(wrqu->data.pointer, wrqu->data.length);
if (IS_ERR(param))
return PTR_ERR(param);

> +
> + 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))) {
> + 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) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (!psta) {
> + ret = -ENOMEM;
> + goto err_free_param;
> + }
> +
> + 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. */
> + if (WLAN_STA_WME&flags)

Add spaces around the & and reverse the order.

if (flags & WLAN_STA_HT) {

> + 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) {

Add spaces and reverse.

> + psta->htpriv.ht_option = true;
> + psta->qos_option = 1;
> + memcpy(&psta->htpriv.ht_cap,
> + &param->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);
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_param;
> + }
> +
> + kfree(param);
> + return 0;
> +
> +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;
> + int updated;
> + struct sta_info *psta;
> + struct ieee_param *param;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;

memdup_user();

> +
> + DBG_88E("rtw_del_sta =%pM\n", (param->sta_addr));
> +
> + if (check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)) != true) {
> + 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) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (!psta) {
> + DBG_88E("rtw_del_sta(), sta has already been removed or never been added\n");
> + ret = -ENOMEM;
> + goto err_free_param;
> + }
> +
> + spin_lock_bh(&pstapriv->asoc_list_lock);
> +
> + updated = 0;
> +
> + if (!list_empty(&psta->asoc_list)) {
> + list_del_init(&psta->asoc_list);
> + pstapriv->asoc_list_cnt--;
> + updated = ap_free_sta(padapter, psta, true, WLAN_REASON_DEAUTH_LEAVING);
> + }
> +
> + spin_unlock_bh(&pstapriv->asoc_list_lock);
> + associated_clients_update(padapter, updated);
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_param;
> + }
> +
> + kfree(param);
> + return 0;
> +
> +err_free_param:
> + kfree(param);
> + return ret;
> +}
> +
> +static int rtw_set_beacon_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret;
> + int len;
> + unsigned char *pbuf;
> + struct ieee_param *param;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;
> +
> + len = wrqu->data.length;
> + pbuf = param->u.bcn_ie.buf;
> +
> + DBG_88E("%s, len =%d\n", __func__, len);
> +
> + if (check_fwstate(pmlmepriv, WIFI_AP_STATE) != true) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + memcpy(&pstapriv->max_num_sta, param->u.bcn_ie.reserved, 2);

It's concerning to me that we are using reserved memory... Reserved
means we it is saved for the future and we aren't supposed to use it.
Maybe the name needs changed. Use a sizeof() instead of a 2.

> +
> + if ((pstapriv->max_num_sta > NUM_STA) || (pstapriv->max_num_sta <= 0))
> + pstapriv->max_num_sta = NUM_STA;
> +
> + if (rtw_check_beacon_data(padapter, pbuf, (len-12-2)) != _SUCCESS) { /* 12 = Param header, 2 = Not packed. */

The white space isn't right here. There are two spaces after the comma.

> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_param;
> + }
> +
> + kfree(param);
> + return 0;
> +
> +err_free_param:
> + kfree(param);
> + return ret;
> +}
> +
> +static int rtw_set_encryption_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret;
> + int param_len;
> + u32 wep_key_idx;
> + u32 wep_key_len;
> + u32 wep_total_len;
> + struct ieee_param *param;
> + struct sta_info *pbcmc_sta;
> + struct ndis_802_11_wep *pwep;
> + struct sta_info *psta = NULL;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> + struct security_priv *psecuritypriv = &(padapter->securitypriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;
> +
> + param_len = wrqu->data.length;
> + param->u.crypt.err = 0;
> + param->u.crypt.alg[IEEE_CRYPT_ALG_NAME_LEN - 1] = '\0';

This is problematica because how do we know that wrqu->data.length is
large enough to hold a ieee_param struct? There should probably be a
check like:

if (wrqu->data.length < sizeof(*param))
return -EINVAL;

Probably all the functions should have this.

> +
> + if (param_len != sizeof(struct ieee_param) + param->u.crypt.key_len) {

We're checking here but we have already written a NUL terminator
possibly beyond the end of the buffer. There are two spaces after the
!=.

> + 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) {
> + if (param->u.crypt.idx >= WEP_KEYS) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> + } else {
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (!psta) {
> + DBG_88E("rtw_set_encryption(), sta has already been removed or never been added\n");
> + ret = -ENOMEM;
> + goto err_free_param;
> + }
> + }
> +
> + if (strcmp(param->u.crypt.alg, "none") == 0 && (!psta)) {
^ ^
Remove the extra parens.

> + /* TODO: Clear default encryption keys. */
> + DBG_88E("clear default encryption keys, keyid =%d\n",
> + param->u.crypt.idx);
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + if (strcmp(param->u.crypt.alg, "WEP") == 0 && (!psta)) {
^ ^
Extra parens.

> + DBG_88E("r871x_set_encryption, crypt.alg = WEP\n");
> +
> + pwep = NULL;
> + wep_key_idx = param->u.crypt.idx;
> + wep_key_len = param->u.crypt.key_len;
> +
> + DBG_88E("r871x_set_encryption, wep_key_idx=%d, len=%d\n",
> + wep_key_idx,
> + wep_key_len);
> +
> + if ((wep_key_idx >= WEP_KEYS) || (wep_key_len <= 0)) {
^ ^ ^ ^
Remove the extra parens. wep_key_len is unsigned so it can't be less
than zero.

> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + if (wep_key_len > 0) {

No need to check this because we just checked on the line before.

> + wep_key_len = wep_key_len <= 5 ? 5 : 13;
> + wep_total_len = wep_key_len + offsetof(struct ndis_802_11_wep, KeyMaterial);
> +
> + pwep = (struct ndis_802_11_wep *)kmalloc(wep_total_len, GFP_KERNEL);
> + if (!pwep) {
> + DBG_88E(" r871x_set_encryption: pwep allocate fail !!!\n");

Remove space at the start of printk

> + ret = -ENOMEM;
> + goto err_free_param;
> + }
> +
> + memset(pwep, 0, wep_total_len);
> + pwep->KeyLength = wep_key_len;
> + pwep->Length = wep_total_len;
> + }
> +
> + pwep->KeyIndex = wep_key_idx;
> + memcpy(pwep->KeyMaterial, param->u.crypt.key, pwep->KeyLength);

Extra space after comma

> +
> + if (param->u.crypt.set_tx) {
> + DBG_88E("wep, set_tx = 1\n");
> +
> + psecuritypriv->ndisencryptstatus = Ndis802_11Encryption1Enabled;
> + psecuritypriv->dot11PrivacyAlgrthm = _WEP40_;
> + psecuritypriv->dot118021XGrpPrivacy = _WEP40_;
> +
> + if (pwep->KeyLength == 13) {
> + psecuritypriv->dot11PrivacyAlgrthm = _WEP104_;
> + psecuritypriv->dot118021XGrpPrivacy = _WEP104_;
> + }
> +
> + psecuritypriv->dot11PrivacyKeyIndex = wep_key_idx;
> + memcpy(&(psecuritypriv->dot11DefKey[wep_key_idx].skey[0]),
^ ^
Remove the extra parens

> + pwep->KeyMaterial,
> + pwep->KeyLength);
> + psecuritypriv->dot11DefKeylen[wep_key_idx] = pwep->KeyLength;
> + set_wep_key(padapter, pwep->KeyMaterial, pwep->KeyLength, wep_key_idx);
> + } else {
> + DBG_88E("wep, set_tx = 0\n");
> +
> + /*
> + * Don't update "psecuritypriv->dot11PrivacyAlgrthm" and
> + * "psecuritypriv->dot11PrivacyKeyIndex = keyid", but rtw_set_key()
> + * can to cam.
> + */
> +
> + memcpy(&(psecuritypriv->dot11DefKey[wep_key_idx].skey[0]),
^ ^
Extra parens

> + pwep->KeyMaterial,
> + pwep->KeyLength);
> + psecuritypriv->dot11DefKeylen[wep_key_idx] = pwep->KeyLength;
> + set_wep_key(padapter, pwep->KeyMaterial, pwep->KeyLength, wep_key_idx);
> + }
> +
> + kfree(pwep);
> + goto free_param;
> + }
> +
> + if (!psta && check_fwstate(pmlmepriv, WIFI_AP_STATE)) { /* Group key. */
> + if (param->u.crypt.set_tx == 1) {
> + if (strcmp(param->u.crypt.alg, "WEP") == 0) {
> + DBG_88E("%s, set group_key, WEP\n", __func__);
> +
> + memcpy(psecuritypriv->dot118021XGrpKey[param->u.crypt.idx].skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len,
> + 16));
> + psecuritypriv->dot118021XGrpPrivacy = _WEP40_;
> +
> + if (param->u.crypt.key_len == 13)
> + psecuritypriv->dot118021XGrpPrivacy = _WEP104_;
> + } else if (strcmp(param->u.crypt.alg, "TKIP") == 0) {
> + DBG_88E("%s, set group_key, TKIP\n", __func__);
> +
> + psecuritypriv->dot118021XGrpPrivacy = _TKIP_;
> + memcpy(psecuritypriv->dot118021XGrpKey[param->u.crypt.idx].skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len,
> + 16));
> + /* Set mic key. */
> + memcpy(psecuritypriv->dot118021XGrptxmickey[param->u.crypt.idx].skey,
> + &(param->u.crypt.key[16]),
> + 8);
> + memcpy(psecuritypriv->dot118021XGrprxmickey[param->u.crypt.idx].skey,
> + &(param->u.crypt.key[24]),
> + 8);
> + psecuritypriv->busetkipkey = true;
> + } else if (strcmp(param->u.crypt.alg, "CCMP") == 0) {
> + DBG_88E("%s, set group_key, CCMP\n", __func__);
> +
> + psecuritypriv->dot118021XGrpPrivacy = _AES_;
> + memcpy(psecuritypriv->dot118021XGrpKey[param->u.crypt.idx].skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len, 16));
> + } else {
> + DBG_88E("%s, set group_key, none\n", __func__);
> +
> + psecuritypriv->dot118021XGrpPrivacy = _NO_PRIVACY_;
> + }
> +
> + psecuritypriv->dot118021XGrpKeyid = param->u.crypt.idx;
> + psecuritypriv->binstallGrpkey = true;
> + psecuritypriv->dot11PrivacyAlgrthm = psecuritypriv->dot118021XGrpPrivacy;
> + set_group_key(padapter, param->u.crypt.key, psecuritypriv->dot118021XGrpPrivacy, param->u.crypt.idx);
> +
> + pbcmc_sta = rtw_get_bcmc_stainfo(padapter);
> + if (pbcmc_sta) {
> + pbcmc_sta->ieee8021x_blocked = false;
> + pbcmc_sta->dot118021XPrivacy = psecuritypriv->dot118021XGrpPrivacy; /* rx will use bmc_sta's dot118021XPrivacy. */
> + }
> + }
> +
> + goto free_param;
> + }
> +
> + if (psecuritypriv->dot11AuthAlgrthm == dot11AuthAlgrthm_8021X && psta) { /* psk/802_1x. */
> + if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
> + if (param->u.crypt.set_tx == 1) {
> + memcpy(psta->dot118021x_UncstKey.skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len, 16));
> +
> + if (strcmp(param->u.crypt.alg, "WEP") == 0) {
> + DBG_88E("%s, set pairwise key, WEP\n", __func__);
> +
> + psta->dot118021XPrivacy = _WEP40_;
> +
> + if (param->u.crypt.key_len == 13)
> + psta->dot118021XPrivacy = _WEP104_;
> + } else if (strcmp(param->u.crypt.alg, "TKIP") == 0) {
> + DBG_88E("%s, set pairwise key, TKIP\n", __func__);
> +
> + psta->dot118021XPrivacy = _TKIP_;
> + /* Set mic key. */
> + memcpy(psta->dot11tkiptxmickey.skey, &(param->u.crypt.key[16]), 8);
> + memcpy(psta->dot11tkiprxmickey.skey, &(param->u.crypt.key[24]), 8);
> + psecuritypriv->busetkipkey = true;
> + } else if (strcmp(param->u.crypt.alg, "CCMP") == 0) {
> + DBG_88E("%s, set pairwise key, CCMP\n", __func__);
> +
> + psta->dot118021XPrivacy = _AES_;
> + } else {
> + DBG_88E("%s, set pairwise key, none\n", __func__);
> +
> + psta->dot118021XPrivacy = _NO_PRIVACY_;
> + }
> +
> + set_pairwise_key(padapter, psta);
> +
> + psta->ieee8021x_blocked = false;
> + } else { /* Group key ? */
> + if (strcmp(param->u.crypt.alg, "WEP") == 0) {
> + memcpy(psecuritypriv->dot118021XGrpKey[param->u.crypt.idx].skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len, 16));
> + psecuritypriv->dot118021XGrpPrivacy = _WEP40_;
> +
> + if (param->u.crypt.key_len == 13)
> + psecuritypriv->dot118021XGrpPrivacy = _WEP104_;
> + } else if (strcmp(param->u.crypt.alg, "TKIP") == 0) {
> + psecuritypriv->dot118021XGrpPrivacy = _TKIP_;
> + memcpy(psecuritypriv->dot118021XGrpKey[param->u.crypt.idx].skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len, 16));
> + /* Set mic key. */
> + memcpy(psecuritypriv->dot118021XGrptxmickey[param->u.crypt.idx].skey,
> + &(param->u.crypt.key[16]), 8);
^ ^
Extra parens

> + memcpy(psecuritypriv->dot118021XGrprxmickey[param->u.crypt.idx].skey,
> + &(param->u.crypt.key[24]), 8);
^ ^
Extra parens

> + psecuritypriv->busetkipkey = true;
> + } else if (strcmp(param->u.crypt.alg, "CCMP") == 0) {
> + psecuritypriv->dot118021XGrpPrivacy = _AES_;
> + memcpy(psecuritypriv->dot118021XGrpKey[param->u.crypt.idx].skey,
> + param->u.crypt.key,
> + min_t(u16, param->u.crypt.key_len, 16));
> + } else {
> + psecuritypriv->dot118021XGrpPrivacy = _NO_PRIVACY_;
> + }
> +
> + psecuritypriv->dot118021XGrpKeyid = param->u.crypt.idx;
> + psecuritypriv->binstallGrpkey = true;
> + psecuritypriv->dot11PrivacyAlgrthm = psecuritypriv->dot118021XGrpPrivacy;
> + set_group_key(padapter,
> + param->u.crypt.key,
> + psecuritypriv->dot118021XGrpPrivacy,
> + param->u.crypt.idx);
> +
> + pbcmc_sta = rtw_get_bcmc_stainfo(padapter);
> + if (pbcmc_sta) {
> + pbcmc_sta->ieee8021x_blocked = false;
> + pbcmc_sta->dot118021XPrivacy = psecuritypriv->dot118021XGrpPrivacy; /* rx will use bmc_sta's dot118021XPrivacy. */
> + }
> + }
> + }
> + }
> +
> +free_param:
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_param;
> + }
> +
> + kfree(param);
> + return 0;
> +
> +err_free_param:
> + kfree(param);
> + return ret;
> +}
> +
> +static int rtw_get_sta_wpaie_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret;
> + struct sta_info *psta;
> + struct ieee_param *param;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct sta_priv *pstapriv = &padapter->stapriv;
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (!ret)
> + goto err_free_param;

memdup_user()

> +
> + DBG_88E("rtw_get_sta_wpaie, sta_addr: %pM\n", (param->sta_addr));
> +
> + if (check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)) != true) {
> + 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) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (!psta) {
> + ret = -ENOMEM;
> + goto err_free_param;
> + }
> +
> + if (psta->wpa_ie[0] == WLAN_EID_RSN ||
> + psta->wpa_ie[0] == WLAN_EID_VENDOR_SPECIFIC) {
> + int copy_len;
> + int wpa_ie_len;
> +
> + wpa_ie_len = psta->wpa_ie[1];
> + copy_len = min_t(int, wpa_ie_len + 2, sizeof(psta->wpa_ie));
> + param->u.wpa_ie.len = copy_len;
> + memcpy(param->u.wpa_ie.reserved, psta->wpa_ie, copy_len);
> + } else {
> + DBG_88E("sta's wpa_ie is NONE\n");
> + }
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_param;
> + }
> +
> + kfree(param);
> + return 0;
> +
> +err_free_param:
> + kfree(param);
> + return ret;
> +}
> +
> +static int rtw_set_wps_beacon_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret;
> + int len;
> + int ie_len;
> + struct ieee_param *param;
> + unsigned char wps_oui[4] = {0x0, 0x50, 0xf2, 0x04};
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> + struct mlme_ext_priv *pmlmeext = &(padapter->mlmeextpriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;

memdup_user()

> +
> + len = wrqu->data.length;
> +
> + DBG_88E("%s, len =%d\n", __func__, len);
> +
> + if (check_fwstate(pmlmepriv, WIFI_AP_STATE) != true) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + kfree(pmlmepriv->wps_beacon_ie);
> + pmlmepriv->wps_beacon_ie = NULL;
> +
> + ie_len = len-12-2; /* 12 = Param header, 2 = Not packed. */

Add spaces around opperators:

ie_len = len - 12 - 2;

> + if (ie_len > 0) {
> + pmlmepriv->wps_beacon_ie = kmalloc(ie_len, GFP_KERNEL);
> + if (!pmlmepriv->wps_beacon_ie) {
> + DBG_88E("%s()-%d: kmalloc() ERROR!\n", __func__, __LINE__);

Delete this printk after a kmalloc(). Kmalloc() already has a stack
trace built-in.

> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + pmlmepriv->wps_beacon_ie_len = ie_len;
> + memcpy(pmlmepriv->wps_beacon_ie, param->u.bcn_ie.buf, ie_len);
> + update_beacon(padapter, _VENDOR_SPECIFIC_IE_, wps_oui, true);
> + pmlmeext->bstart_bss = true;
> + }
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_wps_beacon_ie;
> + }
> +
> + kfree(pmlmepriv->wps_beacon_ie);
> + kfree(param);
> + return 0;
> +
> +err_free_wps_beacon_ie:
> + kfree(pmlmepriv->wps_beacon_ie);
> +
> +err_free_param:
> + kfree(param);
> + return ret;
> +}
> +
> +static int rtw_set_wps_probe_resp_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret;
> + int len;
> + int ie_len;
> + struct ieee_param *param;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;
> +

memdup_user()

> + len = wrqu->data.length;
> +
> + DBG_88E("%s, len =%d\n", __func__, len);
> +
> + if (check_fwstate(pmlmepriv, WIFI_AP_STATE) != true) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + kfree(pmlmepriv->wps_probe_resp_ie);
> + pmlmepriv->wps_probe_resp_ie = NULL;

We should have a local variable instead of using
pmlmepriv->wps_probe_resp_ie. The problem with a shared variable is
that there is no locking and it could have a race and use after free and
it's possibly a security issue. Also it looks like it could buffer
overflow as well because we're re-allocating it with different sizes.

> +
> + ie_len = len-12-2; /* 12 = Param header, 2 = Not packed. */

Add spaces

> + if (ie_len > 0) {

Say ie_len is <= 0, isn't that -EINVAL?

> + pmlmepriv->wps_probe_resp_ie = kmalloc(ie_len, GFP_KERNEL);
> + if (!pmlmepriv->wps_probe_resp_ie) {
> + DBG_88E("%s()-%d: kmalloc() ERROR!\n", __func__, __LINE__);
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + pmlmepriv->wps_probe_resp_ie_len = ie_len;
> + memcpy(pmlmepriv->wps_probe_resp_ie, param->u.bcn_ie.buf, ie_len);
> + }
> +
> + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length)) {
> + ret = -EFAULT;
> + goto err_free_wps_probe_resp_ie;
> + }
> +
> + kfree(pmlmepriv->wps_probe_resp_ie);
> + kfree(param);
> + return 0;
> +
> +err_free_wps_probe_resp_ie:
> + kfree(pmlmepriv->wps_probe_resp_ie);
> +
> +err_free_param:
> + kfree(param);
> + return ret;
> +}
> +
> +static int rtw_set_wps_assoc_resp_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret;
> + int len;
> + int ie_len;
> + struct ieee_param *param;
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> +
> + param = (struct ieee_param *)kmalloc(wrqu->data.length, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> + if (ret)
> + goto err_free_param;

memdup_user()

> +
> + len = wrqu->data.length;
> +
> + DBG_88E("%s, len =%d\n", __func__, len);
> +
> + if (check_fwstate(pmlmepriv, WIFI_AP_STATE) != true) {
> + ret = -EINVAL;
> + goto err_free_param;
> + }
> +
> + kfree(pmlmepriv->wps_assoc_resp_ie);
> + pmlmepriv->wps_assoc_resp_ie = NULL;

Use local variable

I have to go to sleep now, but I can't stress enough how much better
this version is than v1 and v2. Thanks very much.

regards,
dan carpenter

Next message: Solio Sarabia: "[PATCH RFC] veth: make veth aware of gso buffer size"
Previous message: Willem de Bruijn: "Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets"
In reply to: ishraq . i . ashraf: "[PATCH v3] staging: rtl8188eu: Fix private WEXT IOCTL calls"
Next in thread: Johannes Berg: "Re: [PATCH v3] staging: rtl8188eu: Fix private WEXT IOCTL calls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]