* [bug report] HID: intel-ish: enable raw interface to HID devices on ISH
@ 2026-05-18 13:25 Dan Carpenter
2026-05-18 13:51 ` Dan Carpenter
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2026-05-18 13:25 UTC (permalink / raw)
To: Hyungwoo Yang; +Cc: linux-input
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].
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] HID: intel-ish: enable raw interface to HID devices on ISH
2026-05-18 13:25 [bug report] HID: intel-ish: enable raw interface to HID devices on ISH Dan Carpenter
@ 2026-05-18 13:51 ` Dan Carpenter
0 siblings, 0 replies; 2+ messages in thread
From: Dan Carpenter @ 2026-05-18 13:51 UTC (permalink / raw)
To: Zhang Lixu, Jiri Kosina; +Cc: linux-input
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].
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-05-18 13:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-18 13:25 [bug report] HID: intel-ish: enable raw interface to HID devices on ISH Dan Carpenter
2026-05-18 13:51 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox