Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181217210222.115419-1-astrachan@google.com> <45456214.XvNNoR8qLh@avalon>
In-Reply-To: <45456214.XvNNoR8qLh@avalon>
From:   Alistair Strachan <astrachan@google.com>
Date:   Tue, 18 Dec 2018 17:31:25 -0800
Message-ID: <CANDihLEy2BRgm5o2tQJQc_cQpbKxUgH8CWjK51qwXr67-CMn4g@mail.gmail.com>
Subject: Re: [PATCH] media: uvcvideo: Fix 'type' check leading to overflow
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     linux-kernel@vger.kernel.org, syzbot <syzkaller@googlegroups.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Laurent,

On Tue, Dec 18, 2018 at 1:42 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Alistair,
>
> Thank you for the patch.
>
> On Monday, 17 December 2018 23:02:22 EET Alistair Strachan wrote:
> > When initially testing the Camera Terminal Descriptor wTerminalType
> > field (buffer[4]), no mask is used. Later in the function, the MSB is
> > overloaded to store the descriptor subtype, and so a mask of 0x7fff
> > is used to check the type.
> >
> > If a descriptor is specially crafted to set this overloaded bit in the
> > original wTerminalType field, the initial type check will fail (falling
> > through, without adjusting the buffer size), but the later type checks
> > will pass, assuming the buffer has been made suitably large, causing an
> > overflow.
> >
> > This problem could be resolved in a few different ways, but this fix
> > applies the same initial type check as used by UVC_ENTITY_TYPE (once we
> > have a 'term' structure.) Such crafted wTerminalType fields will then be
> > treated as *generic* Input Terminals, not as CAMERA or
> > MEDIA_TRANSPORT_INPUT, avoiding an overflow.
> >
> > Originally reported here:
> > https://groups.google.com/forum/#!topic/syzkaller/Ot1fOE6v1d8
> > A similar (non-compiling) patch was provided at that time.
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Signed-off-by: Alistair Strachan <astrachan@google.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Cc: kernel-team@android.com
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index bc369a0934a3..279a967b8264
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1082,11 +1082,11 @@ static int uvc_parse_standard_control(struct
> > uvc_device *dev, p = 0;
> >               len = 8;
> >
> > -             if (type == UVC_ITT_CAMERA) {
> > +             if ((type & 0x7fff) == UVC_ITT_CAMERA) {
> >                       n = buflen >= 15 ? buffer[14] : 0;
> >                       len = 15;
> >
> > -             } else if (type == UVC_ITT_MEDIA_TRANSPORT_INPUT) {
> > +             } else if ((type & 0x7fff) == UVC_ITT_MEDIA_TRANSPORT_INPUT) {
> >                       n = buflen >= 9 ? buffer[8] : 0;
> >                       p = buflen >= 10 + n ? buffer[9+n] : 0;
> >                       len = 10;
>
> How about rejecting invalid types instead ? Something along the lines of

This looks simpler/better to me. I just re-sent your version of the
change (build + runtime tested.) Thanks for reviewing.

> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index b62cbd800111..33a22c016456 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1106,11 +1106,19 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>                         return -EINVAL;
>                 }
>
> -               /* Make sure the terminal type MSB is not null, otherwise it
> -                * could be confused with a unit.
> +               /*
> +                * Reject invalid terminal types that would cause issues:
> +                *
> +                * - The high byte must be non-zero, otherwise it would be
> +                *   confused with a unit.
> +                *
> +                * - Bit 15 must be 0, as we use it internally as a terminal
> +                *   direction flag.
> +                *
> +                * Other unknown types are accepted.
>                  */
>                 type = get_unaligned_le16(&buffer[4]);
> -               if ((type & 0xff00) == 0) {
> +               if ((type & 0x7f00) == 0 || (type & 0x8000) != 0) {
>                         uvc_trace(UVC_TRACE_DESCR, "device %d videocontrol "
>                                 "interface %d INPUT_TERMINAL %d has invalid "
>                                 "type 0x%04x, skipping\n", udev->devnum,
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>