From: "Jérôme Pouiller" <Jerome.Pouiller@silabs.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
"linux-kernel@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 01/32] staging: wfx: add sanity checks to hif_join()
Date: Thu, 2 Apr 2020 16:14:16 +0000 [thread overview]
Message-ID: <2026476.QLiXXEGFCf@pc-42> (raw)
In-Reply-To: <20200402124223.GQ2001@kadam>
On Thursday 2 April 2020 14:42:23 CEST Dan Carpenter wrote:
> On Wed, Apr 01, 2020 at 01:03:34PM +0200, Jerome Pouiller wrote:
> > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> >
> > Add a few check on start of hif_join().
> >
> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> > drivers/staging/wfx/hif_tx.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> > index 77bca43aca42..445906035e9d 100644
> > --- a/drivers/staging/wfx/hif_tx.c
> > +++ b/drivers/staging/wfx/hif_tx.c
> > @@ -297,6 +297,8 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
> > struct hif_req_join *body = wfx_alloc_hif(sizeof(*body), &hif);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We've got an allocation here. It's a mistake to put the allocation in
> the declaration block because you're going to forget to check for
> failure.
arf... this remark also applies to all functions of hif_tx.c. This
issue has already been reported. I will send a patch that solve that in
one batch.
> > WARN_ON(!conf->basic_rates);
> > + WARN_ON(sizeof(body->ssid) < ssidlen);
>
> Put the variable on the left. WARN_ON(ssidlen > sizeof(body->ssid)).
> I'm not a big fan of adding this sort of debug code, just audit the
> callers to see if it's possible or not.
My personal opinion is these checks does not replace the audit of the
callers. It mainly provides a kind of documentation for the reader
("not supported, please check the callers"). It is especially true when
it is an internal API and there is only one caller.
> I have audited the caller for you, and I believe that this condition
> *is possible* so we need to return -EINVAL in this situation to prevent
> memory corruption.
>
> if (ssidlen > sizeof(body->ssid))
> return -EINVAL;
In this case, I think the problem will also impact wfx_do_join() (the
only caller of hif_join()):
514 u8 ssid[IEEE80211_MAX_SSID_LEN];
[...]
538 if (!conf->ibss_joined)
539 ssidie = ieee80211_bss_get_ie(bss, WLAN_EID_SSID);
540 if (ssidie) {
541 ssidlen = ssidie[1];
542 memcpy(ssid, &ssidie[2], ssidie[1]);
543 }
[...]
554 ret = hif_join(wvif, conf, wvif->channel, ssid, ssidlen);
Does data returned by ieee80211_bss_get_ie() could be bigger than
IEEE80211_MAX_SSID_LEN? Not sure. I am going to add a check in
wfx_do_join(), just in case.
--
Jérôme Pouiller
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-04-02 16:14 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-01 11:03 [PATCH 00/32] staging: wfx: rework the Tx queue Jerome Pouiller
2020-04-01 11:03 ` [PATCH 01/32] staging: wfx: add sanity checks to hif_join() Jerome Pouiller
2020-04-02 12:42 ` Dan Carpenter
2020-04-02 16:14 ` Jérôme Pouiller [this message]
2020-04-01 11:03 ` [PATCH 02/32] staging: wfx: do not stop mac80211 queueing during tx_policy upload Jerome Pouiller
2020-04-01 11:03 ` [PATCH 03/32] staging: wfx: take advantage of ieee80211_{stop/start}_queues Jerome Pouiller
2020-04-01 11:03 ` [PATCH 04/32] staging: wfx: remove "burst" mechanism Jerome Pouiller
2020-04-02 13:05 ` Dan Carpenter
2020-04-02 14:47 ` Jérôme Pouiller
2020-04-01 11:03 ` [PATCH 05/32] staging: wfx: uniformize queue_id retrieval Jerome Pouiller
2020-04-01 11:03 ` [PATCH 06/32] staging: wfx: drop useless queue_id field Jerome Pouiller
2020-04-01 11:03 ` [PATCH 07/32] staging: wfx: avoid useless wake_up Jerome Pouiller
2020-04-01 11:03 ` [PATCH 08/32] staging: wfx: simplify hif_handle_tx_data() Jerome Pouiller
2020-04-02 13:13 ` Dan Carpenter
2020-04-02 14:44 ` Jérôme Pouiller
2020-04-01 11:03 ` [PATCH 09/32] staging: wfx: simplify wfx_tx_queues_empty() Jerome Pouiller
2020-04-01 11:03 ` [PATCH 10/32] staging: wfx: drop unused argument in wfx_get_prio_queue() Jerome Pouiller
2020-04-01 11:03 ` [PATCH 11/32] staging: wfx: simplify wfx_tx_queue_mask_get() Jerome Pouiller
2020-04-01 11:03 ` [PATCH 12/32] staging: wfx: drop useless sta_asleep_mask Jerome Pouiller
2020-04-01 11:03 ` [PATCH 13/32] staging: wfx: drop argument tx_allowed_mask since it is constant now Jerome Pouiller
2020-04-01 11:03 ` [PATCH 14/32] staging: wfx: do not use link_map_cache to track CAB Jerome Pouiller
2020-04-01 11:03 ` [PATCH 15/32] staging: wfx: drop useless link_map_cache Jerome Pouiller
2020-04-01 11:03 ` [PATCH 16/32] staging: wfx: do not rely anymore on link_id to choose packet in queue Jerome Pouiller
2020-04-01 11:03 ` [PATCH 17/32] staging: wfx: drop unused link_id field Jerome Pouiller
2020-04-01 11:03 ` [PATCH 18/32] staging: wfx: drop unused raw_link_id field Jerome Pouiller
2020-04-01 11:03 ` [PATCH 19/32] staging: wfx: rename wfx_tx_get_raw_link_id() Jerome Pouiller
2020-04-01 11:03 ` [PATCH 20/32] staging: wfx: replace wfx_tx_queues_get_after_dtim() by wfx_tx_queues_has_cab() Jerome Pouiller
2020-04-01 11:03 ` [PATCH 21/32] staging: wfx: introduce a counter of pending frames Jerome Pouiller
2020-04-01 11:03 ` [PATCH 22/32] staging: wfx: change the way to choose frame to send Jerome Pouiller
2020-04-01 11:03 ` [PATCH 23/32] staging: wfx: drop now useless field edca_params Jerome Pouiller
2020-04-01 11:03 ` [PATCH 24/32] staging: wfx: drop struct wfx_queue_stats Jerome Pouiller
2020-04-01 11:03 ` [PATCH 25/32] staging: wfx: simplify usage of wfx_tx_queues_put() Jerome Pouiller
2020-04-01 11:03 ` [PATCH 26/32] staging: wfx: improve interface between data_tx.c and queue.c Jerome Pouiller
2020-04-01 11:04 ` [PATCH 27/32] staging: wfx: relocate wfx_skb_dtor() prior its callers Jerome Pouiller
2020-04-01 11:04 ` [PATCH 28/32] staging: wfx: repair wfx_flush() Jerome Pouiller
2020-04-01 11:04 ` [PATCH 29/32] staging: wfx: wfx_flush() did not ensure that frames are processed Jerome Pouiller
2020-04-01 11:04 ` [PATCH 30/32] staging: wfx: fix potential deadlock in wfx_tx_flush() Jerome Pouiller
2020-04-01 11:04 ` [PATCH 31/32] staging: wfx: fix case where AP stop with CAB traffic pending Jerome Pouiller
2020-04-01 11:04 ` [PATCH 32/32] staging: wfx: remove hack about tx_rate policies Jerome Pouiller
2020-04-03 8:03 ` [PATCH 00/32] staging: wfx: rework the Tx queue Dan Carpenter
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=2026476.QLiXXEGFCf@pc-42 \
--to=jerome.pouiller@silabs.com \
--cc=dan.carpenter@oracle.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).