| Message ID | 912d2f8228be077a1743adb797ada1dfcfe99c81.1521806166.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. 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. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 672ab22ccd96..a5dab16ff2d2 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -2833,14 +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); > > /* Copy arguments into temp kernel buffer */ > if (_IOC_DIR(cmd) != _IOC_NONE) { > - if (_IOC_SIZE(cmd) <= sizeof(sbuf)) { > + if (size <= sizeof(sbuf)) { > parg = sbuf; > } else { > /* too big to allocate from stack */ > - mbuf = kvmalloc(_IOC_SIZE(cmd), GFP_KERNEL); > + mbuf = kvmalloc(size, GFP_KERNEL); > if (NULL == mbuf) > return -ENOMEM; > parg = mbuf; > @@ -2848,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 = _IOC_SIZE(cmd); > + unsigned int n = size; > > /* > * In some cases, only a few fields are used as input, > @@ -2869,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 < _IOC_SIZE(cmd)) > - memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n); > + if (n < size) > + memset((u8 *)parg + n, 0, size - n); > } else { > /* read-only ioctl */ > - memset(parg, 0, _IOC_SIZE(cmd)); > + memset(parg, 0, size); > } > } > > @@ -2931,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, _IOC_SIZE(cmd))) > + if (copy_to_user((void __user *)arg, parg, size)) > err = -EFAULT; > break; > }
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 672ab22ccd96..a5dab16ff2d2 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -2833,14 +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); /* Copy arguments into temp kernel buffer */ if (_IOC_DIR(cmd) != _IOC_NONE) { - if (_IOC_SIZE(cmd) <= sizeof(sbuf)) { + if (size <= sizeof(sbuf)) { parg = sbuf; } else { /* too big to allocate from stack */ - mbuf = kvmalloc(_IOC_SIZE(cmd), GFP_KERNEL); + mbuf = kvmalloc(size, GFP_KERNEL); if (NULL == mbuf) return -ENOMEM; parg = mbuf; @@ -2848,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 = _IOC_SIZE(cmd); + unsigned int n = size; /* * In some cases, only a few fields are used as input, @@ -2869,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 < _IOC_SIZE(cmd)) - memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n); + if (n < size) + memset((u8 *)parg + n, 0, size - n); } else { /* read-only ioctl */ - memset(parg, 0, _IOC_SIZE(cmd)); + memset(parg, 0, size); } } @@ -2931,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, _IOC_SIZE(cmd))) + if (copy_to_user((void __user *)arg, parg, size)) err = -EFAULT; break; }
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. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> --- drivers/media/v4l2-core/v4l2-ioctl.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)