linux-kernel - Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YFFiizDjNBMG3uI+@google.com>
Date:   Wed, 17 Mar 2021 10:59:39 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Tomasz Figa <tomasz.figa@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media <linux-media@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > -static int uvc_ioctl_g_selection(struct file *file, void *fh,
> > -                                struct v4l2_selection *sel)
> > +/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
> > +struct uvc_roi_rect {
> > +       __u16                   top;
> > +       __u16                   left;
> > +       __u16                   bottom;
> > +       __u16                   right;
> > +};
> 
> Perhaps __packed; ?

Perhaps.

> > +static int uvc_ioctl_g_selection(struct file *file, void *fh,
> > +                                struct v4l2_selection *sel)
> > +{
> > +       struct uvc_fh *handle = fh;
> > +       struct uvc_streaming *stream = handle->stream;
> > +
> > +       if (sel->type != stream->type)
> > +               return -EINVAL;
> > +
> > +       switch (sel->target) {
> > +       case V4L2_SEL_TGT_CROP_DEFAULT:
> > +       case V4L2_SEL_TGT_CROP_BOUNDS:
> > +       case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +       case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +               return uvc_ioctl_g_sel_target(file, fh, sel);
> > +       case V4L2_SEL_TGT_ROI_CURRENT:
> > +       case V4L2_SEL_TGT_ROI_DEFAULT:
> > +       case V4L2_SEL_TGT_ROI_BOUNDS:
> > +               return uvc_ioctl_g_roi_target(file, fh, sel);
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> 
> Are you sure that there is no lock needed between the control and the
> selection API?

Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
Hmm. They write to different offsets and don't seem to be overlapping.

> > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > +                               struct v4l2_selection *sel)
> > +{
> > +       bool ok = true;
> > +
> > +       if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> > +           sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> > +               return false;
> > +
> > +       /* perhaps also can test against ROI GET_MAX */
> > +
> > +       mutex_lock(&stream->mutex);
[...]
> > +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > +               ok = false;
> > +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > +               ok = false;

> Maybe you should not release this mutex until query_ctrl is done?

Maybe... These two tests are somewhat made up. Not sure if we need them.
On one hand it's reasonable to keep ROI within the frames. On the other
hand - such relation is not spelled out in the spec. If the stream change
its frame dimensions ROI is not getting invalidated or re-calculated by
the firmware. UVC spec says that ROI should be bounded only by the
CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
WINDOW_CONTROL.

So maybe I can remove stream->cuf_frame boundaries check instead.

> > +       mutex_unlock(&stream->mutex);
> > +
> > +       return ok;
> > +}
> > +
> > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > +                          struct v4l2_selection *sel)
> > +{
> > +       struct uvc_fh *handle = fh;
> > +       struct uvc_streaming *stream = handle->stream;
> > +       struct uvc_roi_rect *roi;
> > +       int ret;
> > +
> > +       if (!validate_roi_bounds(stream, sel))
> > +               return -E2BIG;
> > +
> > +       roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> > +       if (!roi)
> > +               return -ENOMEM;
> > +
> > +       roi->left       = sel->r.left;
> > +       roi->top        = sel->r.top;
> > +       roi->right      = sel->r.width;
> > +       roi->bottom     = sel->r.height;
> > +
> > +       ret = uvc_query_ctrl(stream->dev, UVC_SET_CUR, 1, stream->dev->intfnum,
> > +                            UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> > +                            sizeof(struct uvc_roi_rect));
> 
> I think you need to read back from the device the actual value

GET_CUR?

> https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> On success the struct v4l2_rect r field contains the adjusted
> rectangle.

What is the adjusted rectangle here? Does this mean that firmware can
successfully apply SET_CUR and return 0, but in reality it was not happy
with the rectangle dimensions so it modified it behind the scenes?

I can add GET_CUR I guess, but do we want to double the traffic, given
that ROI SET_CUR can be executed relatively often?