drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced

drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced

[Fabio M. De Francesco]

Coccinelle detected that fw is NULL but dereferenced.

static int gb_bootrom_get_firmware(struct gb_operation *op)
{
/* lines of code */
        if (!fw) {
                dev_err(dev, "%s: firmware not available\n", __func__);
                ret = -EINVAL;
                goto unlock;
        }
/* lines of code */
unlock:
        unlock:
        mutex_unlock(&bootrom->mutex);

queue_work:
        /* Refresh timeout */
        if (!ret && (offset + size == fw->size))    <--- here
	next_request = NEXT_REQ_READY_TO_BOOT;
/* lines of code */
}

I really don't know if the following change may break something else:

        if(!ret && fw && (offset + size == fw->size))
	next_request = NEXT_REQ_READY_TO_BOOT;

So, I'll leave the problem to the maintainers or to other people who know how 
the driver is supposed to manage fw == NULL.

Thanks,

Fabio

Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced

[Viresh Kumar]

On 28-05-21, 01:39, Fabio M. De Francesco wrote:
> Coccinelle detected that fw is NULL but dereferenced.
> 
> static int gb_bootrom_get_firmware(struct gb_operation *op)
> {
> /* lines of code */
>         if (!fw) {
>                 dev_err(dev, "%s: firmware not available\n", __func__);
>                 ret = -EINVAL;

ret is set here.

>                 goto unlock;
>         }
> /* lines of code */
> unlock:
>         unlock:
>         mutex_unlock(&bootrom->mutex);
> 
> queue_work:
>         /* Refresh timeout */
>         if (!ret && (offset + size == fw->size))    <--- here

Since we are checking for !ret here, we will never access fw and this is a bug
in the tool and not the code here.

> 	next_request = NEXT_REQ_READY_TO_BOOT;
> /* lines of code */
> }
> 
> I really don't know if the following change may break something else:
> 
>         if(!ret && fw && (offset + size == fw->size))
> 	next_request = NEXT_REQ_READY_TO_BOOT;
> 
> So, I'll leave the problem to the maintainers or to other people who know how 
> the driver is supposed to manage fw == NULL.
> 
> Thanks,
> 
> Fabio
> 

-- 
viresh

Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced

[Fabio M. De Francesco]

