* drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced @ 2021-05-27 23:39 Fabio M. De Francesco 2021-05-28 3:33 ` Viresh Kumar 2021-05-28 10:29 ` Dan Carpenter 0 siblings, 2 replies; 5+ messages in thread From: Fabio M. De Francesco @ 2021-05-27 23:39 UTC (permalink / raw) To: linux-staging; +Cc: Viresh Kumar, Johan Hovold, Alex Elder, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced 2021-05-27 23:39 drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced Fabio M. De Francesco @ 2021-05-28 3:33 ` Viresh Kumar 2021-05-28 8:03 ` Fabio M. De Francesco 2021-05-28 10:29 ` Dan Carpenter 1 sibling, 1 reply; 5+ messages in thread From: Viresh Kumar @ 2021-05-28 3:33 UTC (permalink / raw) To: Fabio M. De Francesco Cc: linux-staging, Viresh Kumar, Johan Hovold, Alex Elder, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced 2021-05-28 3:33 ` Viresh Kumar @ 2021-05-28 8:03 ` Fabio M. De Francesco 2021-05-28 10:42 ` Dan Carpenter 0 siblings, 1 reply; 5+ messages in thread From: Fabio M. De Francesco @ 2021-05-28 8:03 UTC (permalink / raw) To: Viresh Kumar Cc: linux-staging, Viresh Kumar, Johan Hovold, Alex Elder, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced 2021-05-28 8:03 ` Fabio M. De Francesco @ 2021-05-28 10:42 ` Dan Carpenter 0 siblings, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2021-05-28 10:42 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Viresh Kumar, linux-staging, Viresh Kumar, Johan Hovold, Alex Elder, Greg Kroah-Hartman 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced 2021-05-27 23:39 drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced Fabio M. De Francesco 2021-05-28 3:33 ` Viresh Kumar @ 2021-05-28 10:29 ` Dan Carpenter 1 sibling, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2021-05-28 10:29 UTC (permalink / raw) To: Fabio M. De Francesco Cc: linux-staging, Viresh Kumar, Johan Hovold, Alex Elder, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-28 10:42 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-27 23:39 drivers/staging/greybus/bootrom.c: fw is NULL but dereferenced Fabio M. De Francesco 2021-05-28 3:33 ` Viresh Kumar 2021-05-28 8:03 ` Fabio M. De Francesco 2021-05-28 10:42 ` Dan Carpenter 2021-05-28 10:29 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.