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
