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

[Dan Carpenter <dan.carpenter@oracle.com>]

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