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
[off-list ref] wrote:

quoted

On Sat, Jan 26, 2019 at 1:25 PM Ken Sloat
[off-list ref] wrote:

quoted

On Tue, Jan 22, 2019 at 1:53 PM Dan Carpenter [off-list ref] wrote:

quoted

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

quoted

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;

quoted

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:

quoted

On 13 April 2014 07:10, Dmitry Torokhov [off-list ref] wrote:

quoted

Does the version of the patch below still work for you?

Yes it's was tested on b2000 and b2089 sti boards.

quoted

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