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

[Anjelique Melendez <quic_amelende@quicinc.com>]

On 1/25/2022 10:37 AM, Bjorn Andersson wrote:
> 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

I will drop this patch for now so that further discussion can be had. Can send as a separate patch
later.