* [bug report] media: v4l2-ctrls: split up into four source files
@ 2022-02-07 11:34 Dan Carpenter
2022-02-07 11:50 ` Hans Verkuil
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-02-07 11:34 UTC (permalink / raw)
To: hverkuil-cisco; +Cc: linux-media
[ 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.
375 GFP_KERNEL);
376 if (!helpers)
377 return -ENOMEM;
378 }
379
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] media: v4l2-ctrls: split up into four source files
2022-02-07 11:34 [bug report] media: v4l2-ctrls: split up into four source files Dan Carpenter
@ 2022-02-07 11:50 ` Hans Verkuil
2022-02-07 12:15 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2022-02-07 11:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
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.
Regards,
Hans
>
> 375 GFP_KERNEL);
> 376 if (!helpers)
> 377 return -ENOMEM;
> 378 }
> 379
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] media: v4l2-ctrls: split up into four source files
2022-02-07 11:50 ` Hans Verkuil
@ 2022-02-07 12:15 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-02-07 12:15 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-07 12:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-07 11:34 [bug report] media: v4l2-ctrls: split up into four source files Dan Carpenter
2022-02-07 11:50 ` Hans Verkuil
2022-02-07 12:15 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox