[PATCH 0/8] staging: wfx: fix issues reported by Smatch

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Fix the issues reported by Dan using Smatch[1].

[1] https://lore.kernel.org/lkml/20201008131320.GA1042@kadam/

Jérôme Pouiller (8):
  staging: wfx: improve error handling of hif_join()
  staging: wfx: check memory allocation
  staging: wfx: standardize the error when vif does not exist
  staging: wfx: wfx_init_common() returns NULL on error
  staging: wfx: increase robustness of hif_generic_confirm()
  staging: wfx: gpiod_get_value() can return an error
  staging: wfx: drop unicode characters from strings
  staging: wfx: improve robustness of wfx_get_hw_rate()

 drivers/staging/wfx/bh.c      |  2 +-
 drivers/staging/wfx/data_tx.c |  9 ++++++-
 drivers/staging/wfx/hif_rx.c  | 44 +++++++++++++++++++++++------------
 drivers/staging/wfx/hif_tx.c  |  4 +++-
 drivers/staging/wfx/main.c    | 10 ++++++--
 drivers/staging/wfx/sta.c     |  4 ++++
 6 files changed, 53 insertions(+), 20 deletions(-)

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

   hif_tx.c:319 hif_join() error: we previously assumed 'channel' could be null (see line 315)
   311          if (!hif)
   312                  return -ENOMEM;
   313          body->infrastructure_bss_mode = !conf->ibss_joined;
   314          body->short_preamble = conf->use_short_preamble;
   315          if (channel && channel->flags & IEEE80211_CHAN_NO_IR)
                    ^^^^^^^
   316                  body->probe_for_join = 0;
   317          else
   318                  body->probe_for_join = 1;
   319          body->channel_number = channel->hw_value;
                                       ^^^^^^^^^^^^^^^^^
   320          body->beacon_interval = cpu_to_le32(conf->beacon_int);
   321          body->basic_rate_set =

Indeed, channel can't be NULL (else I would have seen plenty of Ooops
this past year). This patch explicitly claims this restriction.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index e61cc2486761..63b437261eb7 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c

@@ -308,11 +308,13 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
 	WARN_ON(!conf->basic_rates);
 	WARN_ON(sizeof(body->ssid) < ssidlen);
 	WARN(!conf->ibss_joined && !ssidlen, "joining an unknown BSS");
+	if (WARN_ON(!channel))
+		return -EINVAL;
 	if (!hif)
 		return -ENOMEM;
 	body->infrastructure_bss_mode = !conf->ibss_joined;
 	body->short_preamble = conf->use_short_preamble;
-	if (channel && channel->flags & IEEE80211_CHAN_NO_IR)
+	if (channel->flags & IEEE80211_CHAN_NO_IR)
 		body->probe_for_join = 0;
 	else
 		body->probe_for_join = 1;

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

   main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
   227          tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
   228          ret = wfx_send_pds(wdev, tmp_buf, pds->size);
                                         ^^^^^^^
   229          kfree(tmp_buf);

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index df11c091e094..a8dc2c033410 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c

@@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
 	if (ret) {
 		dev_err(wdev->dev, "can't load PDS file %s\n",
 			wdev->pdata.file_pds);
-		return ret;
+		goto err1;
 	}
 	tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
+	if (!tmp_buf) {
+		ret = -ENOMEM;
+		goto err2;
+	}
 	ret = wfx_send_pds(wdev, tmp_buf, pds->size);
 	kfree(tmp_buf);
+err2:
 	release_firmware(pds);
+err1:
 	return ret;
 }
 

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

  hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xc2'
  hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xb0'
   234                  if (!wfx_api_older_than(wdev, 1, 4))
   235                          dev_info(wdev->dev, "Rx test ongoing. Temperature: %d°C\n",
                                                                                     ^
   236                                   body->data.rx_stats.current_temp);

So, replace the unicode character.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index f99921e76059..56a5f891447b 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c

@@ -246,7 +246,7 @@ static int hif_generic_indication(struct wfx_dev *wdev,
 		mutex_lock(&wdev->rx_stats_lock);
 		// Older firmware send a generic indication beside RxStats
 		if (!wfx_api_older_than(wdev, 1, 4))
-			dev_info(wdev->dev, "Rx test ongoing. Temperature: %d°C\n",
+			dev_info(wdev->dev, "Rx test ongoing. Temperature: %d degrees C\n",
 				 body->data.rx_stats.current_temp);
 		memcpy(&wdev->rx_stats, &body->data.rx_stats,
 		       sizeof(wdev->rx_stats));

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

    data_tx.c:37 wfx_get_hw_rate() warn: constraint '(struct ieee80211_supported_band)->bitrates' overflow 'band->bitrates' 0 <= abs_rl '0-127' user_rl '' required = '(struct ieee80211_supported_band)->n_bitrates'
    23          struct ieee80211_supported_band *band;
    24
    25          if (rate->idx < 0)
    26                  return -1;
    27          if (rate->flags & IEEE80211_TX_RC_MCS) {
    28                  if (rate->idx > 7) {
    29                          WARN(1, "wrong rate->idx value: %d", rate->idx);
    30                          return -1;
    31                  }
    32                  return rate->idx + 14;
    33          }
    34          // WFx only support 2GHz, else band information should be retrieved
    35          // from ieee80211_tx_info
    36          band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];
    37          return band->bitrates[rate->idx].hw_value;

Add a simple check to make Smatch happy.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/data_tx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index 8db0be08daf8..41f6a604a697 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c

@@ -31,6 +31,10 @@ static int wfx_get_hw_rate(struct wfx_dev *wdev,
 		}
 		return rate->idx + 14;
 	}
