* [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs
@ 2019-02-19 19:37 Dan Carpenter
2019-02-20 22:04 ` Nick Crews
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Dan Carpenter @ 2019-02-19 19:37 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs
2019-02-19 19:37 [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs Dan Carpenter
@ 2019-02-20 22:04 ` Nick Crews
2019-06-07 8:18 ` [bug report] platform/chrome: wilco_ec: Add event handling Dan Carpenter
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Nick Crews @ 2019-02-20 22:04 UTC (permalink / raw)
To: kernel-janitors
Thanks Dan for checking this. I sent a patch that fixes this, subject
"[PATCH -next] platform/chrome: Fix off-by-one error in
wilco_ec/debugfs.c". You should have been CC'd.
Thanks,
Nick
On Tue, Feb 19, 2019 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] platform/chrome: wilco_ec: Add event handling
2019-02-19 19:37 [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs Dan Carpenter
2019-02-20 22:04 ` Nick Crews
@ 2019-06-07 8:18 ` Dan Carpenter
2019-06-07 19:47 ` Nick Crews
2019-06-10 9:36 ` Enric Balletbo i Serra
3 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2019-06-07 8:18 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] platform/chrome: wilco_ec: Add event handling
2019-02-19 19:37 [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs Dan Carpenter
2019-02-20 22:04 ` Nick Crews
2019-06-07 8:18 ` [bug report] platform/chrome: wilco_ec: Add event handling Dan Carpenter
@ 2019-06-07 19:47 ` Nick Crews
2019-06-10 9:36 ` Enric Balletbo i Serra
3 siblings, 0 replies; 5+ messages in thread
From: Nick Crews @ 2019-06-07 19:47 UTC (permalink / raw)
To: kernel-janitors
On Fri, Jun 7, 2019 at 2:18 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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.
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
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] platform/chrome: wilco_ec: Add event handling
2019-02-19 19:37 [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs Dan Carpenter
` (2 preceding siblings ...)
2019-06-07 19:47 ` Nick Crews
@ 2019-06-10 9:36 ` Enric Balletbo i Serra
3 siblings, 0 replies; 5+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-10 9:36 UTC (permalink / raw)
To: kernel-janitors
Hi Nick,
On 7/6/19 21:47, Nick Crews wrote:
> On Fri, Jun 7, 2019 at 2:18 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>
>> 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.
>
> 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.
>
Two patches on top, the first one addind the Reported-by tag.
Enric
> Nick
>
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-10 9:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-19 19:37 [bug report] platform/chrome: wilco_ec: Add support for raw commands in debugfs Dan Carpenter
2019-02-20 22:04 ` Nick Crews
2019-06-07 8:18 ` [bug report] platform/chrome: wilco_ec: Add event handling Dan Carpenter
2019-06-07 19:47 ` Nick Crews
2019-06-10 9:36 ` Enric Balletbo i Serra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).