| Message ID | 20200718231218.170730-1-yepeilin.cs@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [Linux-kernel-mentees,v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage() | expand |
On Sat, Jul 18, 2020 at 07:12:18PM -0400, Peilin Ye wrote: > `uref->usage_index` is not always being properly checked, causing > hiddev_ioctl_usage() to go out of bounds under some cases. Fix it. > Yeah. This code is not obvious. It doesn't come from the user directly so we don't have to worry about nospec. It comes from hiddev_lookup_usage() where we set: uref->usage_index = j; We know that j is less than field->maxusage but we do need to check against field->report_count like your patch does... The two arrays are allocated in hid_register_field(). I don't know the code well enough to say how these arrays are used or why the one is larger than the other so I can't give a proper reviewed-by. But the patch looks reasonable and doesn't introduce any bugs which weren't there in the original code. regards, dan carpenter
The problem is there is another bug on the lines before...
drivers/hid/usbhid/hiddev.c
475 default:
476 if (cmd != HIDIOCGUSAGE &&
477 cmd != HIDIOCGUSAGES &&
478 uref->report_type == HID_REPORT_TYPE_INPUT)
479 goto inval;
480
481 if (uref->report_id == HID_REPORT_ID_UNKNOWN) {
482 field = hiddev_lookup_usage(hid, uref);
This code is obviously buggy because syzkaller triggers an Oops and it's
pretty complicated to review (meaning that you have to jump around to a
lot of different places instead of just reading it from top to bottom
as static analysis would).
The user controlls "uref->report_id". If hiddev_lookup_usage() finds
something we know that uref->usage_index is a valid offset into the
field->usage[] array but it might be too large for the field->value[]
array.
483 if (field == NULL)
484 goto inval;
485 } else {
486 rinfo.report_type = uref->report_type;
487 rinfo.report_id = uref->report_id;
488 if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
489 goto inval;
490
491 if (uref->field_index >= report->maxfield)
492 goto inval;
493 uref->field_index = array_index_nospec(uref->field_index,
494 report->maxfield);
495
496 field = report->field[uref->field_index];
497
498 if (cmd == HIDIOCGCOLLECTIONINDEX) {
499 if (uref->usage_index >= field->maxusage)
500 goto inval;
501 uref->usage_index =
502 array_index_nospec(uref->usage_index,
503 field->maxusage);
504 } else if (uref->usage_index >= field->report_count)
505 goto inval;
506 }
507
508 if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
509 if (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
510 uref->usage_index + uref_multi->num_values >
511 field->report_count)
512 goto inval;
513
514 uref->usage_index =
515 array_index_nospec(uref->usage_index,
516 field->report_count -
517 uref_multi->num_values);
We check that it is a valid offset into the ->value[] array for these
two ioctl cmds.
518 }
519
520 switch (cmd) {
521 case HIDIOCGUSAGE:
522 uref->value = field->value[uref->usage_index];
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Not checked.
523 if (copy_to_user(user_arg, uref, sizeof(*uref)))
524 goto fault;
525 goto goodreturn;
526
527 case HIDIOCSUSAGE:
528 field->value[uref->usage_index] = uref->value;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This one you fixed.
529 goto goodreturn;
530
531 case HIDIOCGCOLLECTIONINDEX:
532 i = field->usage[uref->usage_index].collection_index;
533 kfree(uref_multi);
534 return i;
535 case HIDIOCGUSAGES:
536 for (i = 0; i < uref_multi->num_values; i++)
537 uref_multi->values[i] =
538 field->value[uref->usage_index + i];
fine.
539 if (copy_to_user(user_arg, uref_multi,
540 sizeof(*uref_multi)))
541 goto fault;
542 goto goodreturn;
543 case HIDIOCSUSAGES:
544 for (i = 0; i < uref_multi->num_values; i++)
545 field->value[uref->usage_index + i] =
also fine.
546 uref_multi->values[i];
547 goto goodreturn;
548 }
549
550 goodreturn:
551 kfree(uref_multi);
552 return 0;
553 fault:
554 kfree(uref_multi);
555 return -EFAULT;
556 inval:
557 kfree(uref_multi);
558 return -EINVAL;
559 }
560 }
So another option would be to just add HIDIOCGUSAGE and HIDIOCSUSAGE to
the earlier check. That risks breaking userspace. Another option is to
just add a check like you did earlier to the HIDIOCGUSAGE case.
Probably just do option #2 and resend.
regards,
dan carpenter
On Mon, Jul 20, 2020 at 03:12:57PM +0300, Dan Carpenter wrote: > So another option would be to just add HIDIOCGUSAGE and HIDIOCSUSAGE to > the earlier check. That risks breaking userspace. Another option is to > just add a check like you did earlier to the HIDIOCGUSAGE case. > Probably just do option #2 and resend. Sure, I will just add the same check to the HIDIOCGUSAGE case for the time being. Thank you for the detailed explanation. Here's what I found after digging a bit further though: hid_parser_main() calls different functions in order to process different type of items: drivers/hid/hid-core.c:1193: static int (*dispatch_type[])(struct hid_parser *parser, struct hid_item *item) = { hid_parser_main, hid_parser_global, hid_parser_local, hid_parser_reserved }; In this case, hid_parser_main() calls hid_add_field(), which in turn calls hid_register_field(), which allocates the `field` object as you mentioned: drivers/hid/hid-core.c:102: field = kzalloc((sizeof(struct hid_field) + usages * sizeof(struct hid_usage) + values * sizeof(unsigned)), GFP_KERNEL); Here, `values` equals to `global.report_count`. See how it is being called: drivers/hid/hid-core.c:303: field = hid_register_field(report, usages, parser->global.report_count); In hid_parser_main(), `global.report_count` can be set by calling hid_parser_global(). However, the syzkaller reproducer made hid_parser_main() to call hid_parser_global() __before__ `global.report_count` is properly set. It's zero. So hid_register_field() allocated `field` with `values` equals to zero - No room for value[] at all. I believe this caused the bug. Apparently hid_parser_main() doesn't care about which item (main, local, global and reserved) gets processed first. I am new to this code and I don't know whether this is by design, but this arbitrarity is apparently causing some issues. As another example, in hid_add_field(): drivers/hid/hid-core.c:289: report->size += parser->global.report_size * parser->global.report_count; If `global.report_count` is zero, `report->size` gets increased by zero. Is this working as intended? It seems weird to me. Thank you, Peilin Ye
I made some mistakes in the previous e-mail. Please ignore that. There are a lot of things going on...Sorry for that. On Mon, Jul 20, 2020 at 03:12:57PM +0300, Dan Carpenter wrote: > So another option would be to just add HIDIOCGUSAGE and HIDIOCSUSAGE to > the earlier check. That risks breaking userspace. Another option is to > just add a check like you did earlier to the HIDIOCGUSAGE case. > Probably just do option #2 and resend. Sure, I will just add the same check to the HIDIOCGUSAGE case for the time being. Thank you for the detailed explanation. Here's what I found after digging a bit further though: hid_open_report() calls different functions in order to process different type of items: drivers/hid/hid-core.c:1193: static int (*dispatch_type[])(struct hid_parser *parser, struct hid_item *item) = { hid_parser_main, hid_parser_global, hid_parser_local, hid_parser_reserved }; In this case, hid_parser_main() calls hid_add_field(), which in turn calls hid_register_field(), which allocates the `field` object as you mentioned: drivers/hid/hid-core.c:102: field = kzalloc((sizeof(struct hid_field) + usages * sizeof(struct hid_usage) + values * sizeof(unsigned)), GFP_KERNEL); Here, `values` equals to `global.report_count`. See how it is being called: drivers/hid/hid-core.c:303: field = hid_register_field(report, usages, parser->global.report_count); In hid_open_report(), `global.report_count` can be set by calling hid_parser_global(). However, the syzkaller reproducer made hid_open_report() to call hid_parser_main() __before__ `global.report_count` is properly set. It's zero. So hid_register_field() allocated `field` with `values` equals to zero - No room for value[] at all. I believe this caused the bug. Apparently hid_open_report() doesn't care about which item (main, local, global and reserved) gets processed first. I am new to this code and I don't know whether this is by design, but this arbitrarity is apparently causing some issues. As another example, in hid_add_field(): drivers/hid/hid-core.c:289: report->size += parser->global.report_size * parser->global.report_count; If `global.report_count` is zero, `report->size` gets increased by zero. Is this working as intended? It seems weird to me. Thank you, Peilin Ye
For some reason the reply-to header on your email is bogus: Reply-To: 20200720121257.GJ2571@kadam "kadam" is a system on my home network. On Mon, Jul 20, 2020 at 03:16:56PM -0400, Peilin Ye wrote: > I made some mistakes in the previous e-mail. Please ignore that. There > are a lot of things going on...Sorry for that. > > On Mon, Jul 20, 2020 at 03:12:57PM +0300, Dan Carpenter wrote: > > So another option would be to just add HIDIOCGUSAGE and HIDIOCSUSAGE to > > the earlier check. That risks breaking userspace. Another option is to > > just add a check like you did earlier to the HIDIOCGUSAGE case. > > Probably just do option #2 and resend. > > Sure, I will just add the same check to the HIDIOCGUSAGE case for the > time being. Thank you for the detailed explanation. > > Here's what I found after digging a bit further though: > > hid_open_report() calls different functions in order to process > different type of items: > > drivers/hid/hid-core.c:1193: > > static int (*dispatch_type[])(struct hid_parser *parser, > struct hid_item *item) = { > hid_parser_main, > hid_parser_global, > hid_parser_local, > hid_parser_reserved > }; > > In this case, hid_parser_main() calls hid_add_field(), which in turn > calls hid_register_field(), which allocates the `field` object as you > mentioned: > > drivers/hid/hid-core.c:102: > > field = kzalloc((sizeof(struct hid_field) + > usages * sizeof(struct hid_usage) + > values * sizeof(unsigned)), GFP_KERNEL); Yeah. And in the caller it does: drivers/hid/hid-core.c 297 if (!parser->local.usage_index) /* Ignore padding fields */ 298 return 0; 299 300 usages = max_t(unsigned, parser->local.usage_index, ^^^^^^^^^^^^^^ 301 parser->global.report_count); 302 303 field = hid_register_field(report, usages, parser->global.report_count); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ So ->usages is always greater or equal to ->global.report_count. 304 if (!field) 305 return 0; 306 307 field->physical = hid_lookup_collection(parser, HID_COLLECTION_PHYSICAL); > > Here, `values` equals to `global.report_count`. See how it is being > called: > > drivers/hid/hid-core.c:303: > > field = hid_register_field(report, usages, parser->global.report_count); > > In hid_open_report(), `global.report_count` can be set by calling > hid_parser_global(). > > However, the syzkaller reproducer made hid_open_report() to call > hid_parser_main() __before__ `global.report_count` is properly set. It's > zero. So hid_register_field() allocated `field` with `values` equals to > zero - No room for value[] at all. I believe this caused the bug. I don't know if zero is valid or not. I suspect it is valid. We have no reason to think that it's invalid. regards, dan carpenter
On Tue, Jul 21, 2020 at 10:16:37AM +0300, Dan Carpenter wrote: > For some reason the reply-to header on your email is bogus: > > Reply-To: 20200720121257.GJ2571@kadam > > "kadam" is a system on my home network. That's your message-id :)
On Tue, Jul 21, 2020 at 10:27:49AM +0200, Greg Kroah-Hartman wrote: > On Tue, Jul 21, 2020 at 10:16:37AM +0300, Dan Carpenter wrote: > > For some reason the reply-to header on your email is bogus: > > > > Reply-To: 20200720121257.GJ2571@kadam > > > > "kadam" is a system on my home network. > > That's your message-id :) Ah... It's a typo. Peilin meant "In-Reply-to" but some how set both the In-Reply-to and the Reply-to headers to the same thing. regards, dan carpenter
On Tue, Jul 21, 2020 at 10:16:37AM +0300, Dan Carpenter wrote: > For some reason the reply-to header on your email is bogus: > > Reply-To: 20200720121257.GJ2571@kadam > > "kadam" is a system on my home network. Ah...I thought `Reply-To` and `In-Reply-To` are the same thing...Sorry for the beginner's mistake... > Yeah. And in the caller it does: > > drivers/hid/hid-core.c > 297 if (!parser->local.usage_index) /* Ignore padding fields */ > 298 return 0; > 299 > 300 usages = max_t(unsigned, parser->local.usage_index, > ^^^^^^^^^^^^^^ > 301 parser->global.report_count); > 302 > 303 field = hid_register_field(report, usages, parser->global.report_count); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > So ->usages is always greater or equal to ->global.report_count. > > 304 if (!field) > 305 return 0; > 306 > 307 field->physical = hid_lookup_collection(parser, HID_COLLECTION_PHYSICAL); > > > > > Here, `values` equals to `global.report_count`. See how it is being > > called: > > > > drivers/hid/hid-core.c:303: > > > > field = hid_register_field(report, usages, parser->global.report_count); > > > > In hid_open_report(), `global.report_count` can be set by calling > > hid_parser_global(). > > > > However, the syzkaller reproducer made hid_open_report() to call > > hid_parser_main() __before__ `global.report_count` is properly set. It's > > zero. So hid_register_field() allocated `field` with `values` equals to > > zero - No room for value[] at all. I believe this caused the bug. > > I don't know if zero is valid or not. I suspect it is valid. We have > no reason to think that it's invalid. I see, I will stop guessing and wait for the maintainers' feedback. Thank you, Peilin Ye
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c index 4140dea693e9..c63d07caacef 100644 --- a/drivers/hid/usbhid/hiddev.c +++ b/drivers/hid/usbhid/hiddev.c @@ -525,6 +525,8 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd, goto goodreturn; case HIDIOCSUSAGE: + if (uref->usage_index >= field->report_count) + goto inval; field->value[uref->usage_index] = uref->value; goto goodreturn;
`uref->usage_index` is not always being properly checked, causing hiddev_ioctl_usage() to go out of bounds under some cases. Fix it. This patch fixes the following syzbot bug: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802 Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> --- This patch fixes the bug, but in an ugly way. Checks on `uref` are already being done in this code: if (cmd == HIDIOCGCOLLECTIONINDEX) { if (uref->usage_index >= field->maxusage) goto inval; uref->usage_index = array_index_nospec(uref->usage_index, field->maxusage); } else if (uref->usage_index >= field->report_count) goto inval; However it did not catch this bug since it's in an `else` bracket. Should we move the above code out of the bracket? Would like to hear your opinion. Thank you! drivers/hid/usbhid/hiddev.c | 2 ++ 1 file changed, 2 insertions(+)