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
