Re: [PATCH 01/32] staging: wfx: add sanity checks to hif_join()

[Jérôme Pouiller]

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@xxxxxxxxxx>
> >
> > Add a few check on start of hif_join().
> >
> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> > ---
> >  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