Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote:

> Quoting Anjelique Melendez (2022-01-20 16:25:26)
> >
> > On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
> > > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
> > >
> > >> From: David Collins <collinsd@codeaurora.org>
> > >>
> > >> Add a null check for the pwrkey->data pointer after it is assigned
> > >> in pm8941_pwrkey_probe().  This avoids a potential null pointer
> > >> dereference when pwrkey->data->has_pon_pbs is accessed later in
> > >> the probe function.
> > >>
> > >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> > >> Signed-off-by: David Collins <collinsd@codeaurora.org>
> > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> > >> ---
> > >>  drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> > >>  1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> > >> index 0ce00736e695..ac08ed025802 100644
> > >> --- a/drivers/input/misc/pm8941-pwrkey.c
> > >> +++ b/drivers/input/misc/pm8941-pwrkey.c
> > >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> > >>
> > >>      pwrkey->dev = &pdev->dev;
> > >>      pwrkey->data = of_device_get_match_data(&pdev->dev);
> > >> +    if (!pwrkey->data) {
> > > The only way this can happen is if you add a new compatible and forget
> > > to specify data and when that happens you will get a print in the log
> > > somewhere, which once you realize that you don't have your pwrkey you
> > > might be able to find among all the other prints.
> > >
> > > If you instead don't NULL check this pointer you will get a large splat
> > > in the log, with callstack and all, immediately hinting you that
> > > pwrkey->data is NULL.
> > >
> > >
> > > In other words, there's already a print, a much larger print and I don't
> > > think there's value in handling this mistake gracefully.
> > >
> > > Regards,
> > > Bjorn
> >
> >
> > We would like to the null pointer check in place to avoid static analysis
> >
> > warnings that can be easily fixed.
> >
> 
> Many drivers check that their device_get_match_data() returns a valid
> pointer. I'd like to see that API used in addition to checking the
> return value for NULL so that we can keep the static analysis tools
> happy. Yes it's an impossible case assuming the driver writer didn't
> mess up but it shuts SA up and we don't really have a better solution
> to tell tools that device_get_match_data() can't return NULL.

I'm not saying that device_get_match_data() can't return NULL, I'm
saying that in the very specific cases that it would return NULL it's
useful to have a kernel panic - as that's a much faster way to figure
out that something is wrong.

And as a timely coincidence I tried to introduce such a check last week,
for a case where the cause of the dereference issue definitely wasn't
obvious to me and Greg among others told me that it's wrong:

https://lore.kernel.org/linux-arm-msm/20220118185612.2067031-2-bjorn.andersson@linaro.org/


And just to be clear, I don't care about this case in particular, but I
fear that we have a lot of SA warnings to shut up throughout the kernel.

Regards,
Bjorn