| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200720121257.GJ2571@kadam>
Date: Mon, 20 Jul 2020 15:12:57 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: Peilin Ye <yepeilin.cs@...il.com>
Cc: Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
syzkaller-bugs@...glegroups.com,
linux-kernel-mentees@...ts.linuxfoundation.org,
linux-usb@...r.kernel.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds
write in hiddev_ioctl_usage()
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
Powered by blists - more mailing lists