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 <lei...@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 <lei...@debian.org>
---
drivers/hid/usbhid/hiddev.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
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) {
case HIDIOCGUSAGE:
