- Don't relay on bFormatIndex as an internal array index, since this

field is unreliable and can start with any number.  Use an own internal
array index instead.

- If the read buffer is too small, return a propper error to the calling
functions.  Just check the buffer size if we use the read(2) method since
it doesn't affect mmap(2).

Fixes kernel crashes seen with the M$ LifeCam NX-6000 and internal
(laptop) Sonix chipsets.

Tested by jcs@ (Sonix) and myself (NX-6000).

OK deraadt@

diff --git a/sys/dev/usb/uvideo.c b/sys/dev/usb/uvideo.c
index b4f8e23..fba39e2 100644 (file)
--- a/sys/dev/usb/uvideo.c
+++ b/sys/dev/usb/uvideo.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: uvideo.c,v 1.65 2008/07/29 13:45:04 mglocker Exp $ */
+/*     $OpenBSD: uvideo.c,v 1.66 2008/07/31 15:26:25 mglocker Exp $ */
 
 /*
  * Copyright (c) 2008 Robert Nagy <robert@openbsd.org>
@@ -80,9 +80,9 @@ int           uvideo_vs_parse_desc_format_uncompressed(struct uvideo_softc *,
                    const usb_descriptor_t *);
 int            uvideo_vs_parse_desc_frame(struct uvideo_softc *);
 int            uvideo_vs_parse_desc_frame_mjpeg(struct uvideo_softc *,
-                   const usb_descriptor_t *, int *);
+                   const usb_descriptor_t *);
 int            uvideo_vs_parse_desc_frame_uncompressed(struct uvideo_softc *,
-                   const usb_descriptor_t *, int *);
+                   const usb_descriptor_t *);
 int            uvideo_vs_parse_desc_alt(struct uvideo_softc *,
                    struct usb_attach_arg *uaa, int, int, int);
 int            uvideo_vs_set_alt(struct uvideo_softc *, usbd_interface_handle,
@@ -166,7 +166,7 @@ int         uvideo_try_fmt(void *, struct v4l2_format *);
  */
 caddr_t                uvideo_mappage(void *, off_t, int);
 int            uvideo_get_bufsize(void *);
-void           uvideo_start_read(void *);
+int            uvideo_start_read(void *);
 
 #define DEVNAME(_s) ((_s)->sc_dev.dv_xname)
 
@@ -617,6 +617,8 @@ uvideo_vs_parse_desc_format(struct uvideo_softc *sc)
                desc = usb_desc_iter_next(&iter);
        }
 
+       sc->sc_fmtgrp_idx = 0;
+
        return (0);
 }
 
@@ -634,21 +636,23 @@ uvideo_vs_parse_desc_format_mjpeg(struct uvideo_softc *sc,
                return (-1);
        }
 
