From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jerome Pouiller <Jerome.Pouiller@silabs.com>
Cc: devel@driverdev.osuosl.org, netdev@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"David S . Miller" <davem@davemloft.net>,
Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH 0/7] wfx: move out from the staging area
Date: Thu, 8 Oct 2020 16:13:20 +0300 [thread overview]
Message-ID: <20201008131320.GA1042@kadam> (raw)
In-Reply-To: <20201007101943.749898-1-Jerome.Pouiller@silabs.com>
There are some static checker warnings to look at from linux-next from
Tuesday.
drivers/staging/wfx/hif_tx.c:319 hif_join() error: we previously assumed 'channel' could be null (see line 315)
drivers/staging/wfx/main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
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'
drivers/staging/wfx/bus_spi.c:228 wfx_spi_probe() warn: 'bus->core' could be an error pointer
drivers/staging/wfx/bus_sdio.c:221 wfx_sdio_probe() warn: 'bus->core' could be an error pointer
drivers/staging/wfx/hif_rx.c:26 hif_generic_confirm() warn: negative user subtract: 0-u16max - 4
drivers/staging/wfx/hif_rx.c:98 hif_wakeup_indication() warn: 'gpiod_get_value(wdev->pdata.gpio_wakeup)' returns positive and negative
drivers/staging/wfx/bh.c:24 device_wakeup() warn: 'gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup)' returns positive and negative
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xc2'
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xb0'
drivers/staging/wfx/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'
Some of these are unpublished checks that I haven't published because
they are too crap. The rest of the email is just long explanations.
Skip if not required.
regards,
dan carpenter
#1
drivers/staging/wfx/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)
^^^^^^^
Check for NULL.
316 body->probe_for_join = 0;
317 else
318 body->probe_for_join = 1;
319 body->channel_number = channel->hw_value;
^^^^^^^^^^^^^^^^^
Unchecked dereference.
320 body->beacon_interval = cpu_to_le32(conf->beacon_int);
321 body->basic_rate_set =
#2
drivers/staging/wfx/main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
No check for allocation failure.
228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
229 kfree(tmp_buf);
#3
drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
170 static int hif_scan_complete_indication(struct wfx_dev *wdev,
171 const struct hif_msg *hif,
172 const void *buf)
173 {
174 struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
^^^^^^^^^^^^^^^^^^^
Smatch thinks wdev_to_wvif() can return NULL.
175
176 WARN_ON(!wvif);
177 wfx_scan_complete(wvif);
178
179 return 0;
180 }
#4
drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
572 while ((skb = skb_dequeue(&dropped)) != NULL) {
573 hif = (struct hif_msg *)skb->data;
574 wvif = wdev_to_wvif(wdev, hif->interface);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Same.
575 ieee80211_tx_info_clear_status(IEEE80211_SKB_CB(skb));
576 wfx_skb_dtor(wvif, skb);
577 }
578 }
#5 and #6
drivers/staging/wfx/bus_spi.c:228 wfx_spi_probe() warn: 'bus->core' could be an error pointer
drivers/staging/wfx/bus_sdio.c:221 wfx_sdio_probe() warn: 'bus->core' could be an error pointer
The wfx_init_common() function should return NULL instead of error
pointer if devm_gpiod_get_optional() fails.
#7
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
Can "len" get set to negative 4?
27
28 WARN(!mutex_is_locked(&wdev->hif_cmd.lock), "data locking error");
#8 and #9
drivers/staging/wfx/hif_rx.c:98 hif_wakeup_indication() warn: 'gpiod_get_value(wdev->pdata.gpio_wakeup)' returns positive and negative
drivers/staging/wfx/bh.c:24 device_wakeup() warn: 'gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup)' returns positive and negative
94 static int hif_wakeup_indication(struct wfx_dev *wdev,
95 const struct hif_msg *hif, const void *buf)
96 {
97 if (!wdev->pdata.gpio_wakeup
98 || !gpiod_get_value(wdev->pdata.gpio_wakeup)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Negative error codes from gpiod_get_value() should be treated as error.
99 dev_warn(wdev->dev, "unexpected wake-up indication\n");
100 return -EIO;
101 }
102 return 0;
103 }
#10 and #11
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xc2'
drivers/staging/wfx/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",
^
Can we output non-ascii to dmesg? (I didn't add this Smatch check so I
don't really know the answer).
236 body->data.rx_stats.current_temp);
#12
drivers/staging/wfx/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'
20 static int wfx_get_hw_rate(struct wfx_dev *wdev,
21 const struct ieee80211_tx_rate *rate)
22 {
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;
This code assumes that "rate->idx" can be all sort of invalid values
including negatives but it doesn't cap it against:
if (rate->idx >= band->n_bitrates)
return -1;
38 }
If you you read all the way down to the second end of the email then you
are a true hero. regards again,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
prev parent reply other threads:[~2020-10-08 13:15 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-07 10:19 [PATCH 0/7] wfx: move out from the staging area Jerome Pouiller
2020-10-07 10:19 ` [PATCH 1/7] staging: wfx: fix handling of MMIC error Jerome Pouiller
2020-10-07 10:19 ` [PATCH 2/7] staging: wfx: remove remaining code of 'secure link' feature Jerome Pouiller
2020-10-07 10:19 ` [PATCH 3/7] staging: wfx: fix BA sessions for older firmwares Jerome Pouiller
2020-10-07 10:19 ` [PATCH 4/7] staging: wfx: fix QoS priority for slow buses Jerome Pouiller
2020-10-07 10:19 ` [PATCH 5/7] staging: wfx: update copyrights dates Jerome Pouiller
2020-10-07 10:19 ` [PATCH 6/7] dt-bindings: staging: wfx: silabs,wfx yaml conversion Jerome Pouiller
2020-10-07 10:19 ` [PATCH 7/7] wfx: move out from the staging area Jerome Pouiller
2020-10-07 10:55 ` [PATCH 0/7] " Greg Kroah-Hartman
2020-10-08 7:30 ` Kalle Valo
2020-10-08 9:50 ` Kalle Valo
2020-10-08 10:10 ` Jérôme Pouiller
2020-10-08 11:00 ` Kalle Valo
2020-10-08 13:13 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201008131320.GA1042@kadam \
--to=dan.carpenter@oracle.com \
--cc=Jerome.Pouiller@silabs.com \
--cc=davem@davemloft.net \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).