Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Jan 2022 10:37:09 -0800
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Anjelique Melendez <quic_amelende@quicinc.com>,
        dmitry.torokhov@gmail.com, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        collinsd@codeaurora.org, skakit@codeaurora.org
Subject: Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null
 pointer dereference
Message-ID: <YfBDVQeEsLv6Yj9g@ripper>
References: <20220120204132.17875-1-quic_amelende@quicinc.com>
 <20220120204132.17875-4-quic_amelende@quicinc.com>
 <YenpwnE3WrIEAOlm@ripper>
 <e2015c19-b73b-39a7-ba73-708b2c4552c7@quicinc.com>
 <CAE-0n50+1OU2yt2gihHHCEn-cE-CZuqa_U9W=xWCuYeCQdzExw@mail.gmail.com>
 <Ye8nmnQ3F4QcTIJs@ripper>
 <CAE-0n51bYEkvxu8z2gc_KUv0u+J2Esg0_3AiQRLyTaouNoa78g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAE-0n51bYEkvxu8z2gc_KUv0u+J2Esg0_3AiQRLyTaouNoa78g@mail.gmail.com>
Precedence: bulk

On Mon 24 Jan 17:55 PST 2022, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2022-01-24 14:26:34)
> > 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,
> 
> Indeed, I wasn't implying that you were saying that.
> 
> > 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.
> 
> I see it as more annoying, but maybe that's my workflow? When my kernel
> oopses I have to go back to a recovery kernel, which takes me a few more
> seconds to "repair" my device. If the driver only failed to probe then
> I'd probably be able to boot far enough to get networking and more
> easily replace my kernel with a working device. And I'd have userspace
> access so I could poke around and figure out why the driver failed to
> probe. Now obviously a big stacktrace would be helpful to know that it's
> the power key driver that's busted, but it's not like we're calling some
> internal API here. We're trying to probe a driver and if that oopses
> because the driver writer failed at their job then it's bad on them for
> writing a bad patch but also annoying for the integrator who has to deal
> with the mess they created. I'd rather have a half working system here
> vs. a totally broken one.

Forgot about your recovery cycle, on most of my boards I just load a new
kernel every boot, so there's no cost of recovering from a panic, it
might even save me some time if it crashes completely before userspace
starts consuming cycles.

My only concern is that this "sets" a quite fuzzy precedence. I don't
want us to just fix SA warnings all over the place, but I don't want it
to be inconvenient to work on the kernel...

Regards,
Bjorn