Hi Mauro,
On Mon, Mar 26, 2018 at 07:08:16AM -0300, Mauro Carvalho Chehab wrote:
> 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. To the original + this change:
Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
>
> 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;
> }
>
>
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
