HID: hiddev: fix potential Spectre v1

HID: hiddev: fix potential Spectre v1

[Gustavo A. R. Silva]

uref->field_index, uref->usage_index, finfo.field_index and
cinfo.index can be indirectly controlled by user-space, hence
leading to a potential exploitation of the Spectre variant 1
vulnerability.

This issue was detected with the help of Smatch:

drivers/hid/usbhid/hiddev.c:473 hiddev_ioctl_usage() warn: potential
spectre issue 'report->field' (local cap)
drivers/hid/usbhid/hiddev.c:477 hiddev_ioctl_usage() warn: potential
spectre issue 'field->usage' (local cap)
drivers/hid/usbhid/hiddev.c:757 hiddev_ioctl() warn: potential spectre
issue 'report->field' (local cap)
drivers/hid/usbhid/hiddev.c:801 hiddev_ioctl() warn: potential spectre
issue 'hid->collection' (local cap)

Fix this by sanitizing such structure fields before using them to index
report->field, field->usage and hid->collection

Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/hid/usbhid/hiddev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index e3ce233..23872d0 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -36,6 +36,7 @@
 #include <linux/hiddev.h>
 #include <linux/compat.h>
 #include <linux/vmalloc.h>
+#include <linux/nospec.h>
 #include "usbhid.h"
 
 #ifdef CONFIG_USB_DYNAMIC_MINORS
@@ -469,10 +470,14 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
 
 		if (uref->field_index >= report->maxfield)
 			goto inval;
+		uref->field_index = array_index_nospec(uref->field_index,
+						       report->maxfield);
 
 		field = report->field[uref->field_index];
 		if (uref->usage_index >= field->maxusage)
 			goto inval;
+		uref->usage_index = array_index_nospec(uref->usage_index,
+						       field->maxusage);
 
 		uref->usage_code = field->usage[uref->usage_index].hid;
 
@@ -499,6 +504,8 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
 
 			if (uref->field_index >= report->maxfield)
 				goto inval;
+			uref->field_index = array_index_nospec(uref->field_index,
+							       report->maxfield);
 
 			field = report->field[uref->field_index];
 
@@ -753,6 +760,8 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		if (finfo.field_index >= report->maxfield)
 			break;
+		finfo.field_index = array_index_nospec(finfo.field_index,
+						       report->maxfield);
 
 		field = report->field[finfo.field_index];
 		memset(&finfo, 0, sizeof(finfo));
@@ -797,6 +806,8 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		if (cinfo.index >= hid->maxcollection)
 			break;
+		cinfo.index = array_index_nospec(cinfo.index,
+						 hid->maxcollection);
 
 		cinfo.type = hid->collection[cinfo.index].type;
 		cinfo.usage = hid->collection[cinfo.index].usage;

HID: hiddev: fix potential Spectre v1

[Jiri Kosina]

On Fri, 29 Jun 2018, Gustavo A. R. Silva wrote:

> uref->field_index, uref->usage_index, finfo.field_index and
> cinfo.index can be indirectly controlled by user-space, hence
> leading to a potential exploitation of the Spectre variant 1
> vulnerability.
> 
> This issue was detected with the help of Smatch:

Applied, thanks Gustavo.

HID: hiddev: fix potential Spectre v1

[Breno Leitao]

uref->usage_index can be indirectly controlled by userspace, hence leading
to a potential exploitation of the Spectre variant 1 vulnerability.

This problem might show up in the cmd = HIDIOCGCOLLECTIONINDEX flow at function
hiddev_ioctl_usage(), where uref->usage_index is compared to field->maxusage
and then used as an index to dereference field->usage array.

This is a summary of the current flow, which matches the traditional
Spectre V1 issue:

	copy_from_user(uref, user_arg, sizeof(*uref))
	if (uref->usage_index >= field->maxusage)
		goto inval;
	i = field->usage[uref->usage_index].collection_index;
	return i;

This patch fixes this by sanitizing field uref->usage_index before using it to
index field->usage, thus, avoiding speculation in the first load.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/hid/usbhid/hiddev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 23872d08308c..8829cbc1f6b1 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -512,6 +512,9 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
 			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;
 		}

HID: hiddev: fix potential Spectre v1

[Gustavo A. R. Silva]

Hi Breno,

On 10/17/18 9:47 PM, Breno Leitao wrote:
> uref->usage_index can be indirectly controlled by userspace, hence leading
> to a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This problem might show up in the cmd = HIDIOCGCOLLECTIONINDEX flow at function
> hiddev_ioctl_usage(), where uref->usage_index is compared to field->maxusage
> and then used as an index to dereference field->usage array.
> 
> This is a summary of the current flow, which matches the traditional
> Spectre V1 issue:
> 
> 	copy_from_user(uref, user_arg, sizeof(*uref))
> 	if (uref->usage_index >= field->maxusage)
> 		goto inval;
> 	i = field->usage[uref->usage_index].collection_index;
> 	return i;
> 
> This patch fixes this by sanitizing field uref->usage_index before using it to
> index field->usage, thus, avoiding speculation in the first load.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/hid/usbhid/hiddev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 23872d08308c..8829cbc1f6b1 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -512,6 +512,9 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
>  			if (cmd == HIDIOCGCOLLECTIONINDEX) {
>  				if (uref->usage_index >= field->maxusage)
>  					goto inval;
> +				uref->usage_index =
> +					array_index_nospec(uref->usage_index,
> +							   field->maxusage);

Good catch.

>  			} else if (uref->usage_index >= field->report_count)
>  				goto inval;

Although, notice that this is the same index, and it can be used to index field->value[]
at lines 526 and 532.

Thanks
---
Gustavo

HID: hiddev: fix potential Spectre v1

[Breno Leitao]

Hi Gustavo,

On 10/17/2018 05:30 PM, Gustavo A. R. Silva wrote:
> 
> Hi Breno,
> 
> On 10/17/18 9:47 PM, Breno Leitao wrote:
>> uref->usage_index can be indirectly controlled by userspace, hence leading
>> to a potential exploitation of the Spectre variant 1 vulnerability.
>>
>> This problem might show up in the cmd = HIDIOCGCOLLECTIONINDEX flow at function
>> hiddev_ioctl_usage(), where uref->usage_index is compared to field->maxusage
>> and then used as an index to dereference field->usage array.
>>
>> This is a summary of the current flow, which matches the traditional
>> Spectre V1 issue:
>>
>> 	copy_from_user(uref, user_arg, sizeof(*uref))
>> 	if (uref->usage_index >= field->maxusage)
>> 		goto inval;
>> 	i = field->usage[uref->usage_index].collection_index;
>> 	return i;
>>
>> This patch fixes this by sanitizing field uref->usage_index before using it to
>> index field->usage, thus, avoiding speculation in the first load.
>>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>>  drivers/hid/usbhid/hiddev.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
>> index 23872d08308c..8829cbc1f6b1 100644
>> --- a/drivers/hid/usbhid/hiddev.c
>> +++ b/drivers/hid/usbhid/hiddev.c
>> @@ -512,6 +512,9 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
>>  			if (cmd == HIDIOCGCOLLECTIONINDEX) {
>>  				if (uref->usage_index >= field->maxusage)
>>  					goto inval;
>> +				uref->usage_index =
>> +					array_index_nospec(uref->usage_index,
>> +							   field->maxusage);
> 
> Good catch.
> 
>>  			} else if (uref->usage_index >= field->report_count)
>>  				goto inval;
> 
> Although, notice that this is the same index, and it can be used to index field->value[]
> at lines 526 and 532.

Right, this seems to be a possible problem also, when 'cmd' = HIDIOC{G,S}USAGES.

I am reworking the patch to cover both issues. What do you think of the draft
below?

Thank you for reviewing it!
---

Subject: [PATCH] HID: hiddev: fix potential Spectre v1

uref->usage_index can be indirectly controlled by userspace, hence leading
to a potential exploitation of the Spectre variant 1 vulnerability.

This field is used as an array index by the hiddev_ioctl_usage() function,
when 'cmd' is HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or HIDIOCSUSAGES.

For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
field->maxusage and then used as an index to dereference field->usage
array.  The very same thing happens to the cmd == HIDIOC{G,S}USAGES cases,
where uref->usage_index is checked against an array maximum value and then
it is used as an index in this array.

This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
traditional Spectre V1 first load:

	copy_from_user(uref, user_arg, sizeof(*uref))
	if (uref->usage_index >= field->maxusage)
		goto inval;
	i = field->usage[uref->usage_index].collection_index;
	return i;

This patch fixes this by sanitizing field uref->usage_index before using it
to index field->usage, thus, avoiding speculation in the first load.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/hid/usbhid/hiddev.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

 		case HIDIOCGUSAGE:

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 23872d08308c..053f93bdee72 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -512,14 +512,24 @@ static noinline int hiddev_ioctl_usage(struct hiddev
*hiddev, unsigned int cmd,
 			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;
 		}

-		if ((cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) &&
-		    (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
-		     uref->usage_index + uref_multi->num_values > field->report_count))
-			goto inval;
+		if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
+			if (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
+			    uref->usage_index + uref_multi->num_values >
+				 field->report_count)
+				goto inval;
+
+			uref->usage_index =
+				array_index_nospec(uref->usage_index,
+						   field->report_count -
+						   uref_multi->num_values);
+		}

 		switch (cmd) {

HID: hiddev: fix potential Spectre v1

[Greg Kroah-Hartman]

On Thu, Oct 18, 2018 at 01:50:26PM -0300, Breno Leitao wrote:
> Hi Gustavo,
> 
> On 10/17/2018 05:30 PM, Gustavo A. R. Silva wrote:
> > 
> > Hi Breno,
> > 
> > On 10/17/18 9:47 PM, Breno Leitao wrote:
> >> uref->usage_index can be indirectly controlled by userspace, hence leading
> >> to a potential exploitation of the Spectre variant 1 vulnerability.
> >>
> >> This problem might show up in the cmd = HIDIOCGCOLLECTIONINDEX flow at function
> >> hiddev_ioctl_usage(), where uref->usage_index is compared to field->maxusage
> >> and then used as an index to dereference field->usage array.
> >>
> >> This is a summary of the current flow, which matches the traditional
> >> Spectre V1 issue:
> >>
> >> 	copy_from_user(uref, user_arg, sizeof(*uref))
> >> 	if (uref->usage_index >= field->maxusage)
> >> 		goto inval;
> >> 	i = field->usage[uref->usage_index].collection_index;
> >> 	return i;
> >>
> >> This patch fixes this by sanitizing field uref->usage_index before using it to
> >> index field->usage, thus, avoiding speculation in the first load.
> >>
> >> Signed-off-by: Breno Leitao <leitao@debian.org>
> >> ---
> >>  drivers/hid/usbhid/hiddev.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> >> index 23872d08308c..8829cbc1f6b1 100644
> >> --- a/drivers/hid/usbhid/hiddev.c
> >> +++ b/drivers/hid/usbhid/hiddev.c
> >> @@ -512,6 +512,9 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
> >>  			if (cmd == HIDIOCGCOLLECTIONINDEX) {
> >>  				if (uref->usage_index >= field->maxusage)
> >>  					goto inval;
> >> +				uref->usage_index =
> >> +					array_index_nospec(uref->usage_index,
> >> +							   field->maxusage);
> > 
> > Good catch.
> > 
> >>  			} else if (uref->usage_index >= field->report_count)
> >>  				goto inval;
> > 
> > Although, notice that this is the same index, and it can be used to index field->value[]
> > at lines 526 and 532.
> 
> Right, this seems to be a possible problem also, when 'cmd' = HIDIOC{G,S}USAGES.
> 
> I am reworking the patch to cover both issues. What do you think of the draft
> below?
> 
> Thank you for reviewing it!
> 
> ---
> 
> Subject: [PATCH] HID: hiddev: fix potential Spectre v1
> 
> uref->usage_index can be indirectly controlled by userspace, hence leading
> to a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This field is used as an array index by the hiddev_ioctl_usage() function,
> when 'cmd' is HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or HIDIOCSUSAGES.
> 
> For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
> field->maxusage and then used as an index to dereference field->usage
> array.  The very same thing happens to the cmd == HIDIOC{G,S}USAGES cases,
> where uref->usage_index is checked against an array maximum value and then
> it is used as an index in this array.
> 
> This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
> traditional Spectre V1 first load:
> 
> 	copy_from_user(uref, user_arg, sizeof(*uref))
> 	if (uref->usage_index >= field->maxusage)
> 		goto inval;
> 	i = field->usage[uref->usage_index].collection_index;
> 	return i;
> 
> This patch fixes this by sanitizing field uref->usage_index before using it
> to index field->usage, thus, avoiding speculation in the first load.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/hid/usbhid/hiddev.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

Care to cc: stable as well?

thanks,

greg k-h