diff mbox series

[v3,2/4] staging: wilc1000: validate cfg parameters before scheduling the work

Message ID 20181106000109.6178-3-adham.abozaeid@microchip.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show
Series staging: wilc1000: validate input to set_wiphy_param and return proper errors | expand

Commit Message

Adham Abozaeid Nov. 6, 2018, 12:01 a.m. UTC 
From: Adham Abozaeid <adham.abozaeid@micochip.com>

Validate cfg parameters after being called by cfg80211 in set_wiphy_params
before scheduling the work executed in handle_cfg_param

Signed-off-by: Adham Abozaeid <adham.abozaeid@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c     | 61 ++++++-------------
 .../staging/wilc1000/wilc_wfi_cfgoperations.c | 50 ++++++++++++---
 2 files changed, 62 insertions(+), 49 deletions(-)

Comments

kernel test robot Nov. 7, 2018, 8:46 a.m. UTC | #1 
Hi Adham,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.20-rc1 next-20181106]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Adham-Abozaeid-microchip-com/staging-wilc1000-validate-input-to-set_wiphy_param-and-return-proper-errors/20181106-120308

smatch warnings:
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c:1152 set_wiphy_params() warn: always true condition '(wiphy->retry_short < 256) => (0-255 < 256)'
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c:1164 set_wiphy_params() warn: always true condition '(wiphy->retry_long < 256) => (0-255 < 256)'

vim +1152 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c

  1141	
  1142	static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
  1143	{
  1144		int ret;
  1145		struct cfg_param_attr cfg_param_val;
  1146		struct wilc_priv *priv = wiphy_priv(wiphy);
  1147		struct wilc_vif *vif = netdev_priv(priv->dev);
  1148	
  1149		cfg_param_val.flag = 0;
  1150	
  1151		if (changed & WIPHY_PARAM_RETRY_SHORT) {
> 1152			if (wiphy->retry_short > 0 && wiphy->retry_short < 256) {
  1153				netdev_dbg(vif->ndev,
  1154					   "Setting WIPHY_PARAM_RETRY_SHORT %d\n",
  1155					   wiphy->retry_short);
  1156				cfg_param_val.flag  |= RETRY_SHORT;
  1157				cfg_param_val.short_retry_limit = wiphy->retry_short;
  1158			} else {
  1159				netdev_err(vif->ndev, "Short retry limit out of range\n");
  1160				return -EINVAL;
  1161			}
  1162		}
  1163		if (changed & WIPHY_PARAM_RETRY_LONG) {
> 1164			if (wiphy->retry_long > 0 && wiphy->retry_long < 256) {
  1165				netdev_dbg(vif->ndev,
  1166					   "Setting WIPHY_PARAM_RETRY_LONG %d\n",
  1167					   wiphy->retry_long);
  1168				cfg_param_val.flag |= RETRY_LONG;
  1169				cfg_param_val.long_retry_limit = wiphy->retry_long;
  1170			} else {
  1171				netdev_err(vif->ndev, "Long retry limit out of range\n");
  1172				return -EINVAL;
  1173			}
  1174		}
  1175		if (changed & WIPHY_PARAM_FRAG_THRESHOLD) {
  1176			if (wiphy->frag_threshold > 255 &&
  1177			    wiphy->frag_threshold < 7937) {
  1178				netdev_dbg(vif->ndev,
  1179					   "Setting WIPHY_PARAM_FRAG_THRESHOLD %d\n",
  1180					   wiphy->frag_threshold);
  1181				cfg_param_val.flag |= FRAG_THRESHOLD;
  1182				cfg_param_val.frag_threshold = wiphy->frag_threshold;
  1183			} else {
  1184				netdev_err(vif->ndev,
  1185					   "Fragmentation threshold out of range\n");
  1186				return -EINVAL;
  1187			}
  1188		}
  1189	
  1190		if (changed & WIPHY_PARAM_RTS_THRESHOLD) {
  1191			if (wiphy->rts_threshold > 255) {
  1192				netdev_dbg(vif->ndev,
  1193					   "Setting WIPHY_PARAM_RTS_THRESHOLD %d\n",
  1194					   wiphy->rts_threshold);
  1195				cfg_param_val.flag |= RTS_THRESHOLD;
  1196				cfg_param_val.rts_threshold = wiphy->rts_threshold;
  1197			} else {
  1198				netdev_err(vif->ndev, "RTS threshold out of range\n");
  1199				return -EINVAL;
  1200			}
  1201		}
  1202	
  1203		ret = wilc_hif_set_cfg(vif, &cfg_param_val);
  1204		if (ret)
  1205			netdev_err(priv->dev, "Error in setting WIPHY PARAMS\n");
  1206	
  1207		return ret;
  1208	}
  1209	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Greg KH Nov. 8, 2018, 11:22 a.m. UTC | #2 