-       if (d->bFormatIndex == UVIDEO_MAX_FORMAT) {
+       if (sc->sc_fmtgrp_idx > UVIDEO_MAX_FORMAT) {
                printf("%s: too many MJPEG format descriptors found!\n",
                    DEVNAME(sc));
                return (-1);
        }
-       sc->sc_fmtgrp[d->bFormatIndex].format = (struct uvideo_format_desc *)d;
-       sc->sc_fmtgrp[d->bFormatIndex].format_dfidx =
-           sc->sc_fmtgrp[d->bFormatIndex].format->u.mjpeg.bDefaultFrameIndex;
 
-       sc->sc_fmtgrp[d->bFormatIndex].pixelformat = V4L2_PIX_FMT_MJPEG;
+       sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format =
+           (struct uvideo_format_desc *)d;
+       sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format_dfidx =
+           sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format->u.mjpeg.bDefaultFrameIndex;
+       sc->sc_fmtgrp[sc->sc_fmtgrp_idx].pixelformat = V4L2_PIX_FMT_MJPEG;
 
        if (sc->sc_fmtgrp_cur == NULL)
                /* set MJPEG format */
-               sc->sc_fmtgrp_cur = &sc->sc_fmtgrp[d->bFormatIndex];
+               sc->sc_fmtgrp_cur = &sc->sc_fmtgrp[sc->sc_fmtgrp_idx];
 
+       sc->sc_fmtgrp_idx++;
        sc->sc_fmtgrp_num++;
 
        return (0);
@@ -669,16 +673,17 @@ uvideo_vs_parse_desc_format_uncompressed(struct uvideo_softc *sc,
                return (-1);
        }
 
-       if (d->bFormatIndex == UVIDEO_MAX_FORMAT) {
+       if (sc->sc_fmtgrp_idx > UVIDEO_MAX_FORMAT) {
                printf("%s: too many UNCOMPRESSED format descriptors found!\n",
                    DEVNAME(sc));
                return (-1);
        }
-       sc->sc_fmtgrp[d->bFormatIndex].format = (struct uvideo_format_desc *)d;
-       sc->sc_fmtgrp[d->bFormatIndex].format_dfidx =
-           sc->sc_fmtgrp[d->bFormatIndex].format->u.uc.bDefaultFrameIndex;
 
-       i = d->bFormatIndex;
+       sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format =
+           (struct uvideo_format_desc *)d;
+       sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format_dfidx =
+           sc->sc_fmtgrp[sc->sc_fmtgrp_idx].format->u.uc.bDefaultFrameIndex;
+       i = sc->sc_fmtgrp_idx;
        if (!strcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, "YUY2")) {
                sc->sc_fmtgrp[i].pixelformat = V4L2_PIX_FMT_YUYV;
        } else if (!strcmp(sc->sc_fmtgrp[i].format->u.uc.guidFormat, "NV12")) {
@@ -689,8 +694,9 @@ uvideo_vs_parse_desc_format_uncompressed(struct uvideo_softc *sc,
 
        if (sc->sc_fmtgrp_cur == NULL)
                /* set UNCOMPRESSED format */
-               sc->sc_fmtgrp_cur = &sc->sc_fmtgrp[d->bFormatIndex];
+               sc->sc_fmtgrp_cur = &sc->sc_fmtgrp[sc->sc_fmtgrp_idx];
 
+       sc->sc_fmtgrp_idx++;
        sc->sc_fmtgrp_num++;
 
        return (0);
@@ -701,7 +707,6 @@ uvideo_vs_parse_desc_frame(struct uvideo_softc *sc)
 {
        usbd_desc_iter_t iter;
        const usb_descriptor_t *desc;
-       int fmtidx = 0;
 
        DPRINTF(1, "%s: %s\n", DEVNAME(sc), __func__);
 
@@ -715,14 +720,14 @@ uvideo_vs_parse_desc_frame(struct uvideo_softc *sc)
 
                switch (desc->bDescriptorSubtype) {
                case UDESCSUB_VS_FRAME_MJPEG:
-                       if (uvideo_vs_parse_desc_frame_mjpeg(sc, desc, &fmtidx))
+                       if (uvideo_vs_parse_desc_frame_mjpeg(sc, desc))
                                return (1);
                        break;
                case UDESCSUB_VS_FRAME_UNCOMPRESSED:
                        /* XXX do correct length calculation */
                        if (desc->bLength > 25) {
                                if (uvideo_vs_parse_desc_frame_uncompressed(sc,
-                                   desc, &fmtidx))
+                                   desc))
                                        return (1);
                        }
                        break;
@@ -736,58 +741,62 @@ uvideo_vs_parse_desc_frame(struct uvideo_softc *sc)
 
 int
 uvideo_vs_parse_desc_frame_mjpeg(struct uvideo_softc *sc,
-    const usb_descriptor_t *desc, int *fmtidx)
+    const usb_descriptor_t *desc)
 {
        struct usb_video_frame_mjpeg_desc *d;
+       int fmtidx;
 
        d = (struct usb_video_frame_mjpeg_desc *)(uint8_t *)desc;
 
-       if (d->bFrameIndex == 1)
-               ++*fmtidx;
-
        if (d->bFrameIndex == UVIDEO_MAX_FRAME) {
                printf("%s: too many MJPEG frame descriptors found!\n",
                    DEVNAME(sc));
                return (1);
        }
-       sc->sc_fmtgrp[*fmtidx].frame[d->bFrameIndex] = d;
+
+       fmtidx = sc->sc_fmtgrp_idx;
+       sc->sc_fmtgrp[fmtidx].frame[d->bFrameIndex] = d;
 
        /*
         * If bDefaultFrameIndex is not set by the device
         * use the first bFrameIndex available, otherwise
         * set it to the default one.
         */
-       if (sc->sc_fmtgrp[*fmtidx].format->u.mjpeg.bDefaultFrameIndex == 0) {
-               sc->sc_fmtgrp[*fmtidx].frame_cur =
-                   sc->sc_fmtgrp[*fmtidx].frame[1];
-       } else if (sc->sc_fmtgrp[*fmtidx].format->u.mjpeg.bDefaultFrameIndex ==
+       if (sc->sc_fmtgrp[fmtidx].format->u.mjpeg.bDefaultFrameIndex == 0) {
+               sc->sc_fmtgrp[fmtidx].frame_cur =
+                   sc->sc_fmtgrp[fmtidx].frame[1];
+       } else if (sc->sc_fmtgrp[fmtidx].format->u.mjpeg.bDefaultFrameIndex ==
            d->bFrameIndex) {
-               sc->sc_fmtgrp[*fmtidx].frame_cur =
-                   sc->sc_fmtgrp[*fmtidx].frame[d->bFrameIndex];
+               sc->sc_fmtgrp[fmtidx].frame_cur =
+                   sc->sc_fmtgrp[fmtidx].frame[d->bFrameIndex];
        }
 
-       sc->sc_fmtgrp[*fmtidx].frame_num++;
+       sc->sc_fmtgrp[fmtidx].frame_num++;
+
+       if (d->bFrameIndex ==
+           sc->sc_fmtgrp[fmtidx].format->bNumFrameDescriptors)
+               sc->sc_fmtgrp_idx++;
 
        return (0);
 }
 
 int
 uvideo_vs_parse_desc_frame_uncompressed(struct uvideo_softc *sc,
-    const usb_descriptor_t *desc, int *fmtidx)
+    const usb_descriptor_t *desc)
 {
        struct usb_video_frame_uncompressed_desc *d;
+       int fmtidx;
 
        d = (struct usb_video_frame_uncompressed_desc *)(uint8_t *)desc;
 
-       if (d->bFrameIndex == 1)
-               ++*fmtidx;
-
        if (d->bFrameIndex == UVIDEO_MAX_FRAME) {
                printf("%s: too many UNCOMPRESSED frame descriptors found!\n",
                    DEVNAME(sc));
                return (1);
        }
-       sc->sc_fmtgrp[*fmtidx].frame[d->bFrameIndex] =
+
+       fmtidx = sc->sc_fmtgrp_idx;
+       sc->sc_fmtgrp[fmtidx].frame[d->bFrameIndex] =
            (struct usb_video_frame_mjpeg_desc *)d;
 
        /*
@@ -795,16 +804,20 @@ uvideo_vs_parse_desc_frame_uncompressed(struct uvideo_softc *sc,
         * use the first bFrameIndex available, otherwise
         * set it to the default one.
         */
-       if (sc->sc_fmtgrp[*fmtidx].format->u.uc.bDefaultFrameIndex == 0) {
-               sc->sc_fmtgrp[*fmtidx].frame_cur =
-                   sc->sc_fmtgrp[*fmtidx].frame[1];
-       } else if (sc->sc_fmtgrp[*fmtidx].format->u.uc.bDefaultFrameIndex ==
+       if (sc->sc_fmtgrp[fmtidx].format->u.uc.bDefaultFrameIndex == 0) {
+               sc->sc_fmtgrp[fmtidx].frame_cur =
+                   sc->sc_fmtgrp[fmtidx].frame[1];
+       } else if (sc->sc_fmtgrp[fmtidx].format->u.uc.bDefaultFrameIndex ==
            d->bFrameIndex) {
-               sc->sc_fmtgrp[*fmtidx].frame_cur =
-                   sc->sc_fmtgrp[*fmtidx].frame[d->bFrameIndex];
+               sc->sc_fmtgrp[fmtidx].frame_cur =
+                   sc->sc_fmtgrp[fmtidx].frame[d->bFrameIndex];
        }
 
-       sc->sc_fmtgrp[*fmtidx].frame_num++;
+       sc->sc_fmtgrp[fmtidx].frame_num++;
+
+       if (d->bFrameIndex ==
+           sc->sc_fmtgrp[fmtidx].format->bNumFrameDescriptors)
+               sc->sc_fmtgrp_idx++;
 
        return (0);
 }
@@ -1170,7 +1183,8 @@ uvideo_vs_alloc_sample(struct uvideo_softc *sc)
        fb->buf_size = UGETDW(sc->sc_desc_probe.dwMaxVideoFrameSize);
 
        /* don't overflow the upper layer sample buffer */
-       if (sc->sc_max_fbuf_size < fb->buf_size) {
+       if (sc->sc_max_fbuf_size < fb->buf_size &&
+           sc->sc_mmap_flag == 0) {
                printf("%s: sofware video buffer is too small!\n", DEVNAME(sc));
                return (USBD_NOMEM);
        }
@@ -2099,10 +2113,10 @@ uvideo_enum_fmt(void *v, struct v4l2_fmtdesc *fmtdesc)
                /* type not supported */
                return (EINVAL);
 
-       idx = fmtdesc->index + 1;
-       if (idx > sc->sc_fmtgrp_num) 
+       if (fmtdesc->index == sc->sc_fmtgrp_num) 
                /* no more formats left */
                return (EINVAL);
+       idx = fmtdesc->index;
 
        switch (sc->sc_fmtgrp[idx].format->bDescriptorSubtype) {
        case UDESCSUB_VS_FORMAT_MJPEG:
@@ -2160,7 +2174,7 @@ uvideo_s_fmt(void *v, struct v4l2_format *fmt)
            DEVNAME(sc), __func__, fmt->fmt.pix.width, fmt->fmt.pix.height);
 
        /* search requested pixel format */
-       for (found = 0, i = 1; i <= sc->sc_fmtgrp_num; i++) {
+       for (found = 0, i = 0; i < sc->sc_fmtgrp_num; i++) {
                if (fmt->fmt.pix.pixelformat == sc->sc_fmtgrp[i].pixelformat) {
                        found = 1;
                        break;
@@ -2357,8 +2371,12 @@ int
 uvideo_streamon(void *v, int type)
 {
        struct uvideo_softc *sc = v;
+       usbd_status error;
+
+       error = uvideo_vs_init(sc);
+       if (error != USBD_NORMAL_COMPLETION)
+               return (EINVAL);
 
-       uvideo_vs_init(sc);
        uvideo_vs_start(sc);
 
        return (0);
@@ -2388,7 +2406,7 @@ uvideo_try_fmt(void *v, struct v4l2_format *fmt)
            DEVNAME(sc), __func__, fmt->fmt.pix.width, fmt->fmt.pix.height);
 
        /* search requested pixel format */
-       for (found = 0, i = 1; i <= sc->sc_fmtgrp_num; i++) {
+       for (found = 0, i = 0; i < sc->sc_fmtgrp_num; i++) {
                if (fmt->fmt.pix.pixelformat == sc->sc_fmtgrp[i].pixelformat) {
                        found = 1;
                        break;
@@ -2440,22 +2458,29 @@ uvideo_get_bufsize(void *v)
        /* find the maximum frame size */
        bzero(probe_data, sizeof(probe_data));
        error = uvideo_vs_get_probe(sc, probe_data, GET_MAX);
-       if (error != USBD_NORMAL_COMPLETION)
+       if (error != USBD_NORMAL_COMPLETION) {
                return (EINVAL);
+       }
 
        sc->sc_max_fbuf_size = UGETDW(pc->dwMaxVideoFrameSize);
 
        return (sc->sc_max_fbuf_size);
 }
 
-void
+int
 uvideo_start_read(void *v)
 {
        struct uvideo_softc *sc = v;
+       usbd_status error;
 
        if (sc->sc_mmap_flag)
                sc->sc_mmap_flag = 0;
 
-       uvideo_vs_init(sc);
+       error = uvideo_vs_init(sc);
+       if (error != USBD_NORMAL_COMPLETION)
+               return (EINVAL);
+
        uvideo_vs_start(sc);
+
+       return (0);
 }

diff --git a/sys/dev/usb/uvideo.h b/sys/dev/usb/uvideo.h
index dcb55cc..21b9aa5 100644 (file)
--- a/sys/dev/usb/uvideo.h
+++ b/sys/dev/usb/uvideo.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: uvideo.h,v 1.25 2008/07/26 11:42:43 mglocker Exp $ */
+/*     $OpenBSD: uvideo.h,v 1.26 2008/07/31 15:26:25 mglocker Exp $ */
 
 /*
  * Copyright (c) 2007 Robert Nagy <robert@openbsd.org>
@@ -501,6 +501,7 @@ struct uvideo_softc {
        struct usb_video_input_header_desc_all   sc_desc_vs_input_header;
 
 #define UVIDEO_MAX_FORMAT                       8
+       int                                      sc_fmtgrp_idx;
        int                                      sc_fmtgrp_num;
        struct uvideo_format_group              *sc_fmtgrp_cur;
        struct uvideo_format_group               sc_fmtgrp[UVIDEO_MAX_FORMAT];

diff --git a/sys/dev/video.c b/sys/dev/video.c
index f2be15e..dd741a8 100644 (file)
--- a/sys/dev/video.c
+++ b/sys/dev/video.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: video.c,v 1.19 2008/07/26 11:42:43 mglocker Exp $     */
+/*     $OpenBSD: video.c,v 1.20 2008/07/31 15:26:25 mglocker Exp $     */
 /*
  * Copyright (c) 2008 Robert Nagy <robert@openbsd.org>
  * Copyright (c) 2008 Marcus Glocker <mglocker@openbsd.org>
@@ -146,8 +146,10 @@ videoread(dev_t dev, struct uio *uio, int ioflag)
 
        /* start the stream */
        if (sc->hw_if->start_read && !sc->sc_start_read) {
+               error = sc->hw_if->start_read(sc->hw_hdl);
+               if (error)
+                       return (error);
                sc->sc_start_read = 1;
-               sc->hw_if->start_read(sc->hw_hdl);
        }
 
        DPRINTF(("resid=%d\n", uio->uio_resid));

diff --git a/sys/dev/video_if.h b/sys/dev/video_if.h
index cc5c744..69ff317 100644 (file)
--- a/sys/dev/video_if.h
+++ b/sys/dev/video_if.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: video_if.h,v 1.13 2008/06/13 05:00:32 mglocker Exp $  */
+/*     $OpenBSD: video_if.h,v 1.14 2008/07/31 15:26:25 mglocker Exp $  */
 /*
  * Copyright (c) 2008 Robert Nagy <robert@openbsd.org>
  * Copyright (c) 2008 Marcus Glocker <mglocker@openbsd.org>
@@ -52,7 +52,7 @@ struct video_hw_if {
 
        /* other functions */
        int     (*get_bufsize)(void *);
-       void    (*start_read)(void *);
+       int     (*start_read)(void *);
 };
 
 struct video_attach_args {