Em Fri, 23 Mar 2018 23:53:56 +0200
Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> escreveu:
> Hi Mauro,
>
> On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> > While the code there is right, it produces three false positives:
> > drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> > drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> > drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error: memset() 'parg' too small (128 vs 16383)
> >
> > Store the ioctl size on a cache var, in order to suppress those.
>
> I have to say I'm not a big fan of changing perfectly fine code in order to
> please static analysers.
Well, there's a side effect of this patch: it allows gcc to better
optimize the text size, with is good:
text data bss dec hex filename
34538 2320 0 36858 8ffa old/drivers/media/v4l2-core/v4l2-ioctl.o
34490 2320 0 36810 8fca new/drivers/media/v4l2-core/v4l2-ioctl.o
> What's this, smatch? I wonder if it could be fixed
> instead of changing the code. That'd be presumably a lot more work though.
Yes, the warnings came from smatch. No idea how easy/hard would be to
change it.
>
> On naming --- "size" is rather more generic, but it's not a long function
> either. I'd be a bit more specific, e.g. ioc_size or arg_size.
Agreed.
I'll add the enclosed patch changing it to ioc_size.
Thanks,
Mauro
[PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
Instead of just calling it as "size", let's name it as "ioc_size",
as it reflects better its contents.
As this is constant along the function, also mark it as const.
Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a5dab16ff2d2..f48c505550e0 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2833,15 +2833,15 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
size_t array_size = 0;
void __user *user_ptr = NULL;
void **kernel_ptr = NULL;
- size_t size = _IOC_SIZE(cmd);
+ const size_t ioc_size = _IOC_SIZE(cmd);
/* Copy arguments into temp kernel buffer */
if (_IOC_DIR(cmd) != _IOC_NONE) {
- if (size <= sizeof(sbuf)) {
+ if (ioc_size <= sizeof(sbuf)) {
parg = sbuf;
} else {
/* too big to allocate from stack */
- mbuf = kvmalloc(size, GFP_KERNEL);
+ mbuf = kvmalloc(ioc_size, GFP_KERNEL);
if (NULL == mbuf)
return -ENOMEM;
parg = mbuf;
@@ -2849,7 +2849,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
err = -EFAULT;
if (_IOC_DIR(cmd) & _IOC_WRITE) {
- unsigned int n = size;
+ unsigned int n = ioc_size;
/*
* In some cases, only a few fields are used as input,
@@ -2870,11 +2870,11 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
goto out;
/* zero out anything we don't copy from userspace */
- if (n < size)
- memset((u8 *)parg + n, 0, size - n);
+ if (n < ioc_size)
+ memset((u8 *)parg + n, 0, ioc_size - n);
} else {
/* read-only ioctl */
- memset(parg, 0, size);
+ memset(parg, 0, ioc_size);
}
}
@@ -2932,7 +2932,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
switch (_IOC_DIR(cmd)) {
case _IOC_READ:
case (_IOC_WRITE | _IOC_READ):
- if (copy_to_user((void __user *)arg, parg, size))
+ if (copy_to_user((void __user *)arg, parg, ioc_size))
err = -EFAULT;
break;
}
