Re: [bug report] media: v4l2-ctrls: split up into four source files

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Feb 07, 2022 at 12:50:41PM +0100, Hans Verkuil wrote:
> Hi Dan,
> 
> On 2/7/22 12:34, Dan Carpenter wrote:
> > [ This code is older, but it showed up as a new warning because of
> >   moving the files around - dan ]
> > 
> > Hello Hans Verkuil,
> > 
> > The patch 71c689dc2e73: "media: v4l2-ctrls: split up into four source
> > files" from Apr 27, 2021, leads to the following Smatch static
> > checker warning:
> > 
> > drivers/media/v4l2-core/v4l2-ctrls-api.c:374 v4l2_g_ext_ctrls_common() warn: uncapped user size for kvmalloc() will WARN
> > drivers/media/v4l2-core/v4l2-ctrls-api.c:545 try_set_ext_ctrls_common() warn: uncapped user size for kvmalloc() will WARN
> > 
> > drivers/media/v4l2-core/v4l2-ctrls-api.c
> >     351 int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
> >     352                             struct v4l2_ext_controls *cs,
> >     353                             struct video_device *vdev)
> >     354 {
> >     355         struct v4l2_ctrl_helper helper[4];
> >     356         struct v4l2_ctrl_helper *helpers = helper;
> >     357         int ret;
> >     358         int i, j;
> >     359         bool is_default, is_request;
> >     360 
> >     361         is_default = (cs->which == V4L2_CTRL_WHICH_DEF_VAL);
> >     362         is_request = (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL);
> >     363 
> >     364         cs->error_idx = cs->count;
> >     365         cs->which = V4L2_CTRL_ID2WHICH(cs->which);
> >     366 
> >     367         if (!hdl)
> >     368                 return -EINVAL;
> >     369 
> >     370         if (cs->count == 0)
> >     371                 return class_check(hdl, cs->which);
> >     372 
> >     373         if (cs->count > ARRAY_SIZE(helper)) {
> > --> 374                 helpers = kvmalloc_array(cs->count, sizeof(helper[0]),
> > 
> > These days if "cs->count" is larger than INT_MAX it will trigger a
> > WARN() because basically "people shouldn't be so trusting of user
> > space".  kvmalloc() used to be able to allocate more than INT_MAX but
> > that led to integer overflow problmes and security bugs.
> > 
> > This "cs->count" value comes from the user via the ioctl.  I don't know
> > what a sensible upper bound is though.
> 
> cs->count is capped to 1024 in drivers/media/v4l2-core/v4l2-ioctl.c.
> Search for V4L2_CID_MAX_CTRLS.
> 
> There is no way for smatch to know this of course.

Yeah.  It's tricky.  The switch statement in check_array_args() is too
much data for Smatch and the void *arg confuses Smatch as well.  :/

Thanks for looking at this.

regards,
dan carpenter