Re: HID: wacom: check for wacom->shared before following the pointer

On Mar 19 2015 or thereabouts, Ping Cheng wrote:

On Thu, Mar 19, 2015 at 9:06 AM, Benjamin Tissoires
[off-list ref] wrote:

quoted

Hi Dan,

On Mar 19 2015 or thereabouts, Dan Carpenter wrote:

quoted

Hello Benjamin Tissoires,

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

The patch e2c7d8877e5c: "HID: wacom: check for wacom->shared before
following the pointer" from Mar 5, 2015, leads to the following
Smatch complaint:

drivers/hid/wacom_wac.c:602 wacom_intuos_inout()
       error: we previously assumed 'wacom->shared' could be null (see line 584)

drivers/hid/wacom_wac.c
   583
   584                if (wacom->shared) {

In the original code we checked "if (features->quirks & WACOM_QUIRK_MULTI_INPUT)"
which is ensures that "wacom->shared" is non-NULL.

   585                        wacom->shared->stylus_in_proximity = true;
   586
   587                        if (wacom->shared->touch_down)
   588                                return 1;
   589                }
   590
   591                /* in Range while exiting */
   592                if (((data[1] & 0xfe) == 0x20) && wacom->reporting_data) {
   593                        input_report_key(input, BTN_TOUCH, 0);
   594                        input_report_abs(input, ABS_PRESSURE, 0);
   595                        input_report_abs(input, ABS_DISTANCE, wacom->features.distance_max);
   596                        return 2;
   597                }
   598
   599                /* Exit report */
   600                if ((data[1] & 0xfe) == 0x80) {
   601                        if (features->quirks & WACOM_QUIRK_MULTI_INPUT)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We still check for that here.  Smatch is confused.

   602                                wacom->shared->stylus_in_proximity = false;
                                ^^^^^^^^^^^^^
This is not a bug, but change the previous change to
"if (wacom->shared)" would make the code more consistent.

Yep, I agree. That's for these cases that I preferred having a test
against wacom->shared not null rather than (features->quirks &
WACOM_QUIRK_MULTI_INPUT).

quoted

   603                        wacom->reporting_data = false;
   604

[ snip ]

  1072  static int wacom_24hdt_irq(struct wacom_wac *wacom)
  1073  {
  1074          struct input_dev *input = wacom->input;
  1075          unsigned char *data = wacom->data;
  1076          int i;
  1077          int current_num_contacts = data[61];
  1078          int contacts_to_send = 0;
  1079          int num_contacts_left = 4; /* maximum contacts per packet */
  1080          int byte_per_packet = WACOM_BYTES_PER_24HDT_PACKET;
  1081          int y_offset = 2;
  1082          static int contact_with_no_pen_down_count = 0;
  1083
  1084          if (wacom->features.type == WACOM_27QHDT) {
  1085                  current_num_contacts = data[63];
  1086                  num_contacts_left = 10;
  1087                  byte_per_packet = WACOM_BYTES_PER_QHDTHID_PACKET;
  1088                  y_offset = 0;
  1089          }
  1090
  1091          /*
  1092           * First packet resets the counter since only the first
  1093           * packet in series will have non-zero current_num_contacts.
  1094           */
  1095          if (current_num_contacts) {
  1096                  wacom->num_contacts_left = current_num_contacts;
  1097                  contact_with_no_pen_down_count = 0;
  1098          }
  1099
  1100          contacts_to_send = min(num_contacts_left, wacom->num_contacts_left);
  1101
  1102          for (i = 0; i < contacts_to_send; i++) {
  1103                  int offset = (byte_per_packet * i) + 1;
  1104                  bool touch = (data[offset] & 0x1) && !wacom->shared->stylus_in_proximity;
                                                              ^^^^^^^^^^^^^
I assume this hardware is always quirky so this won't cause a NULL
deref?

Yes. 24hdt has the quirk WACOM_QUIRK_MULTI_INPUT, so wacom->shared can
not be null. I wonder what we could put in the code to make static
checkers happy...

I am working on a solution. Patches will be submitted soon.

Thanks Ping. That lower me my workload and is very appreciated.
 

Thank you Dan and Benjamin.

Thanks dan for your answer (not in this thread, but heh...)

Cheers,
Benjamin