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
