diff mbox series

media: v4l2-ioctl: explicitly initialize argument buffer

Message ID 9c393beb-c45b-6dc3-9955-867c6abffdc4@I-love.SAKURA.ne.jp
State New
Headers show
Series media: v4l2-ioctl: explicitly initialize argument buffer | expand

Commit Message

Tetsuo Handa June 18, 2021, 10:34 a.m. UTC
KMSAN complains that ioctl(VIDIOC_QUERYBUF_TIME32) copies uninitialized
kernel stack memory to userspace [1], for video_usercopy() calls
copy_to_user() even if __video_do_ioctl() returned -EINVAL error.

Generally, copy_to_user() needn't be called when there was an error.
But video_usercopy() has always_copy logic which forces copy_to_user().
Therefore, instead of not calling copy_to_user(), explicitly initialize
argument buffer.

  ----------
  /* Compile for 32bit userspace and run on 64bit kernel. */
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <fcntl.h>
  #include <sys/ioctl.h>
  #define VIDIOC_QUERYBUF_TIME32 0xc0505609

  int main(int argc, char *argv[])
  {
          char buf[128] = { };

          ioctl(open("/dev/video0", O_RDONLY), VIDIOC_QUERYBUF_TIME32, &buf);
          return 0;
  }
  ----------

Link: https://syzkaller.appspot.com/bug?id=eb945b02a7b3060a8a60dab673c02f3ab20a048b [1]
Reported-by: syzbot <syzbot+142888ffec98ab194028@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann June 18, 2021, 10:41 a.m. UTC | #1
On Fri, Jun 18, 2021 at 12:34 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> KMSAN complains that ioctl(VIDIOC_QUERYBUF_TIME32) copies uninitialized
> kernel stack memory to userspace [1], for video_usercopy() calls
> copy_to_user() even if __video_do_ioctl() returned -EINVAL error.
>
> Generally, copy_to_user() needn't be called when there was an error.
> But video_usercopy() has always_copy logic which forces copy_to_user().
> Therefore, instead of not calling copy_to_user(), explicitly initialize
> argument buffer.
>
>   ----------
>   /* Compile for 32bit userspace and run on 64bit kernel. */
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   #include <fcntl.h>
>   #include <sys/ioctl.h>
>   #define VIDIOC_QUERYBUF_TIME32 0xc0505609
>
>   int main(int argc, char *argv[])
>   {
>           char buf[128] = { };
>
>           ioctl(open("/dev/video0", O_RDONLY), VIDIOC_QUERYBUF_TIME32, &buf);
>           return 0;
>   }
>   ----------
>
> Link: https://syzkaller.appspot.com/bug?id=eb945b02a7b3060a8a60dab673c02f3ab20a048b [1]
> Reported-by: syzbot <syzbot+142888ffec98ab194028@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

This should no longer be necessary once my recent patches propagate
into linux-next,
see https://patchwork.linuxtv.org/project/linux-media/list/?series=5678&state=*

      Arnd
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 2673f51aafa4..ba204e0200d3 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3240,7 +3240,7 @@  long
 video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 	       v4l2_kioctl func)
 {
-	char	sbuf[128];
+	char	sbuf[128] = { };
 	void    *mbuf = NULL, *array_buf = NULL;
 	void	*parg = (void *)arg;
 	long	err  = -EINVAL;
@@ -3258,7 +3258,7 @@  video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 			parg = sbuf;
 		} else {
 			/* too big to allocate from stack */
-			mbuf = kmalloc(ioc_size, GFP_KERNEL);
+			mbuf = kzalloc(ioc_size, GFP_KERNEL);
 			if (NULL == mbuf)
 				return -ENOMEM;
 			parg = mbuf;