On Tue, Nov 06, 2018 at 12:01:18AM +0000, Adham.Abozaeid@microchip.com wrote:
> From: Adham Abozaeid <adham.abozaeid@micochip.com>
> 
> Validate cfg parameters after being called by cfg80211 in set_wiphy_params
> before scheduling the work executed in handle_cfg_param
> 
> Signed-off-by: Adham Abozaeid <adham.abozaeid@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c     | 61 ++++++-------------
>  .../staging/wilc1000/wilc_wfi_cfgoperations.c | 50 ++++++++++++---
>  2 files changed, 62 insertions(+), 49 deletions(-)

Please fix this up so that the 0-day bot does not complain about it.

thanks,

greg k-h
Adham Abozaeid Nov. 8, 2018, 4:27 p.m. UTC | #3 
On 11/8/18 4:22 AM, Greg KH wrote:
> On Tue, Nov 06, 2018 at 12:01:18AM +0000, Adham.Abozaeid@microchip.com wrote:
>> From: Adham Abozaeid <adham.abozaeid@micochip.com>
>>
>> Validate cfg parameters after being called by cfg80211 in set_wiphy_params
>> before scheduling the work executed in handle_cfg_param
>>
>> Signed-off-by: Adham Abozaeid <adham.abozaeid@microchip.com>
>> ---
>>  drivers/staging/wilc1000/host_interface.c     | 61 ++++++-------------
>>  .../staging/wilc1000/wilc_wfi_cfgoperations.c | 50 ++++++++++++---
>>  2 files changed, 62 insertions(+), 49 deletions(-)
> Please fix this up so that the 0-day bot does not complain about it.

Sure Greg. I'll submit v4 with the fix for the bot warning.
I assume I shouldn't include patch 1/4 in the new version of the series since I see it was applied already, so I'll only resend the other 3 patches

Thanks,
Adham
diff mbox series

Patch

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index b89116c57064..c1215c194907 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -377,61 +377,41 @@  static void handle_cfg_param(struct work_struct *work)
 	if (param->flag & RETRY_SHORT) {
 		u16 retry_limit = param->short_retry_limit;
 
-		if (retry_limit > 0 && retry_limit < 256) {
-			wid_list[i].id = WID_SHORT_RETRY_LIMIT;
-			wid_list[i].val = (s8 *)&param->short_retry_limit;
-			wid_list[i].type = WID_SHORT;
-			wid_list[i].size = sizeof(u16);
-			hif_drv->cfg_values.short_retry_limit = retry_limit;
-		} else {
-			netdev_err(vif->ndev, "Range(1~256) over\n");
-			goto unlock;
-		}
+		wid_list[i].id = WID_SHORT_RETRY_LIMIT;
+		wid_list[i].val = (s8 *)&param->short_retry_limit;
+		wid_list[i].type = WID_SHORT;
+		wid_list[i].size = sizeof(u16);
+		hif_drv->cfg_values.short_retry_limit = retry_limit;
 		i++;
 	}
 	if (param->flag & RETRY_LONG) {
 		u16 limit = param->long_retry_limit;
 
-		if (limit > 0 && limit < 256) {
-			wid_list[i].id = WID_LONG_RETRY_LIMIT;
-			wid_list[i].val = (s8 *)&param->long_retry_limit;
-			wid_list[i].type = WID_SHORT;
-			wid_list[i].size = sizeof(u16);
-			hif_drv->cfg_values.long_retry_limit = limit;
-		} else {
-			netdev_err(vif->ndev, "Range(1~256) over\n");
-			goto unlock;
-		}
+		wid_list[i].id = WID_LONG_RETRY_LIMIT;
+		wid_list[i].val = (s8 *)&param->long_retry_limit;
+		wid_list[i].type = WID_SHORT;
+		wid_list[i].size = sizeof(u16);
+		hif_drv->cfg_values.long_retry_limit = limit;
 		i++;
 	}
 	if (param->flag & FRAG_THRESHOLD) {
 		u16 frag_th = param->frag_threshold;
 
-		if (frag_th > 255 && frag_th < 7937) {
-			wid_list[i].id = WID_FRAG_THRESHOLD;
-			wid_list[i].val = (s8 *)&param->frag_threshold;
-			wid_list[i].type = WID_SHORT;
-			wid_list[i].size = sizeof(u16);
-			hif_drv->cfg_values.frag_threshold = frag_th;
-		} else {
-			netdev_err(vif->ndev, "Threshold Range fail\n");
-			goto unlock;
-		}
+		wid_list[i].id = WID_FRAG_THRESHOLD;
+		wid_list[i].val = (s8 *)&param->frag_threshold;
+		wid_list[i].type = WID_SHORT;
+		wid_list[i].size = sizeof(u16);
+		hif_drv->cfg_values.frag_threshold = frag_th;
 		i++;
 	}
 	if (param->flag & RTS_THRESHOLD) {
 		u16 rts_th = param->rts_threshold;
 
-		if (rts_th > 255) {
-			wid_list[i].id = WID_RTS_THRESHOLD;
-			wid_list[i].val = (s8 *)&param->rts_threshold;
-			wid_list[i].type = WID_SHORT;
-			wid_list[i].size = sizeof(u16);
-			hif_drv->cfg_values.rts_threshold = rts_th;
-		} else {
-			netdev_err(vif->ndev, "Threshold Range fail\n");
-			goto unlock;
-		}
+		wid_list[i].id = WID_RTS_THRESHOLD;
+		wid_list[i].val = (s8 *)&param->rts_threshold;
+		wid_list[i].type = WID_SHORT;
+		wid_list[i].size = sizeof(u16);
+		hif_drv->cfg_values.rts_threshold = rts_th;
 		i++;
 	}
 
