| Message ID | 1539805628-16485-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HID: hiddev: fix potential Spectre v1 | expand |
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
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(-) 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:
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
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; }
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(+)