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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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