lkml.org 
[lkml]   [2018]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution
    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?

    \
     
     \ /
      Last update: 2018-01-06 18:41    [W:2.618 / U:11.912 seconds]
    ©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site