Re: [bug report] Input: add st-keyscan driver
From: Gabriel FERNANDEZ
Date: Wed Jan 30 2019 - 03:59:02 EST
Hi all,
I prefer to fix it. I guess people used their own correction.
I will send you a fix asap.
Best Regards
Gabriel
On 1/27/19 2:20 AM, Ken Sloat wrote:
> On Sat, Jan 26, 2019 at 5:15 PM Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx> wrote:
>> On Sat, Jan 26, 2019 at 1:25 PM Ken Sloat
>> <ken.sloat@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>>> On Tue, Jan 22, 2019 at 1:53 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>>>> Hello Gabriel FERNANDEZ,
>>> Hello Dan,
>>>
>>> I have added CCs for the maintainers as well as Gabriel Fernandez as
>>> currently you just have the linux-input mailing list
>>>
>>>> The patch 062589b13991: "Input: add st-keyscan driver" from Apr 12,
>>>> 2014, leads to the following static checker warning:
>>>>
>>>> drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
>>>> error: potential zalloc NULL dereference: 'keypad_data->input_dev'
>>>>
>>>> drivers/input/keyboard/st-keyscan.c
>>>> 125 static int keyscan_probe(struct platform_device *pdev)
>>>> 126 {
>>>> 127 struct st_keyscan *keypad_data;
>>>> 128 struct input_dev *input_dev;
>>>> 129 struct resource *res;
>>>> 130 int error;
>>>> 131
>>>> 132 if (!pdev->dev.of_node) {
>>>> 133 dev_err(&pdev->dev, "no DT data present\n");
>>>> 134 return -EINVAL;
>>>> 135 }
>>>> 136
>>>> 137 keypad_data = devm_kzalloc(&pdev->dev, sizeof(*keypad_data),
>>>> 138 GFP_KERNEL);
>>>> 139 if (!keypad_data)
>>>> 140 return -ENOMEM;
>>>> 141
>>>> 142 input_dev = devm_input_allocate_device(&pdev->dev);
>>>> 143 if (!input_dev) {
>>>> 144 dev_err(&pdev->dev, "failed to allocate the input device\n");
>>>> 145 return -ENOMEM;
>>>> 146 }
>>>> 147
>>>> 148 input_dev->name = pdev->name;
>>>> 149 input_dev->phys = "keyscan-keys/input0";
>>>> 150 input_dev->dev.parent = &pdev->dev;
>>>> 151 input_dev->open = keyscan_open;
>>>> 152 input_dev->close = keyscan_close;
>>>> 153
>>>> 154 input_dev->id.bustype = BUS_HOST;
>>>> 155
>>>> --> 156 error = keypad_matrix_key_parse_dt(keypad_data);
>>>> ^^^^^^^^^^^
>>> I agree with you this would be a problem
>>> to clarify the NULL derefence would occur here within keypad_matrix_key_parse_dt
>>>
>>> struct device *dev = keypad_data->input_dev->dev.parent;
>>>
>>>> This assumes we have set "keypad_data->input_dev = input_dev;" but we
>>>> don't do that until...
>>>>
>>>> 157 if (error)
>>>> 158 return error;
>>>> 159
>>>> 160 error = matrix_keypad_build_keymap(NULL, NULL,
>>>> 161 keypad_data->n_rows,
>>>> 162 keypad_data->n_cols,
>>>> 163 NULL, input_dev);
>>>> 164 if (error) {
>>>> 165 dev_err(&pdev->dev, "failed to build keymap\n");
>>>> 166 return error;
>>>> 167 }
>>>> 168
>>>> 169 input_set_drvdata(input_dev, keypad_data);
>>>> 170
>>>> 171 keypad_data->input_dev = input_dev;
>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> this line here. This driver has never worked and it was included almost
>>>> five years ago. Is it worth fixing?
>>>>
>>>> 172
>>>> 173 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> 174 keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
>>>> 175 if (IS_ERR(keypad_data->base))
>>>> 176 return PTR_ERR(keypad_data->base);
>>>> 177
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>> Here is the interesting thing, I was looking on patchwork, and several
>>> of the patches including what appears to be the latest actually set
>>> "keypad_data->input_dev = input_dev" before calling
>>> "keypad_matrix_key_parse_dt"
>>>
>>> From v4 on patchwork
>>> + if (IS_ERR(keypad_data->clk)) {
>>> + dev_err(&pdev->dev, "cannot get clock");
>>> + return PTR_ERR(keypad_data->clk);
>>> + }
>>> +
>>> + keypad_data->input_dev = input_dev;
>>> +
>>> + input_dev->name = pdev->name;
>>> + input_dev->phys = "keyscan-keys/input0";
>>> + input_dev->dev.parent = &pdev->dev;
>>> + input_dev->open = keyscan_open;
>>> + input_dev->close = keyscan_close;
>>> +
>>> + input_dev->id.bustype = BUS_HOST;
>>> +
>>> + error = keypad_matrix_key_parse_dt(keypad_data);
>>>
>>> According to patchwork, these aren't listed as accepted, so I'm not
>>> sure where the exact accepted patch came from. Looking at the commit
>>> log, it looks like the issue you showed above was made in the original
>>> commit 062589b1399176a9c14bc68e16169f40439d658c so I'm not quite sure
>>> what is going on here. Maybe the maintainer can chime in with the
>>> original patch/mailing list discussion on this. For reference, I've
>>> added the patchwork links below
>>> https://patchwork.kernel.org/patch/3854341/
>>> https://patchwork.kernel.org/patch/3968891/
>>> https://patchwork.kernel.org/patch/3969991/
>> It may very well be that I messed up when applying the patch. I guess
>> whatever platform that is using the driver has not attempted to update
>> their kernel since then.
>>
>> Thanks.
>>
>> --
>> Dmitry
> Hi Dmitry,
>
> Thanks for the quick response. Yes I was just looking at the other
> mailing lists patchwork and while comments were missing on the
> linux-input list for the v4 patch, there was a discussion on the
> regular kernel mailing list:
> https://lore.kernel.org/patchwork/patch/455450/#630445
>
> It looks like you told him he didn't need to submit a v5 but generated
> one based on the changes suggested in the discussion:
>
> [begin quote]
> On Wed, Apr 16, 2014 at 10:49:29AM +0200, Gabriel Fernandez wrote:
>> On 13 April 2014 07:10, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
>>> Does the version of the patch below still work for you?
>>>
>> Yes it's was tested on b2000 and b2089 sti boards.
>>
>>> Thanks.
>>>
>>> --
>>> Dmitry
>>>
>> Thanks for yours remarks, i will prepare a v5 versions.
>
> If the version I sent to you works then you do not need to prepare v5,
> I'll just apply what I have.
>
> Thanks!
> [end quote]
>
> So I guess Dan's original question remains, should this be fixed
> considering it's so old and apparently nobody could possibly be using
> it in the kernel since it doesn't work (in which case the driver
> should probably be dropped) or should we fix it?
>
> Thanks,
> Ken Sloat