Re: [bug report] Input: add st-keyscan driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux