platform/chrome: wilco_ec: Add support for raw commands in debugfs

Hello Nick Crews,

The patch 6d9f76dd4b35: "platform/chrome: wilco_ec: Add support for raw commands in debugfs" from Feb 8, 2019, leads to the following static checker warning:

drivers/platform/chrome/wilco_ec/debugfs.c:150 raw_write()
warn: 'ret - 3' negative one (off by one?)

drivers/platform/chrome/wilco_ec/debugfs.c

    119 static ssize_t raw_write(struct file *file, const char __user *user_buf,
    120                          size_t count, loff_t *ppos)
    121 {
    122         char *buf = debug_info->formatted_data;
    123         struct wilco_ec_message msg;
    124         u8 request_data[TYPE_AND_DATA_SIZE];
    125         ssize_t kcount;
    126         int ret;
    127
    128         if (count > FORMATTED_BUFFER_SIZE)
    129                 return -EINVAL;
    130
    131         kcount = simple_write_to_buffer(buf, FORMATTED_BUFFER_SIZE, ppos,
    132                                         user_buf, count);
    133         if (kcount < 0)
    134                 return kcount;
    135
    136         ret = parse_hex_sentence(buf, kcount, request_data, TYPE_AND_DATA_SIZE);
    137         if (ret < 0)
    138                 return ret;
    139         /* Need at least two bytes for message type */
    140         if (ret < 2)
                    ^^^^^^^

Assume "ret = 2".

    141                 return -EINVAL;
    142
    143         /* Clear response data buffer */
    144         memset(debug_info->raw_data, '\0', EC_MAILBOX_DATA_SIZE_EXTENDED);
    145
    146         msg.type = request_data[0] << 8 | request_data[1];
    147         msg.flags = WILCO_EC_FLAG_RAW;
    148         msg.command = ret > 2 ? request_data[2] : 0;
                ^^^^^^^^^^^^^
So command is zero.
    149         msg.request_data = ret > 3 ? request_data + 3 : 0;
--> 150         msg.request_size = ret - 3;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^

And request_size is u32max.

    151         msg.response_data = debug_info->raw_data;
    152         msg.response_size = EC_MAILBOX_DATA_SIZE;
    153
    154         /* Telemetry commands use extended response data */
    155         if (msg.type = WILCO_EC_MSG_TELEMETRY_LONG) {
    156                 msg.flags |= WILCO_EC_FLAG_EXTENDED_DATA;
    157                 msg.response_size = EC_MAILBOX_DATA_SIZE_EXTENDED;
    158         }
    159
    160         ret = wilco_ec_mailbox(debug_info->ec, &msg);
                                                       ^^^^

It leads to memory corruption when we do the memmove() in wilco_ec_prepare().

    161         if (ret < 0)
    162                 return ret;
    163         debug_info->response_size = ret;
    164
    165         return count;
    166 }

regards,
dan carpenter

Hello Nick Crews,

The patch f7b0bc5eafa4: "platform/chrome: wilco_ec: Add event handling" from May 23, 2019, leads to the following static checker warning:

drivers/platform/chrome/wilco_ec/event.c:352 event_read()
warn: inconsistent returns 'dev_data->lock'.
Locked on : 345
Unlocked on: 323,333,352

