* [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
@ 2020-07-18 23:12 Peilin Ye
2020-07-20 11:54 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Peilin Ye @ 2020-07-18 23:12 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-usb, syzkaller-bugs, linux-kernel, linux-input,
linux-kernel-mentees, Peilin Ye
`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(+)
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;
--
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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
2020-07-20 19:52 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
2020-07-29 11:37 ` [Linux-kernel-mentees] [PATCH v2 RESEND] " Peilin Ye
2 siblings, 1 reply; 15+ messages in thread
From: Dan Carpenter @ 2020-07-20 11:54 UTC (permalink / raw)
To: Peilin Ye
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-07-20 11:54 ` Dan Carpenter
@ 2020-07-20 12:12 ` Dan Carpenter
2020-07-20 19:05 ` Peilin Ye
2020-07-20 19:16 ` Peilin Ye
0 siblings, 2 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-07-20 12:12 UTC (permalink / raw)
To: Peilin Ye
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-07-20 12:12 ` Dan Carpenter
@ 2020-07-20 19:05 ` Peilin Ye
2020-07-20 19:16 ` Peilin Ye
1 sibling, 0 replies; 15+ messages in thread
From: Peilin Ye @ 2020-07-20 19:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-07-20 12:12 ` Dan Carpenter
2020-07-20 19:05 ` Peilin Ye
@ 2020-07-20 19:16 ` Peilin Ye
2020-07-21 7:16 ` Dan Carpenter
1 sibling, 1 reply; 15+ messages in thread
From: Peilin Ye @ 2020-07-20 19:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Linux-kernel-mentees] [PATCH v2] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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 19:52 ` Peilin Ye
2020-07-23 14:51 ` Dan Carpenter
2020-07-29 11:37 ` [Linux-kernel-mentees] [PATCH v2 RESEND] " Peilin Ye
2 siblings, 1 reply; 15+ messages in thread
From: Peilin Ye @ 2020-07-20 19:52 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-usb, syzkaller-bugs, linux-kernel, linux-input,
linux-kernel-mentees, Peilin Ye, Dan Carpenter
`uref->usage_index` is not always being properly checked, causing
hiddev_ioctl_usage() to go out of bounds under some cases. Fix it.
Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Change in v2:
- Add the same check for the `HIDIOCGUSAGE` case. (Suggested by
Dan Carpenter <dan.carpenter@oracle.com>)
drivers/hid/usbhid/hiddev.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 4140dea693e9..4f97e6c12059 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -519,12 +519,16 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
switch (cmd) {
case HIDIOCGUSAGE:
+ if (uref->usage_index >= field->report_count)
+ goto inval;
uref->value = field->value[uref->usage_index];
if (copy_to_user(user_arg, uref, sizeof(*uref)))
goto fault;
goto goodreturn;
case HIDIOCSUSAGE:
+ if (uref->usage_index >= field->report_count)
+ goto inval;
field->value[uref->usage_index] = uref->value;
goto goodreturn;
--
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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 ` Peilin Ye
0 siblings, 2 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-07-21 7:16 UTC (permalink / raw)
To: Peilin Ye
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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
1 sibling, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-21 8:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees, Peilin Ye
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 :)
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-07-21 8:27 ` Greg Kroah-Hartman
@ 2020-07-21 8:39 ` Dan Carpenter
0 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-07-21 8:39 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees, Peilin Ye
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-07-21 7:16 ` Dan Carpenter
2020-07-21 8:27 ` Greg Kroah-Hartman
@ 2020-07-21 8:39 ` Peilin Ye
1 sibling, 0 replies; 15+ messages in thread
From: Peilin Ye @ 2020-07-21 8:39 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-07-20 19:52 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
@ 2020-07-23 14:51 ` Dan Carpenter
0 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-07-23 14:51 UTC (permalink / raw)
To: Peilin Ye
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
On Mon, Jul 20, 2020 at 03:52:09PM -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.
>
> Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Change in v2:
> - Add the same check for the `HIDIOCGUSAGE` case. (Suggested by
> Dan Carpenter <dan.carpenter@oracle.com>)
Looks good to me. Thanks!
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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 19:52 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
@ 2020-07-29 11:37 ` Peilin Ye
2020-07-29 14:35 ` Dan Carpenter
2020-08-17 10:21 ` Jiri Kosina
2 siblings, 2 replies; 15+ messages in thread
From: Peilin Ye @ 2020-07-29 11:37 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-usb, syzkaller-bugs, linux-kernel, linux-input,
linux-kernel-mentees, Peilin Ye, Dan Carpenter
`uref->usage_index` is not always being properly checked, causing
hiddev_ioctl_usage() to go out of bounds under some cases. Fix it.
Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Change in v2:
- Add the same check for the `HIDIOCGUSAGE` case. (Suggested by
Dan Carpenter <dan.carpenter@oracle.com>)
drivers/hid/usbhid/hiddev.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 4140dea693e9..4f97e6c12059 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -519,12 +519,16 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
switch (cmd) {
case HIDIOCGUSAGE:
+ if (uref->usage_index >= field->report_count)
+ goto inval;
uref->value = field->value[uref->usage_index];
if (copy_to_user(user_arg, uref, sizeof(*uref)))
goto fault;
goto goodreturn;
case HIDIOCSUSAGE:
+ if (uref->usage_index >= field->report_count)
+ goto inval;
field->value[uref->usage_index] = uref->value;
goto goodreturn;
--
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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
1 sibling, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-07-29 14:35 UTC (permalink / raw)
To: Peilin Ye
Cc: linux-usb, Jiri Kosina, syzkaller-bugs, linux-kernel,
Benjamin Tissoires, linux-input, linux-kernel-mentees
On Wed, Jul 29, 2020 at 07:37:12AM -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.
>
> Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Change in v2:
> - Add the same check for the `HIDIOCGUSAGE` case. (Suggested by
> Dan Carpenter <dan.carpenter@oracle.com>)
Why are you resending this?
regards,
dan carpenter
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
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
1 sibling, 1 reply; 15+ messages in thread
From: Jiri Kosina @ 2020-08-17 10:21 UTC (permalink / raw)
To: Peilin Ye
Cc: linux-usb, syzkaller-bugs, linux-kernel, Benjamin Tissoires,
linux-input, linux-kernel-mentees, Dan Carpenter
On Wed, 29 Jul 2020, 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.
>
> Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Change in v2:
> - Add the same check for the `HIDIOCGUSAGE` case. (Suggested by
> Dan Carpenter <dan.carpenter@oracle.com>)
Applied, thanks.
--
Jiri Kosina
SUSE Labs
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
2020-08-17 10:21 ` Jiri Kosina
@ 2020-08-18 10:00 ` Peilin Ye
0 siblings, 0 replies; 15+ messages in thread
From: Peilin Ye @ 2020-08-18 10:00 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-usb, syzkaller-bugs, linux-kernel, Benjamin Tissoires,
linux-input, linux-kernel-mentees, Dan Carpenter
On Mon, Aug 17, 2020 at 12:21:41PM +0200, Jiri Kosina wrote:
> On Wed, 29 Jul 2020, 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.
> >
> > Reported-by: syzbot+34ee1b45d88571c2fa8b@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?id=f2aebe90b8c56806b050a20b36f51ed6acabe802
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> > Change in v2:
> > - Add the same check for the `HIDIOCGUSAGE` case. (Suggested by
> > Dan Carpenter <dan.carpenter@oracle.com>)
>
> Applied, thanks.
Thank you for reviewing the patch!
Peilin Ye
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-08-18 10:00 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).