linux-kernel - Re: staging: most: warning: ‘mbo’ may be used uninitialized in this function

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] [day] [month] [year] [list]

Message-ID: <20160318155732.GB2555@arch.fritz.box>
Date:	Fri, 18 Mar 2016 16:57:32 +0100
From:	Andrey Shvetsov <andrey.shvetsov.ml@...il.com>
To:	Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:	Christian Gromm <christian.gromm@...rochip.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	driverdevel <devel@...verdev.osuosl.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: staging: most: warning: ‘mbo’
 may be used uninitialized in this function

On Fri, Mar 18, 2016 at 01:41:19PM +0100, Geert Uytterhoeven wrote:
> On Fri, Mar 18, 2016 at 6:42 AM, Linux Kernel Mailing List
> <linux-kernel@...r.kernel.org> wrote:
> > Web:        https://git.kernel.org/torvalds/c/f45b0fba43f415f69982df743dfa9b5d1b57785e
> > Commit:     f45b0fba43f415f69982df743dfa9b5d1b57785e
> > Parent:     b3c9f3c56c41cbebe7804b48ba8e6e484509c2c0
> > Refname:    refs/heads/master
> > Author:     Christian Gromm <christian.gromm@...rochip.com>
> > AuthorDate: Tue Dec 22 10:53:06 2015 +0100
> > Committer:  Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > CommitDate: Sun Feb 7 17:34:58 2016 -0800
> >
> >     staging: most: remove stacked_mbo
> >
> >     This patch makes use of kfifo_peek and kfifo_skip, which renders the
> >     variable stacked_mbo useless. It is therefore removed.
> >
> >     Signed-off-by: Christian Gromm <christian.gromm@...rochip.com>
> >     Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > ---
> >  drivers/staging/most/aim-cdev/cdev.c | 16 +++-------------
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
> > index d9c3f56..0ee2f08 100644
> > --- a/drivers/staging/most/aim-cdev/cdev.c
> > +++ b/drivers/staging/most/aim-cdev/cdev.c
> 
> > @@ -249,11 +246,7 @@ aim_read(struct file *filp, char __user *buf, size_t count, loff_t *offset)
> >         struct aim_channel *c = filp->private_data;
> >
> >         mutex_lock(&c->io_mutex);
> > -       if (c->stacked_mbo) {
> > -               mbo = c->stacked_mbo;
> > -               goto start_copy;
> > -       }
> > -       while ((!kfifo_out(&c->fifo, &mbo, 1)) && (c->dev)) {
> > +       while (c->dev && !kfifo_peek(&c->fifo, &mbo)) {
> 
> drivers/staging/most/aim-cdev/cdev.c:241: warning: ‘mbo’ may be used
> uninitialized in this function
> 
> From looking at the code, it's not obvious to me if this is a false
> positive or not.
> Can it happen that mbo is not initialized fully, e.g. if less than sizeof(mbo)
> bytes have been read from the kfifo?
> 
mbo is not touched by the kfifo_peek in the case where (c->dev == NULL),
but since we protect the c->dev by the io_mutex it remains NULL until
the check after the while loop where we quit.

Looks like the analyzer suspects that c->dev may be changed to a valid
pointer before the second check.

So, it is false positive, but it is worth to initialize the mbo with the
NULL to get rid of the warning.

regards
andy