drivers/platform/chrome/wilco_ec/event.c
306 static ssize_t event_read(struct file *filp, char __user *buf, size_t count,
307 loff_t *pos)

   308  {
   309          struct event_device_data *dev_data = filp->private_data;
   310          struct ec_event_entry *entry;
   311          ssize_t n_bytes_written = 0;
   312          int err;
   313
   314          /* We only will give them the entire event at once */
   315          if (count != 0 && count < EC_ACPI_MAX_EVENT_SIZE)
   316                  return -EINVAL;
   317
   318          mutex_lock(&dev_data->lock);
   319
   320          while (dev_data->num_events = 0) {
   321                  if (filp->f_flags & O_NONBLOCK) {
   322                          mutex_unlock(&dev_data->lock);
   323                          return -EAGAIN;
   324                  }
   325                  /* Need to unlock so that data can actually get added to the
   326                   * queue, and since we recheck before use and it's just
   327                   * comparing pointers, this is safe unlocked.
   328                   */
   329                  mutex_unlock(&dev_data->lock);
   330                  err = wait_event_interruptible(dev_data->wq,
   331                                                 dev_data->num_events);
   332                  if (err)
   333                          return err;
   334
   335                  /* Device was removed as we waited? */
   336                  if (!dev_data->exist)
   337                          return -ENODEV;
   338                  mutex_lock(&dev_data->lock);
   339          }
   340
   341          entry = list_first_entry(&dev_data->events,
   342                                   struct ec_event_entry, list);
   343          n_bytes_written = entry->size;
   344          if (copy_to_user(buf, &entry->event, n_bytes_written))
   345                  return -EFAULT;

We need to unlock here. But also maybe we should do other error handling like the list_del() as well? I'm not sure.

   346          list_del(&entry->list);
   347          kfree(entry);
   348          dev_data->num_events--;
   349
   350          mutex_unlock(&dev_data->lock);
   351
   352          return n_bytes_written;
   353  }

regards,
dan carpenter

Quoted 54 lines · Dan Carpenter

Hello Nick Crews,

The patch f7b0bc5eafa4: "platform/chrome: wilco_ec: Add event handling" from May 23, 2019, leads to the following static checker warning:

drivers/platform/chrome/wilco_ec/event.c:352 event_read()
warn: inconsistent returns 'dev_data->lock'.
Locked on : 345
Unlocked on: 323,333,352

drivers/platform/chrome/wilco_ec/event.c
306 static ssize_t event_read(struct file *filp, char __user *buf, size_t count,
307 loff_t *pos)

   308  {
   309          struct event_device_data *dev_data = filp->private_data;
   310          struct ec_event_entry *entry;
   311          ssize_t n_bytes_written = 0;
   312          int err;
   313
   314          /* We only will give them the entire event at once */
   315          if (count != 0 && count < EC_ACPI_MAX_EVENT_SIZE)
   316                  return -EINVAL;
   317
   318          mutex_lock(&dev_data->lock);
   319
   320          while (dev_data->num_events = 0) {
   321                  if (filp->f_flags & O_NONBLOCK) {
   322                          mutex_unlock(&dev_data->lock);
   323                          return -EAGAIN;
   324                  }
   325                  /* Need to unlock so that data can actually get added to the
   326                   * queue, and since we recheck before use and it's just
   327                   * comparing pointers, this is safe unlocked.
   328                   */
   329                  mutex_unlock(&dev_data->lock);
   330                  err = wait_event_interruptible(dev_data->wq,
   331                                                 dev_data->num_events);
   332                  if (err)
   333                          return err;
   334
   335                  /* Device was removed as we waited? */
   336                  if (!dev_data->exist)
   337                          return -ENODEV;
   338                  mutex_lock(&dev_data->lock);
   339          }
   340
   341          entry = list_first_entry(&dev_data->events,
   342                                   struct ec_event_entry, list);
   343          n_bytes_written = entry->size;
   344          if (copy_to_user(buf, &entry->event, n_bytes_written))
   345                  return -EFAULT;

Quoted 2 lines

We need to unlock here. But also maybe we should do other error handling like the list_del() as well? I'm not sure.

Thanks, I'll look into this.

Enric, should I submit a completely new version of this, or just a patch on top of this? Also, I still want to add in the circular buffer for events to prevent OOM.

Nick

Quoted 12 lines

   346          list_del(&entry->list);
   347          kfree(entry);
   348          dev_data->num_events--;
   349
   350          mutex_unlock(&dev_data->lock);
   351
   352          return n_bytes_written;
   353  }

regards,
dan carpenter