From: Dan Carpenter <dan.carpenter@oracle.com>
To: Peilin Ye <yepeilin.cs@gmail.com>
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
syzkaller-bugs@googlegroups.com,
linux-kernel-mentees@lists.linuxfoundation.org,
linux-usb@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
Date: Mon, 20 Jul 2020 15:12:57 +0300 [thread overview]
Message-ID: <20200720121257.GJ2571@kadam> (raw)
In-Reply-To: <20200720115400.GI2549@kadam>
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
next prev parent reply other threads:[~2020-07-20 12:13 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-18 23:12 [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage() Peilin Ye
2020-07-20 11:54 ` Dan Carpenter
2020-07-20 12:12 ` Dan Carpenter [this message]
2020-07-20 19:05 ` Peilin Ye
2020-07-20 19:16 ` Peilin Ye
2020-07-21 7:16 ` Dan Carpenter
2020-07-21 8:27 ` Greg Kroah-Hartman
2020-07-21 8:39 ` Dan Carpenter
2020-07-21 8:39 ` Peilin Ye
2020-07-20 19:52 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
2020-07-23 14:51 ` Dan Carpenter
2020-07-29 11:37 ` [Linux-kernel-mentees] [PATCH v2 RESEND] " Peilin Ye
2020-07-29 14:35 ` Dan Carpenter
2020-08-17 10:21 ` Jiri Kosina
2020-08-18 10:00 ` Peilin Ye
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200720121257.GJ2571@kadam \
--to=dan.carpenter@oracle.com \
--cc=benjamin.tissoires@redhat.com \
--cc=gregkh@linuxfoundation.org \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=syzkaller-bugs@googlegroups.com \
--cc=yepeilin.cs@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).