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@xxxxxxxxxxxxxxxx>
> ---
> 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;
> }
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
