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

[Dan Carpenter <error27@gmail.com>]

This code is 7 years old now and Hyungwoo Yang's email is dead.  I know
Jiri reads mail to the list but let me add a Zhang Lixu to the CC list.

regards,
dan carpenter

On Mon, May 18, 2026 at 04:25:22PM +0300, Dan Carpenter wrote:
> 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].