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@xxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxx>
> ---
> drivers/hid/usbhid/hiddev.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
Care to cc: stable as well?
thanks,
greg k-h
