* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
@ 2020-07-20 12:12 ` Dan Carpenter
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ messages in thread
From: Peilin Ye @ 2020-07-20 19:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
syzkaller-bugs, linux-kernel-mentees, linux-usb, linux-input,
linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
@ 2020-07-20 19:05 ` Peilin Ye
0 siblings, 0 replies; 30+ 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] 30+ 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:16 ` Peilin Ye
-1 siblings, 0 replies; 30+ messages in thread
From: Peilin Ye @ 2020-07-20 19:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
syzkaller-bugs, linux-kernel-mentees, linux-usb, linux-input,
linux-kernel
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
^ permalink raw reply [flat|nested] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-07-21 7:16 UTC (permalink / raw)
To: Peilin Ye
Cc: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
syzkaller-bugs, linux-kernel-mentees, linux-usb, linux-input,
linux-kernel
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
^ permalink raw reply [flat|nested] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-21 8:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: Peilin Ye, Jiri Kosina, Benjamin Tissoires, syzkaller-bugs,
linux-kernel-mentees, linux-usb, linux-input, linux-kernel
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 :)
^ permalink raw reply [flat|nested] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-07-21 8:39 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Peilin Ye, Jiri Kosina, Benjamin Tissoires, syzkaller-bugs,
linux-kernel-mentees, linux-usb, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
@ 2020-07-21 8:39 ` Dan Carpenter
0 siblings, 0 replies; 30+ 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] 30+ 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:39 ` Peilin Ye
-1 siblings, 0 replies; 30+ messages in thread
From: Peilin Ye @ 2020-07-21 8:39 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
syzkaller-bugs, linux-kernel-mentees, linux-usb, linux-input,
linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()
@ 2020-07-21 8:39 ` Peilin Ye
0 siblings, 0 replies; 30+ 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] 30+ messages in thread