2018-11-08 21:50:28

by Adham Abozaeid

[permalink] [raw]
Subject: [PATCH v4 0/3] 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(-)

--
2.17.1



2018-11-08 21:50:29

by Adham Abozaeid

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

From: Adham Abozaeid <[email protected]>

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(-)

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..a6f4fad43bf7 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -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);
--
2.17.1


2018-11-08 21:50:30

by Adham Abozaeid

[permalink] [raw]
Subject: [PATCH v4 3/3] staging: wilc1000: Remove unused mutex cfg_values_lock

From: Adham Abozaeid <[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(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 8d1142776310..2bf91df1ded1 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -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;
diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
index 1e2e50e91f76..523b46bfb488 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -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;
--
2.17.1


2018-11-08 21:50:31

by Adham Abozaeid

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

From: Adham Abozaeid <[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(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index c1215c194907..8d1142776310 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -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;

diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
index df9fc33be094..1e2e50e91f76 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -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;

--
2.17.1


2018-11-08 22:13:31

by Greg Kroah-Hartman

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

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