+	if (rate->idx >= band->n_bitrates) {
+		WARN(1, "wrong rate->idx value: %d", rate->idx);
+		return -1;
+	}
 	// WFx only support 2GHz, else band information should be retrieved
 	// from ieee80211_tx_info
 	band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

    drivers/staging/wfx/hif_rx.c:26 hif_generic_confirm() warn: negative user subtract: 0-u16max - 4
    20  static int hif_generic_confirm(struct wfx_dev *wdev,
    21                                 const struct hif_msg *hif, const void *buf)
    22  {
    23          // All confirm messages start with status
    24          int status = le32_to_cpup((__le32 *)buf);
    25          int cmd = hif->id;
    26          int len = le16_to_cpu(hif->len) - 4; // drop header
                                              ^^^^^
    27
    28          WARN(!mutex_is_locked(&wdev->hif_cmd.lock), "data locking error");

In fact, rx_helper() already make the necessary checks on the value of
hif->len. Never mind, add an explicit check to make Smatch happy.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_rx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index ca09467cba05..2d4265257112 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c

@@ -40,10 +40,10 @@ static int hif_generic_confirm(struct wfx_dev *wdev,
 	}
 
 	if (wdev->hif_cmd.buf_recv) {
-		if (wdev->hif_cmd.len_recv >= len)
+		if (wdev->hif_cmd.len_recv >= len && len > 0)
 			memcpy(wdev->hif_cmd.buf_recv, buf, len);
 		else
-			status = -ENOMEM;
+			status = -EIO;
 	}
 	wdev->hif_cmd.ret = status;
 

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

    hif_rx.c:98 hif_wakeup_indication() warn: 'gpiod_get_value(wdev->pdata.gpio_wakeup)' returns positive and negative
    bh.c:24 device_wakeup() warn: 'gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup)' returns positive and negative

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/bh.c     | 2 +-
 drivers/staging/wfx/hif_rx.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 586b769c0446..2ffa587aefaa 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c

@@ -21,7 +21,7 @@ static void device_wakeup(struct wfx_dev *wdev)
 
 	if (!wdev->pdata.gpio_wakeup)
 		return;
-	if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup))
+	if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup) >= 0)
 		return;
 
 	if (wfx_api_older_than(wdev, 1, 4)) {

diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index 2d4265257112..f99921e76059 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c

@@ -94,8 +94,8 @@ static int hif_startup_indication(struct wfx_dev *wdev,
 static int hif_wakeup_indication(struct wfx_dev *wdev,
 				 const struct hif_msg *hif, const void *buf)
 {
-	if (!wdev->pdata.gpio_wakeup
-	    || !gpiod_get_value(wdev->pdata.gpio_wakeup)) {
+	if (!wdev->pdata.gpio_wakeup ||
+	    gpiod_get_value(wdev->pdata.gpio_wakeup) == 0) {
 		dev_warn(wdev->dev, "unexpected wake-up indication\n");
 		return -EIO;
 	}

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

    bus_spi.c:228 wfx_spi_probe() warn: 'bus->core' could be an error pointer
    bus_sdio.c:221 wfx_sdio_probe() warn: 'bus->core' could be an error pointer

bus->core contains the result of wfx_init_common(). With this patch,
wfx_init_common() returns a valid pointer or NULL.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index a8dc2c033410..e7bc1988124a 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c

@@ -309,7 +309,7 @@ struct wfx_dev *wfx_init_common(struct device *dev,
 	wdev->pdata.gpio_wakeup = devm_gpiod_get_optional(dev, "wakeup",
 							  GPIOD_OUT_LOW);
 	if (IS_ERR(wdev->pdata.gpio_wakeup))
-		return ERR_CAST(wdev->pdata.gpio_wakeup);
+		return NULL;
 	if (wdev->pdata.gpio_wakeup)
 		gpiod_set_consumer_name(wdev->pdata.gpio_wakeup, "wfx wakeup");
 

-- 
2.28.0

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Smatch complains:

   drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
   drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'

Indeed, if the vif id returned by the device does not exist anymore,
wdev_to_wvif() could return NULL.

In add, the error is not handled uniformly in the code, sometime a
WARN() is displayed but code continue, sometime a dev_warn() is
displayed, sometime it is just not tested, ...

This patch standardize that.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/data_tx.c |  5 ++++-
 drivers/staging/wfx/hif_rx.c  | 34 ++++++++++++++++++++++++----------
 drivers/staging/wfx/sta.c     |  4 ++++
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index b4d5dd3d2d23..8db0be08daf8 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c

@@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb)
 			      sizeof(struct hif_req_tx) +
 			      req->fc_offset;
 
-	WARN_ON(!wvif);
+	if (!wvif) {
+		pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__);
+		return;
+	}
 	wfx_tx_policy_put(wvif, req->retry_policy_index);
 	skb_pull(skb, offset);
 	ieee80211_tx_status_irqsafe(wvif->wdev->hw, skb);

diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index d6dfab094b03..ca09467cba05 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c

@@ -110,9 +110,9 @@ static int hif_receive_indication(struct wfx_dev *wdev,
 	const struct hif_ind_rx *body = buf;
 
 	if (!wvif) {
-		dev_warn(wdev->dev, "ignore rx data for non-existent vif %d\n",
-			 hif->interface);
-		return 0;
+		dev_warn(wdev->dev, "%s: ignore rx data for non-existent vif %d\n",
+			 __func__, hif->interface);
+		return -EIO;
 	}
 	skb_pull(skb, sizeof(struct hif_msg) + sizeof(struct hif_ind_rx));
 	wfx_rx_cb(wvif, body, skb);

@@ -128,8 +128,8 @@ static int hif_event_indication(struct wfx_dev *wdev,
 	int type = le32_to_cpu(body->event_id);
 
 	if (!wvif) {
-		dev_warn(wdev->dev, "received event for non-existent vif\n");
-		return 0;
+		dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+		return -EIO;
 	}
 
 	switch (type) {

@@ -161,7 +161,10 @@ static int hif_pm_mode_complete_indication(struct wfx_dev *wdev,
 {
 	struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
 
-	WARN_ON(!wvif);
+	if (!wvif) {
+		dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+		return -EIO;
+	}
 	complete(&wvif->set_pm_mode_complete);
 
 	return 0;

@@ -173,7 +176,11 @@ static int hif_scan_complete_indication(struct wfx_dev *wdev,
 {
 	struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
 
-	WARN_ON(!wvif);
+	if (!wvif) {
+		dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+		return -EIO;
+	}
+
 	wfx_scan_complete(wvif);
 
 	return 0;

@@ -185,7 +192,10 @@ static int hif_join_complete_indication(struct wfx_dev *wdev,
 {
 	struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
 
-	WARN_ON(!wvif);
+	if (!wvif) {
+		dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+		return -EIO;
+	}
 	dev_warn(wdev->dev, "unattended JoinCompleteInd\n");
 
 	return 0;

@@ -195,11 +205,15 @@ static int hif_suspend_resume_indication(struct wfx_dev *wdev,
 					 const struct hif_msg *hif,
 					 const void *buf)
 {
-	struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
 	const struct hif_ind_suspend_resume_tx *body = buf;
+	struct wfx_vif *wvif;
 
 	if (body->bc_mc_only) {
-		WARN_ON(!wvif);
+		wvif = wdev_to_wvif(wdev, hif->interface);
+		if (!wvif) {
+			dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+			return -EIO;
+		}
 		if (body->resume)
 			wfx_suspend_resume_mc(wvif, STA_NOTIFY_AWAKE);
 		else

diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index a246f0d1d6e9..2320a81eae0b 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c

@@ -619,6 +619,10 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set)
 	struct wfx_sta_priv *sta_dev = (struct wfx_sta_priv *)&sta->drv_priv;
 	struct wfx_vif *wvif = wdev_to_wvif(wdev, sta_dev->vif_id);
 
+	if (!wvif) {
+		dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+		return -EIO;
+	}
 	schedule_work(&wvif->update_tim_work);
 	return 0;
 }

-- 
2.28.0

On Fri, Oct 09, 2020 at 07:13:07PM +0200, Jerome Pouiller wrote:

7 more quoted lines

This now causes a clang warning:

drivers/staging/wfx/data_tx.c:34:19: warning: variable 'band' is uninitialized when used here [-Wuninitialized]
        if (rate->idx >= band->n_bitrates) {
                         ^~~~
drivers/staging/wfx/data_tx.c:23:39: note: initialize the variable 'band' to silence this warning
        struct ieee80211_supported_band *band;
                                             ^
                                              = NULL
1 warning generated.

Cheers,
Nathan