Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Mon, Jan 8, 2018 at 3:23 AM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Saturday, 6 January 2018 03:10:32 EET Dan Williams wrote:
>> Static analysis reports that 'index' may be a user controlled value that
>> is used as a data dependency to read 'pin' from the
>> 'selector->baSourceID' array. In order to avoid potential leaks of
>> kernel memory values, block speculative execution of the instruction
>> stream that could issue reads based on an invalid value of 'pin'.
>
> I won't repeat the arguments already made in the thread regarding having
> documented coverity rules for this, even if I agree with them.
>
>> Based on an original patch by Elena Reshetova.
>>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>> Cc: linux-media@xxxxxxxxxxxxxxx
>> Signed-off-by: Elena Reshetova <elena.reshetova@xxxxxxxxx>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>>  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index 3e7e283a44a8..7442626dc20e 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/mm.h>
>>  #include <linux/wait.h>
>>  #include <linux/atomic.h>
>> +#include <linux/compiler.h>
>>
>>  #include <media/v4l2-common.h>
>>  #include <media/v4l2-ctrls.h>
>> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void
>> *fh, struct uvc_entity *iterm = NULL;
>>       u32 index = input->index;
>>       int pin = 0;
>> +     __u8 *elem;
>>
>>       if (selector == NULL ||
>>           (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
>> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void
>> *fh, break;
>>               }
>>               pin = iterm->id;
>> -     } else if (index < selector->bNrInPins) {
>> -             pin = selector->baSourceID[index];
>> +     } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
>> +                                     selector->bNrInPins))) {
>> +             pin = *elem;
>>               list_for_each_entry(iterm, &chain->entities, chain) {
>>                       if (!UVC_ENTITY_IS_ITERM(iterm))
>>                               continue;
>
> (adding a bit more context)
>
>>                       if (iterm->id == pin)
>>                               break;
>>               }
>>       }
>>
>>       if (iterm == NULL || iterm->id != pin)
>>               return -EINVAL;
>>
>>       memset(input, 0, sizeof(*input));
>>       input->index = index;
>>       strlcpy(input->name, iterm->name, sizeof(input->name));
>>       if (UVC_ENTITY_TYPE(iterm) == UVC_ITT_CAMERA)
>>               input->type = V4L2_INPUT_TYPE_CAMERA;
>
> So pin is used to search for an entry in the chain->entities list. Entries in
> that list are allocated separately through kmalloc and can thus end up in
> different cache lines, so I agree we have an issue. However, this is mitigated
> by the fact that typical UVC devices have a handful (sometimes up to a dozen)
> entities, so an attacker would only be able to read memory values that are
> equal to the entity IDs used by the device. Entity IDs can be freely allocated
> but typically count continuously from 0. It would take a specially-crafted UVC
> device to be able to read all memory.
>
> On the other hand, as this is nowhere close to being a fast path, I think we
> can close this potential hole as proposed in the patch. So,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

Thanks Laurent!

> Will you merge the whole series in one go, or would you like me to take the
> patch in my tree ? In the latter case I'll wait until the nospec_array_ptr()
> gets merged in mainline.

I'll track it for now. Until the 'nospec_array_ptr()' discussion
resolves there won't be a stabilized commit-id for you to base a
branch.