Input: cyapa - add gen5 trackpad device basic functions support

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: 2015?1?21? 21:20
To: Dudley Du
Cc: [email protected]
Subject: re: Input: cyapa - add gen5 trackpad device basic functions support

Hello Dudley Du,

This is a semi-automatic email about new static checker warnings.

The patch 6972a859601a: "Input: cyapa - add gen5 trackpad device basic functions support" from Jan 17, 2015, leads to the following Smatch complaint:

drivers/input/mouse/cyapa_gen5.c:2564 cyapa_gen5_irq_cmd_handler()
warn: variable dereferenced before check 'gen5_pip->resp_len' (see line
2559)

drivers/input/mouse/cyapa_gen5.c
2558 */

  2559length = *gen5_pip->resp_len;
                                         ^^^^^^^^^^^^^^^^^^^

Patch introduces a dereference.

2560cyapa_empty_pip_output_data(cyapa,
2561gen5_pip->resp_data,
2562&length,
2563gen5_pip->resp_sort_func);

  2564if (gen5_pip->resp_len && length != 0) {
                                    ^^^^^^^^^^^^^^^^^^

Patch introduces a check.

  2565*gen5_pip->resp_len = length;
  2566atomic_dec(&gen5_pip->cmd_issued);

regards,
dan carpenter

Hi Carpenter,

Thanks for the information.
Could you indicate the tool and the command to generate this warning message?

In the code,
1) length = *gen5_pip->resp_len to get the expected response length,
2) then cyapa_empty_pip_output_data() try to polling the response data with the expect length,
3) at last, the length stored the real response length that it got in the polling function.
4) if the real response length is not 0, then assign the real response to replace the excpeted response length.

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: 2015?1?22? 16:31
To: Dudley Du
Cc: [email protected]
Subject: Re: Input: cyapa - add gen5 trackpad device basic functions support

Quoted 5 lines · Dudley Du

Hi Carpenter,

Thanks for the information.
Could you indicate the tool and the command to generate this warning message?

This a Smatch warning.

Quoted 4 lines

In the code,
1) length = *gen5_pip->resp_len to get the expected response length,
2) then cyapa_empty_pip_output_data() try to polling the response data with the

expect length,

Quoted 1 line

3) at last, the length stored the real response length that it got in the polling

function.

Quoted 1 line

4) if the real response length is not 0, then assign the real response to replace the

excpeted response length.

The error message is that we dereferenced gen5_pip->resp_len before we checked whether it was NULL. I believe you are saying that cyapa_empty_pip_output_data() can modify "gen5_pip->resp_len" so we need to do the check for NULL.

The problem is that I don't see where "gen5_pip->resp_len" gets changed inside cyapa_empty_pip_output_data(). Smatch is supposed to do cross function analysis and detect this but it doesn't see the modification either. I have been working on this code recently in Smatch so Smatch may be buggy.

Can you help me out here so I can improve the tools?

According to Smatch "gen5_pip->resp_len" is set in two different functions.

$ smdb where cyapa_gen5_cmd_states resp_len
drivers/input/mouse/cyapa_gen5.c | cyapa_gen5_initialize          | (struct
cyapa_gen5_cmd_states)->resp_len | 0
drivers/input/mouse/cyapa_gen5.c | cyapa_i2c_pip_cmd_irq_sync     | (struct
cyapa_gen5_cmd_states)->resp_len | 0,4096-2117777777777777777

Also I looked at the call tree to see if cyapa_empty_pip_output_data calls cyapa_i2c_pip_cmd_irq_sync but it doesn't.

$ smdb call_tree cyapa_i2c_pip_cmd_irq_sync | grep
cyapa_empty_pip_output_data

But, uh.. it's been years since I tried looking at the call_tree code so I have no idea if it works...

regards,
dan carpenter