@@ -441,7 +421,6 @@  static void handle_cfg_param(struct work_struct *work)
 	if (ret)
 		netdev_err(vif->ndev, "Error in setting CFG params\n");
 
-unlock:
 	mutex_unlock(&hif_drv->cfg_values_lock);
 	kfree(msg);
 }
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 4fbbbbd5a64b..26bb78a49d81 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1149,21 +1149,55 @@  static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
 	cfg_param_val.flag = 0;
 
 	if (changed & WIPHY_PARAM_RETRY_SHORT) {
-		cfg_param_val.flag  |= RETRY_SHORT;
-		cfg_param_val.short_retry_limit = wiphy->retry_short;
+		if (wiphy->retry_short > 0 && wiphy->retry_short < 256) {
+			netdev_dbg(vif->ndev,
+				   "Setting WIPHY_PARAM_RETRY_SHORT %d\n",
+				   wiphy->retry_short);
+			cfg_param_val.flag  |= RETRY_SHORT;
+			cfg_param_val.short_retry_limit = wiphy->retry_short;
+		} else {
+			netdev_err(vif->ndev, "Short retry limit out of range\n");
+			return -EINVAL;
+		}
 	}
 	if (changed & WIPHY_PARAM_RETRY_LONG) {
-		cfg_param_val.flag |= RETRY_LONG;
-		cfg_param_val.long_retry_limit = wiphy->retry_long;
+		if (wiphy->retry_long > 0 && wiphy->retry_long < 256) {
+			netdev_dbg(vif->ndev,
+				   "Setting WIPHY_PARAM_RETRY_LONG %d\n",
+				   wiphy->retry_long);
+			cfg_param_val.flag |= RETRY_LONG;
+			cfg_param_val.long_retry_limit = wiphy->retry_long;
+		} else {
+			netdev_err(vif->ndev, "Long retry limit out of range\n");
+			return -EINVAL;
+		}
 	}
 	if (changed & WIPHY_PARAM_FRAG_THRESHOLD) {
-		cfg_param_val.flag |= FRAG_THRESHOLD;
-		cfg_param_val.frag_threshold = wiphy->frag_threshold;
+		if (wiphy->frag_threshold > 255 &&
+		    wiphy->frag_threshold < 7937) {
+			netdev_dbg(vif->ndev,
+				   "Setting WIPHY_PARAM_FRAG_THRESHOLD %d\n",
+				   wiphy->frag_threshold);
+			cfg_param_val.flag |= FRAG_THRESHOLD;
+			cfg_param_val.frag_threshold = wiphy->frag_threshold;
+		} else {
+			netdev_err(vif->ndev,
+				   "Fragmentation threshold out of range\n");
+			return -EINVAL;
+		}
 	}
 
 	if (changed & WIPHY_PARAM_RTS_THRESHOLD) {
-		cfg_param_val.flag |= RTS_THRESHOLD;
-		cfg_param_val.rts_threshold = wiphy->rts_threshold;
+		if (wiphy->rts_threshold > 255) {
+			netdev_dbg(vif->ndev,
+				   "Setting WIPHY_PARAM_RTS_THRESHOLD %d\n",
+				   wiphy->rts_threshold);
+			cfg_param_val.flag |= RTS_THRESHOLD;
+			cfg_param_val.rts_threshold = wiphy->rts_threshold;
+		} else {
+			netdev_err(vif->ndev, "RTS threshold out of range\n");
+			return -EINVAL;
+		}
 	}
 
 	ret = wilc_hif_set_cfg(vif, &cfg_param_val);