[bug report] HID: intel-ish: enable raw interface to HID devices on ISH

[Dan Carpenter <error27@gmail.com>]

Hello Hyungwoo Yang,

Commit e19595fcabb5 ("HID: intel-ish: enable raw interface to HID
devices on ISH") from Mar 4, 2019 (linux-next), leads to the
following Smatch static checker warning:

	drivers/hid/intel-ish-hid/ishtp-hid-client.c:225 process_recv()
	warn: potentially one past the end of array 'client_data->hid_devices[i]'

drivers/hid/intel-ish-hid/ishtp-hid-client.c
    66 static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
    67                          size_t data_len)
    68 {
    69         struct hostif_msg *recv_msg;
    70         unsigned char *payload;
    71         struct device_info *dev_info;
    72         int i, j;
    73         size_t        payload_len, total_len, cur_pos, raw_len, msg_len;
    74         int report_type;
    75         struct report_list *reports_list;
    76         struct report *report;
    77         size_t report_len;
    78         struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
    79         int curr_hid_dev = client_data->cur_hid_dev;
    80         struct ishtp_hid_data *hid_data = NULL;
    81         struct hid_device *hid = NULL;
    82 
    83         payload = recv_buf + sizeof(struct hostif_msg_hdr);
    84         total_len = data_len;
    85         cur_pos = 0;
    86 
    87         do {
    88                 if (cur_pos + sizeof(struct hostif_msg) > total_len) {
    89                         dev_err(cl_data_to_dev(client_data),
    90                                 "[hid-ish]: error, received %u which is less than data header %u\n",
    91                                 (unsigned int)data_len,
    92                                 (unsigned int)sizeof(struct hostif_msg_hdr));
    93                         ++client_data->bad_recv_cnt;
    94                         ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
    95                         break;
    96                 }
    97 
    98                 recv_msg = (struct hostif_msg *)(recv_buf + cur_pos);
    99                 payload_len = recv_msg->hdr.size;
    100 
    101                 /* Sanity checks */
    102                 if (cur_pos + payload_len + sizeof(struct hostif_msg) >
    103                                 total_len) {
    104                         ++client_data->bad_recv_cnt;
    105                         report_bad_packet(hid_ishtp_cl, recv_msg, cur_pos,
    106                                           payload_len);
    107                         ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
    108                         break;
    109                 }
    110 
    111                 hid_ishtp_trace(client_data,  "%s %d\n",
    112                                 __func__, recv_msg->hdr.command & CMD_MASK);
    113 
    114                 switch (recv_msg->hdr.command & CMD_MASK) {
    115                 case HOSTIF_DM_ENUM_DEVICES:
    116                         if ((!(recv_msg->hdr.command & ~CMD_MASK) ||
    117                                         client_data->init_done)) {
    118                                 ++client_data->bad_recv_cnt;
    119                                 report_bad_packet(hid_ishtp_cl, recv_msg,
    120                                                   cur_pos,
    121                                                   payload_len);
    122                                 ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
    123                                 break;
    124                         }
    125                         client_data->hid_dev_count = (unsigned int)*payload;
    126                         if (!client_data->hid_devices)
    127                                 client_data->hid_devices = devm_kcalloc(
    128                                                 cl_data_to_dev(client_data),
    129                                                 client_data->hid_dev_count,
    130                                                 sizeof(struct device_info),
    131                                                 GFP_KERNEL);

We only allocate the client_data->hid_devices buffer one time, and
then re-use it after that.

1) However, how do we know that the new value of client_data->hid_dev_count
   assigned on line 125 is the same or smaller than the previous value?
2) I think we need to update client_data->num_hid_devices as well since
   that has to be == client_data->hid_dev_count.  Also we assume that if
   client_data->num_hid_devices is non-zero then client_data->hid_devices
   was allocated successfully.

Or maybe client_data->hid_devices is always NULL here and the check
can be removed that way?  In that case we just need to make a cleanup
and there is no security bug, however there is another security issue
below.

    132                         if (!client_data->hid_devices) {
    133                                 dev_err(cl_data_to_dev(client_data),
    134                                 "Mem alloc failed for hid device info\n");
    135                                 wake_up_interruptible(&client_data->init_wait);
    136                                 break;
    137                         }
    138                         for (i = 0; i < client_data->hid_dev_count; ++i) {
    139                                 if (1 + sizeof(struct device_info) * i >=
    140                                                 payload_len) {
    141                                         dev_err(cl_data_to_dev(client_data),
    142                                                 "[hid-ish]: [ENUM_DEVICES]: content size %zu is bigger than payload_len %zu\n",
    143                                                 1 + sizeof(struct device_info)
    144                                                 * i, payload_len);


3) This if statement is testing if we are reading beyond the end of the
   buffer, but it just prints an error message and continues reading.
   So it is an information leak.  It should error out.

    145                                 }
    146 
    147                                 if (1 + sizeof(struct device_info) * i >=
    148                                                 data_len)
    149                                         break;

4) It's weird that if we have an overflow in this inner loop we break but
   potentially still keep iterating in the outer loop.

    150 
    151                                 dev_info = (struct device_info *)(payload + 1 +
    152                                         sizeof(struct device_info) * i);
    153                                 if (client_data->hid_devices)
                                            ^^^^^^^^^^^^^^^^^^^^^^^^
5) It's not a bug, but this NULL check can be removed.  It triggers a
   static checker warning about inconsistent NULL checking.

regards,
dan carpenter

    154                                         memcpy(client_data->hid_devices + i,
    155                                                dev_info,
    156                                                sizeof(struct device_info));
    157                         }
    158 
    159                         client_data->enum_devices_done = true;
    160                         wake_up_interruptible(&client_data->init_wait);
    161 

This email is a free service from the Smatch-CI project [smatch.sf.net].