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

[Dan Williams <dan.j.williams@intel.com>]

On Sat, Jan 6, 2018 at 1:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
>> On Fri, Jan 05, 2018 at 05:10:32PM -0800, 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'.
>> >
>> > Based on an original patch by Elena Reshetova.
>> >
>> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> > Cc: linux-media@vger.kernel.org
>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> >  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;
>>
>> I dug through this before, and I couldn't find where index came from
>> userspace, I think seeing the coverity rule would be nice.
>
> Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
> crazy complex (rightfully so), it's amazing that coverity could navigate
> that whole thing :)
>
> While I'm all for fixing this type of thing, I feel like we need to do
> something "else" for this as playing whack-a-mole for this pattern is
> going to be a never-ending battle for all drivers for forever.  Either
> we need some way to mark this data path to make it easy for tools like
> sparse to flag easily, or we need to catch the issue in the driver
> subsystems, which unfortunatly, would harm the drivers that don't have
> this type of issue (like here.)
>
> I'm guessing that other operating systems, which don't have the luxury
> of auditing all of their drivers are going for the "big hammer in the
> subsystem" type of fix, right?
>
> I don't have a good answer for this, but if there was some better way to
> rewrite these types of patterns to just prevent the need for the
> nospec_array_ptr() type thing, that might be the best overall for
> everyone.  Much like ebpf did with their changes.  That way a simple
> coccinelle rule would be able to catch the pattern and rewrite it.
>
> Or am I just dreaming?

At least on the coccinelle front you're dreaming. Julia already took a
look and said:

"I don't think Coccinelle would be good for doing this (ie
implementing taint analysis) because the dataflow is too complicated."

Perhaps the Coverity instance Dave mentioned at Ksummit 2012 has a
role to play here?