Messages in this thread |  | | Date | Thu, 2 Apr 2020 15:42:23 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 01/32] staging: wfx: add sanity checks to hif_join() |
| |
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
|  |