| 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 |
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 --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;
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(-)