On Friday, May 28, 2021 5:33:01 AM CEST Viresh Kumar wrote:
> On 28-05-21, 01:39, Fabio M. De Francesco wrote:
> > Coccinelle detected that fw is NULL but dereferenced.
> > 
> > static int gb_bootrom_get_firmware(struct gb_operation *op)
> > {
> > /* lines of code */
> > 
> >         if (!fw) {
> >         
> >                 dev_err(dev, "%s: firmware not available\n", __func__);
> >                 ret = -EINVAL;
> 
> ret is set here.
>
Oh sorry. I entirely skipped that "ret = -EINVAL". Another case where one 
should avoid blindly trusting the output of static analyzers without looking 
carefully at the whole context ... :(

Unfortunately, Coccinelle has a high false positive rate. Just yesterday I ran 
it against the entire driver / staging and more than 80% of the warnings / 
errors weren't true.

It takes so long to distinguish the fake from the real that I wonder if it's 
worth it to run the (slow) Coccinelle, wait for the output messages and verify 
their veracity.

Thanks,

Fabio
> 
> >                 goto unlock;
> >         
> >         }
> > 
> > /* lines of code */
> > 
> > unlock:
> >         unlock:
> >         mutex_unlock(&bootrom->mutex);
> > 
> > queue_work:
> >         /* Refresh timeout */
> >         if (!ret && (offset + size == fw->size))    <--- here
> 
> Since we are checking for !ret here, we will never access fw and this is a 
bug
> in the tool and not the code here.
> 
> > 	next_request = NEXT_REQ_READY_TO_BOOT;
> > 
> > /* lines of code */
> > }
> > 
> > I really don't know if the following change may break something else:
> >         if(!ret && fw && (offset + size == fw->size))
> > 	
> > 	next_request = NEXT_REQ_READY_TO_BOOT;
> > 
> > So, I'll leave the problem to the maintainers or to other people who know 
how
> > the driver is supposed to manage fw == NULL.
> > 
> > Thanks,
> > 
> > Fabio

Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced

[Dan Carpenter]

On Fri, May 28, 2021 at 01:39:14AM +0200, Fabio M. De Francesco wrote:
> Coccinelle detected that fw is NULL but dereferenced.
> 
> static int gb_bootrom_get_firmware(struct gb_operation *op)
> {
> /* lines of code */
>         if (!fw) {
>                 dev_err(dev, "%s: firmware not available\n", __func__);
>                 ret = -EINVAL;
>                 goto unlock;
>         }
> /* lines of code */
> unlock:
>         unlock:
>         mutex_unlock(&bootrom->mutex);
> 
> queue_work:
>         /* Refresh timeout */
>         if (!ret && (offset + size == fw->size))    <--- here
              ^^^
False positive.  The "!ret" check already ensures that "fw" is non-NULL.

regards,
dan carpenter

Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced

[Dan Carpenter]

On Fri, May 28, 2021 at 10:03:52AM +0200, Fabio M. De Francesco wrote:
> On Friday, May 28, 2021 5:33:01 AM CEST Viresh Kumar wrote:
> > On 28-05-21, 01:39, Fabio M. De Francesco wrote:
> > > Coccinelle detected that fw is NULL but dereferenced.
> > > 
> > > static int gb_bootrom_get_firmware(struct gb_operation *op)
> > > {
> > > /* lines of code */
> > > 
> > >         if (!fw) {
> > >         
> > >                 dev_err(dev, "%s: firmware not available\n", __func__);
> > >                 ret = -EINVAL;
> > 
> > ret is set here.
> >
> Oh sorry. I entirely skipped that "ret = -EINVAL". Another case where one 
> should avoid blindly trusting the output of static analyzers without looking 
> carefully at the whole context ... :(
> 
> Unfortunately, Coccinelle has a high false positive rate. Just yesterday I ran 
> it against the entire driver / staging and more than 80% of the warnings / 
> errors weren't true.
> 
> It takes so long to distinguish the fake from the real that I wonder if it's 
> worth it to run the (slow) Coccinelle, wait for the output messages and verify 
> their veracity.

With static checkers, we are always targetting 100% false positive rate
because kernel devs fix the real bugs.

Smatch tracks the relationship between "ret" and "fw" so it knows that
"fw" is non-NULL and you could verify that by applying this diff and
running `smatch_scripts/kchecker drivers/staging/greybus/bootrom.c`

diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
index a8efb86de140..9976d1bac839 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -238,6 +238,7 @@ static int gb_bootrom_firmware_size_request(struct gb_operation *op)
 	return ret;
 }
 
+#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
 static int gb_bootrom_get_firmware(struct gb_operation *op)
 {
 	struct gb_bootrom *bootrom = gb_connection_get_data(op->connection);
@@ -298,6 +299,8 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
 
 queue_work:
 	/* Refresh timeout */
+	if (!ret)
+		__smatch_implied(fw);
 	if (!ret && (offset + size == fw->size))
 		next_request = NEXT_REQ_READY_TO_BOOT;
 	else

But this test for for potential NULL dereference has a very high false
positive rate so I seldom look at the output for it.  The problem is
that GCC used to complain about uninitialized variables and GCC had a
high false positive rate.  So just to make GCC be quiet, people would do
"fw = NULL;" when the code was too complicated for GCC to understand.
Then all the complicated code stopped being a GCC uninitialized variable
warning and became a static checker warning of dereferencing a NULL
pointer.  It's all the most complicated code to parse which does this
and so that's obviously going to be tricky.  You often need to know
outside context about how the function is called.

And it's tricky, not only for static checkers but also for humans as
well.  So when I'd review it, most of the time I couldn't really figure
out if it's a real bug so I'd have to just leave it.  #frustrating

regards,
dan carpenter