[Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Dan Carpenter]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Dan Carpenter]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Peilin Ye]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Peilin Ye]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Dan Carpenter]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Greg Kroah-Hartman]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Dan Carpenter]

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

Re: [Linux-kernel-mentees] [PATCH v1] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

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

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

[Linux-kernel-mentees] [PATCH v2] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

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

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

Re: [Linux-kernel-mentees] [PATCH v2] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Dan Carpenter]

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

[Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

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

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

Re: [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Dan Carpenter]

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

Re: [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Jiri Kosina]

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

Re: [Linux-kernel-mentees] [PATCH v2 RESEND] usbhid: Fix slab-out-of-bounds write in hiddev_ioctl_usage()

[Peilin Ye]

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