| Message ID | 20200401110405.80282-2-Jerome.Pouiller@silabs.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | staging: wfx: rework the Tx queue | expand |
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. > > 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. 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; > + WARN(!conf->ibss_joined && !ssidlen, "joining an unknown BSS"); > body->infrastructure_bss_mode = !conf->ibss_joined; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Potential NULL dererefence because of the unchecked allocation. regards, dan carpenter
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.
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); WARN_ON(!conf->basic_rates); + WARN_ON(sizeof(body->ssid) < ssidlen); + WARN(!conf->ibss_joined && !ssidlen, "joining an unknown BSS"); body->infrastructure_bss_mode = !conf->ibss_joined; body->short_preamble = conf->use_short_preamble; if (channel && channel->flags & IEEE80211_CHAN_NO_IR)