* [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
* 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
* [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 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).