staging: wilc1000: validate input to set_wiphy_param and return proper

PATCH Superseded [email protected] 2 participants 5 messages Nov 8, 2018 Staging

lore

Share & tools

lore

[email protected]

author

2018-11-08 21:50 UTC #0 lore raw

v4 cover staging: wilc1000: validate input to set_wiphy_param and return proper

From: Adham Abozaeid <[email protected]>

Validate input parameters to set_wiphy_param before scheduling handle_cfg_param() to validate them.
This way proper errors can be returned to caller. Also cleaned up unused code in handle_cfg_param.

Changes since v1:
- Correction spelling in subject of patch#2
- Added From: tag to have correct email from value

Changes since v2:
- Reverted unnecessary file permission changes

Changes since v3:
- Fixed 0-day bot warnings for always true conditions for retry_short and retry_long
- Fixed typo in From: tag

Adham Abozaeid (3):
staging: wilc1000: validate cfg parameters before scheduling the work
staging: wilc1000: Don't keep a copy of wiphy parameters in the driver
staging: wilc1000: Remove unused mutex cfg_values_lock

 drivers/staging/wilc1000/host_interface.c     | 75 ++++---------------
 drivers/staging/wilc1000/host_interface.h     |  3 -
 .../staging/wilc1000/wilc_wfi_cfgoperations.c | 32 +++++++-
 3 files changed, 44 insertions(+), 66 deletions(-)

Signature · 1 line

2.17.1

[email protected]

author

2018-11-08 21:50 UTC #1 lore raw

PATCH v4 1/3 staging: wilc1000: validate cfg parameters before scheduling the work

From: Adham Abozaeid <[email protected]>

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 <[email protected]>

---
 drivers/staging/wilc1000/host_interface.c     | 61 ++++++-------------
 .../staging/wilc1000/wilc_wfi_cfgoperations.c | 32 ++++++++--
 2 files changed, 48 insertions(+), 45 deletions(-)

drivers/staging/wilc1000/host_interface.c +20 ~~-41~~

index b89116c57064..c1215c194907 100644
@@ -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);
 }

drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +28 -4

index 4fbbbbd5a64b..a6f4fad43bf7 100644
@@ -1149,21 +1149,45 @@ static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
 	cfg_param_val.flag = 0;
 
 	if (changed & WIPHY_PARAM_RETRY_SHORT) {
+		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;
 	}
 	if (changed & WIPHY_PARAM_RETRY_LONG) {
+		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;
 	}
 	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);

Signature · 1 line

2.17.1

Greg KH

maintainer

2018-11-08 22:13 UTC #2 lore raw

Re: [PATCH v4 1/3] staging: wilc1000: validate cfg parameters before scheduling the work

Quoted 3 lines · On Thu, Nov 08, 2018 at 09:50:25PM +0000, [email protected] wrote:

From: Adham Abozaeid <[email protected]>
From: Adham Abozaeid <[email protected]>

Twice? Something went wrong on your side, for all of these patches :(

Please fix up and resend.

thanks,

greg k-h

[email protected]

author

2018-11-08 21:50 UTC #3 lore raw

PATCH v4 2/3 staging: wilc1000: Don't keep a copy of wiphy parameters in the driver

In reply to [email protected]

From: Adham Abozaeid <[email protected]>

host_if_drv.cfg_values is a write only member, and can be removed

Signed-off-by Adham Abozaeid <[email protected]>

---
 drivers/staging/wilc1000/host_interface.c | 13 -------------
 drivers/staging/wilc1000/host_interface.h |  1 -
 2 files changed, 14 deletions(-)

drivers/staging/wilc1000/host_interface.c +0 ~~-13~~

index c1215c194907..8d1142776310 100644
@@ -375,43 +375,31 @@ static void handle_cfg_param(struct work_struct *work)
 	mutex_lock(&hif_drv->cfg_values_lock);
 
 	if (param->flag & RETRY_SHORT) {
-		u16 retry_limit = param->short_retry_limit;
-
 		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;
-
 		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;
-
 		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;
-
 		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++;
 	}

@@ -3256,7 +3244,6 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	mutex_lock(&hif_drv->cfg_values_lock);
 
 	hif_drv->hif_state = HOST_IF_IDLE;
-	hif_drv->cfg_values.scan_source = DEFAULT_SCAN;
 
 	hif_drv->p2p_timeout = 0;

drivers/staging/wilc1000/host_interface.h +0 -1

index df9fc33be094..1e2e50e91f76 100644
@@ -293,7 +293,6 @@ struct host_if_drv {
 	enum host_if_state hif_state;
 
 	u8 assoc_bssid[ETH_ALEN];
-	struct cfg_param_attr cfg_values;
 	/*lock to protect concurrent setting of cfg params*/
 	struct mutex cfg_values_lock;

Signature · 1 line

2.17.1

[email protected]

author

2018-11-08 21:50 UTC #4 lore raw

PATCH v4 3/3 staging: wilc1000: Remove unused mutex cfg_values_lock

In reply to [email protected]

From: Adham Abozaeid <[email protected]>

After removing cfg_values member, cfg_values_lock that was used to protect it can also be removed.

Signed-off-by Adham Abozaeid <[email protected]>

---
 drivers/staging/wilc1000/host_interface.c | 9 ---------
 drivers/staging/wilc1000/host_interface.h | 2 --
 2 files changed, 11 deletions(-)

drivers/staging/wilc1000/host_interface.c +0 -9

index 8d1142776310..2bf91df1ded1 100644
@@ -369,11 +369,8 @@ static void handle_cfg_param(struct work_struct *work)
 	struct cfg_param_attr *param = &msg->body.cfg_info;
 	int ret;
 	struct wid wid_list[32];
-	struct host_if_drv *hif_drv = vif->hif_drv;
 	int i = 0;
 
-	mutex_lock(&hif_drv->cfg_values_lock);
-
 	if (param->flag & RETRY_SHORT) {
 		wid_list[i].id = WID_SHORT_RETRY_LIMIT;
 		wid_list[i].val = (s8 *)&param->short_retry_limit;

@@ -409,7 +406,6 @@ static void handle_cfg_param(struct work_struct *work)
 	if (ret)
 		netdev_err(vif->ndev, "Error in setting CFG params\n");
 
-	mutex_unlock(&hif_drv->cfg_values_lock);
 	kfree(msg);
 }

@@ -3240,15 +3236,10 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	timer_setup(&hif_drv->connect_timer, timer_connect_cb, 0);
 	timer_setup(&hif_drv->remain_on_ch_timer, listen_timer_cb, 0);
 
-	mutex_init(&hif_drv->cfg_values_lock);
-	mutex_lock(&hif_drv->cfg_values_lock);
-
 	hif_drv->hif_state = HOST_IF_IDLE;
 
 	hif_drv->p2p_timeout = 0;
 
-	mutex_unlock(&hif_drv->cfg_values_lock);
-
 	wilc->clients_count++;
 
 	return 0;

drivers/staging/wilc1000/host_interface.h +0 -2

index 1e2e50e91f76..523b46bfb488 100644
@@ -293,8 +293,6 @@ struct host_if_drv {
 	enum host_if_state hif_state;
 
 	u8 assoc_bssid[ETH_ALEN];
-	/*lock to protect concurrent setting of cfg params*/
-	struct mutex cfg_values_lock;
 
 	struct timer_list scan_timer;
 	struct wilc_vif *scan_timer_vif;

Signature · 1 line

2.17.1