netdev - Re: [PATCH 2/8] staging: wfx: check memory allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 10 Oct 2020 16:18:11 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jérôme Pouiller <jerome.pouiller@...abs.com>
Cc:     Kalle Valo <kvalo@...eaurora.org>, devel@...verdev.osuosl.org,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH 2/8] staging: wfx: check memory allocation

On Sat, Oct 10, 2020 at 02:07:13PM +0200, Jérôme Pouiller wrote:
> On Friday 9 October 2020 20:51:01 CEST Kalle Valo wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > Jerome Pouiller <Jerome.Pouiller@...abs.com> writes:
> > 
> > > From: Jérôme Pouiller <jerome.pouiller@...abs.com>
> > >
> > > Smatch complains:
> > >
> > >    main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
> > >    227          tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > >    228          ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > >                                          ^^^^^^^
> > >    229          kfree(tmp_buf);
> > >
> > > Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
> > > ---
> > >  drivers/staging/wfx/main.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > index df11c091e094..a8dc2c033410 100644
> > > --- a/drivers/staging/wfx/main.c
> > > +++ b/drivers/staging/wfx/main.c
> > > @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > >       if (ret) {
> > >               dev_err(wdev->dev, "can't load PDS file %s\n",
> > >                       wdev->pdata.file_pds);
> > > -             return ret;
> > > +             goto err1;
> > >       }
> > >       tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > +     if (!tmp_buf) {
> > > +             ret = -ENOMEM;
> > > +             goto err2;
> > > +     }
> > >       ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > >       kfree(tmp_buf);
> > > +err2:
> > >       release_firmware(pds);
> > > +err1:
> > >       return ret;
> > >  }
> > 
> > A minor style issue but using more descriptive error labels make the
> > code more readable and maintainable, especially in a bigger function.
> > For example, err2 could be called err_release_firmware.
> > 
> > And actually err1 could be removed and the goto replaced with just
> > "return ret;". Then err2 could be renamed to a simple err.
> 
> It was the case in the initial code. However, I have preferred to not
> mix 'return' and 'goto' inside the same function. Probably a matter of
> taste.
>

Ideally you can read a function from top to bottom and understand with
out skipping around.  Imagine if novels were written like that "goto
bottom_of_page;" but then at the bottom it just said "Just kidding".
"return ret;" is more readable than "goto err;"

These sorts of rules where "there is only one return per function" are
meant to make people think about cleanup before returning.  But most of
my work is in error handling code and it doesn't help.  If people don't
think about cleanup, changing the style won't make them start thinking
about it.  There was one driver which was written with locked code
indented one tab and the inventor of that style still introduced a
locking bug in his code.

	spin_lock(); {
		frob();
		frob();
		if (ret)
			return ret;  // <-- forgot to unlock;
		frob();
	} spin_unlock();

Btw, I have created a new Smatch check to find unwind bugs.  It's called
check_unwind.c and it's easy to add new alloc/free pairings to that
code.  This is the best way to prevent unwind bugs.  The style changes
don't make a measurable difference in real life and they make the code
messy.

And GW-BASIC label names are a pox upon the earth.

regards,
dan carpenter