| Message ID | 20210610214305.4170835-3-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l2: compat ioctl fixes | expand |
On 10/06/2021 23:43, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > As seen from a recent syzbot bug report, mistakes in the compat ioctl > implementation can lead to uninitialized kernel stack data getting used > as input for driver ioctl handlers. > > The reported bug is now fixed, but it's possible that other related > bugs are still present or get added in the future. As the drivers need > to check user input already, the possible impact is fairly low, but it > might still cause an information leak. > > To be on the safe side, always clear the entire ioctl buffer before > calling the conversion handler functions that are meant to initialize > them. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++------------ > 1 file changed, 29 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 58df927aec7e..bf5eb07296a5 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -3122,12 +3122,23 @@ static int video_get_user(void __user *arg, void *parg, > > if (cmd == real_cmd) { > if (copy_from_user(parg, (void __user *)arg, n)) > - err = -EFAULT; > - } else if (in_compat_syscall()) { > - err = v4l2_compat_get_user(arg, parg, cmd); > - } else { > - switch (cmd) { > + return -EFAULT; > + > + /* zero out anything we don't copy from userspace */ > + if (n < _IOC_SIZE(real_cmd)) > + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); This should always happen, not just when cmd == real_cmd. The comment is a bit misleading: besides zeroing what isn't copied from userspace, it also zeroes copied fields based on INFO_FL_CLEAR_MASK. With this change that no longer happens and v4l2-compliance starts complaining. > + > + return 0; > + } > + > + /* zero out whole buffer first to deal with missing emulation */ > + memset(parg, 0, _IOC_SIZE(real_cmd)); > + > + if (in_compat_syscall()) > + return v4l2_compat_get_user(arg, parg, cmd); > + > #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) > + switch (cmd) { > case VIDIOC_QUERYBUF_TIME32: > case VIDIOC_QBUF_TIME32: > case VIDIOC_DQBUF_TIME32: The 'case' statements need to be indented one tab less. > @@ -3140,28 +3151,24 @@ static int video_get_user(void __user *arg, void *parg, > > *vb = (struct v4l2_buffer) { > .index = vb32.index, > - .type = vb32.type, > - .bytesused = vb32.bytesused, > - .flags = vb32.flags, > - .field = vb32.field, > - .timestamp.tv_sec = vb32.timestamp.tv_sec, > - .timestamp.tv_usec = vb32.timestamp.tv_usec, > - .timecode = vb32.timecode, > - .sequence = vb32.sequence, > - .memory = vb32.memory, > - .m.userptr = vb32.m.userptr, > - .length = vb32.length, > - .request_fd = vb32.request_fd, > + .type = vb32.type, > + .bytesused = vb32.bytesused, > + .flags = vb32.flags, > + .field = vb32.field, > + .timestamp.tv_sec = vb32.timestamp.tv_sec, > + .timestamp.tv_usec = vb32.timestamp.tv_usec, > + .timecode = vb32.timecode, > + .sequence = vb32.sequence, > + .memory = vb32.memory, > + .m.userptr = vb32.m.userptr, > + .length = vb32.length, > + .request_fd = vb32.request_fd, Can you put these whitespace changes in a separate patch? > }; > break; > } > -#endif > - } > } > +#endif > > - /* zero out anything we don't copy from userspace */ > - if (!err && n < _IOC_SIZE(real_cmd)) > - memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); > return err; > } > > I ended up with this code, and then my tests passed: if (cmd == real_cmd) { if (copy_from_user(parg, (void __user *)arg, n)) return -EFAULT; } else if (in_compat_syscall()) { memset(parg, 0, n); err = v4l2_compat_get_user(arg, parg, cmd); } else { #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) memset(parg, 0, n); switch (cmd) { case VIDIOC_QUERYBUF_TIME32: case VIDIOC_QBUF_TIME32: case VIDIOC_DQBUF_TIME32: case VIDIOC_PREPARE_BUF_TIME32: { struct v4l2_buffer_time32 vb32; struct v4l2_buffer *vb = parg; if (copy_from_user(&vb32, arg, sizeof(vb32))) return -EFAULT; *vb = (struct v4l2_buffer) { .index = vb32.index, .type = vb32.type, .bytesused = vb32.bytesused, .flags = vb32.flags, .field = vb32.field, .timestamp.tv_sec = vb32.timestamp.tv_sec, .timestamp.tv_usec = vb32.timestamp.tv_usec, .timecode = vb32.timecode, .sequence = vb32.sequence, .memory = vb32.memory, .m.userptr = vb32.m.userptr, .length = vb32.length, .request_fd = vb32.request_fd, }; break; } } #endif } /* zero out anything we don't copy from userspace */ if (!err && n < _IOC_SIZE(real_cmd)) memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); return err; That said, I also ran the regression tests on a i686 VM, and there I got a bunch of failures, but that was *without* your patches, so I think something unrelated broke. I'll have to dig more into this in the next few days. But I wanted to get this out first, since this patch is clearly wrong. Regards, Hans
On Fri, Jun 11, 2021 at 2:05 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > On 10/06/2021 23:43, Arnd Bergmann wrote: > > @@ -3122,12 +3122,23 @@ static int video_get_user(void __user *arg, void *parg, > > > > if (cmd == real_cmd) { > > if (copy_from_user(parg, (void __user *)arg, n)) > > - err = -EFAULT; > > - } else if (in_compat_syscall()) { > > - err = v4l2_compat_get_user(arg, parg, cmd); > > - } else { > > - switch (cmd) { > > + return -EFAULT; > > + > > + /* zero out anything we don't copy from userspace */ > > + if (n < _IOC_SIZE(real_cmd)) > > + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); > > This should always happen, not just when cmd == real_cmd. Ok, got it. I was trying to simplify this, but I went a little too far, so in the case of VIDIOC_QUERYBUF_TIME32 I dropped the final clearing of the extra data, leaving the user data in place. > The comment is a bit misleading: besides zeroing what isn't copied from > userspace, it also zeroes copied fields based on INFO_FL_CLEAR_MASK. I'm not following here, isn't that the same? We copy 'n' bytes, and then we clear 'size - n' bytes, which is everything that wasn't copied. > With this change that no longer happens and v4l2-compliance starts complaining. > > > + > > + return 0; > > + } > > + > > + /* zero out whole buffer first to deal with missing emulation */ > > + memset(parg, 0, _IOC_SIZE(real_cmd)); > > + > > + if (in_compat_syscall()) > > + return v4l2_compat_get_user(arg, parg, cmd); > > + > > #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) > > + switch (cmd) { > > case VIDIOC_QUERYBUF_TIME32: > > case VIDIOC_QBUF_TIME32: > > case VIDIOC_DQBUF_TIME32: > > The 'case' statements need to be indented one tab less. It seems this is no longer needed when I go back to having the switch() inside the else{}. > > @@ -3140,28 +3151,24 @@ static int video_get_user(void __user *arg, void *parg, > > > > *vb = (struct v4l2_buffer) { > > .index = vb32.index, > > - .type = vb32.type, > > - .bytesused = vb32.bytesused, > > - .flags = vb32.flags, > > - .field = vb32.field, > > - .timestamp.tv_sec = vb32.timestamp.tv_sec, > > - .timestamp.tv_usec = vb32.timestamp.tv_usec, > > - .timecode = vb32.timecode, > > - .sequence = vb32.sequence, > > - .memory = vb32.memory, > > - .m.userptr = vb32.m.userptr, > > - .length = vb32.length, > > - .request_fd = vb32.request_fd, > > + .type = vb32.type, > > + .bytesused = vb32.bytesused, > > + .flags = vb32.flags, > > + .field = vb32.field, > > + .timestamp.tv_sec = vb32.timestamp.tv_sec, > > + .timestamp.tv_usec = vb32.timestamp.tv_usec, > > + .timecode = vb32.timecode, > > + .sequence = vb32.sequence, > > + .memory = vb32.memory, > > + .m.userptr = vb32.m.userptr, > > + .length = vb32.length, > > + .request_fd = vb32.request_fd, > > Can you put these whitespace changes in a separate patch? Sure. > I ended up with this code, and then my tests passed: > > if (cmd == real_cmd) { > if (copy_from_user(parg, (void __user *)arg, n)) > return -EFAULT; > } else if (in_compat_syscall()) { > memset(parg, 0, n); > err = v4l2_compat_get_user(arg, parg, cmd); > } else { > #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) > memset(parg, 0, n); > switch (cmd) { > case VIDIOC_QUERYBUF_TIME32: > case VIDIOC_QBUF_TIME32: > case VIDIOC_DQBUF_TIME32: > case VIDIOC_PREPARE_BUF_TIME32: { > struct v4l2_buffer_time32 vb32; > struct v4l2_buffer *vb = parg; > > if (copy_from_user(&vb32, arg, sizeof(vb32))) > return -EFAULT; > > *vb = (struct v4l2_buffer) { > .index = vb32.index, > .type = vb32.type, > .bytesused = vb32.bytesused, > .flags = vb32.flags, > .field = vb32.field, > .timestamp.tv_sec = vb32.timestamp.tv_sec, > .timestamp.tv_usec = vb32.timestamp.tv_usec, > .timecode = vb32.timecode, > .sequence = vb32.sequence, > .memory = vb32.memory, > .m.userptr = vb32.m.userptr, > .length = vb32.length, > .request_fd = vb32.request_fd, > }; > break; > } > } > #endif > } > > /* zero out anything we don't copy from userspace */ > if (!err && n < _IOC_SIZE(real_cmd)) > memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); > > return err; Ok, so this version just adds the two memset(), without any other changes. That is clearly the safest change, and I'll send it like this in v3. > That said, I also ran the regression tests on a i686 VM, and there I got a > bunch of failures, but that was *without* your patches, so I think something > unrelated broke. I'll have to dig more into this in the next few days. > > But I wanted to get this out first, since this patch is clearly wrong. Thanks a lot for taking a look and giving it an initial test. I have updated the git tree at git://git.kernel.org:/pub/scm/linux/kernel/git/arnd/playground.git playground/v4l2-compat-ioctl with the changes you pointed out. Let me know when you have found out what was going on in the VM guest, and I'll send it as v3 or integrate additional fixes that you find necessary. Arnd
Hi Arnd, On 11/06/2021 17:22, Arnd Bergmann wrote: > On Fri, Jun 11, 2021 at 2:05 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> On 10/06/2021 23:43, Arnd Bergmann wrote: >>> @@ -3122,12 +3122,23 @@ static int video_get_user(void __user *arg, void *parg, >>> >>> if (cmd == real_cmd) { >>> if (copy_from_user(parg, (void __user *)arg, n)) >>> - err = -EFAULT; >>> - } else if (in_compat_syscall()) { >>> - err = v4l2_compat_get_user(arg, parg, cmd); >>> - } else { >>> - switch (cmd) { >>> + return -EFAULT; >>> + >>> + /* zero out anything we don't copy from userspace */ >>> + if (n < _IOC_SIZE(real_cmd)) >>> + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); >> >> This should always happen, not just when cmd == real_cmd. > > Ok, got it. I was trying to simplify this, but I went a little too far, so > in the case of VIDIOC_QUERYBUF_TIME32 I dropped the final > clearing of the extra data, leaving the user data in place. > >> The comment is a bit misleading: besides zeroing what isn't copied from >> userspace, it also zeroes copied fields based on INFO_FL_CLEAR_MASK. > > I'm not following here, isn't that the same? We copy 'n' bytes, and then we > clear 'size - n' bytes, which is everything that wasn't copied. > >> With this change that no longer happens and v4l2-compliance starts complaining. >> >>> + >>> + return 0; >>> + } >>> + >>> + /* zero out whole buffer first to deal with missing emulation */ >>> + memset(parg, 0, _IOC_SIZE(real_cmd)); >>> + >>> + if (in_compat_syscall()) >>> + return v4l2_compat_get_user(arg, parg, cmd); >>> + >>> #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) >>> + switch (cmd) { >>> case VIDIOC_QUERYBUF_TIME32: >>> case VIDIOC_QBUF_TIME32: >>> case VIDIOC_DQBUF_TIME32: >> >> The 'case' statements need to be indented one tab less. > > It seems this is no longer needed when I go back to having the switch() > inside the else{}. > >>> @@ -3140,28 +3151,24 @@ static int video_get_user(void __user *arg, void *parg, >>> >>> *vb = (struct v4l2_buffer) { >>> .index = vb32.index, >>> - .type = vb32.type, >>> - .bytesused = vb32.bytesused, >>> - .flags = vb32.flags, >>> - .field = vb32.field, >>> - .timestamp.tv_sec = vb32.timestamp.tv_sec, >>> - .timestamp.tv_usec = vb32.timestamp.tv_usec, >>> - .timecode = vb32.timecode, >>> - .sequence = vb32.sequence, >>> - .memory = vb32.memory, >>> - .m.userptr = vb32.m.userptr, >>> - .length = vb32.length, >>> - .request_fd = vb32.request_fd, >>> + .type = vb32.type, >>> + .bytesused = vb32.bytesused, >>> + .flags = vb32.flags, >>> + .field = vb32.field, >>> + .timestamp.tv_sec = vb32.timestamp.tv_sec, >>> + .timestamp.tv_usec = vb32.timestamp.tv_usec, >>> + .timecode = vb32.timecode, >>> + .sequence = vb32.sequence, >>> + .memory = vb32.memory, >>> + .m.userptr = vb32.m.userptr, >>> + .length = vb32.length, >>> + .request_fd = vb32.request_fd, >> >> Can you put these whitespace changes in a separate patch? > > Sure. > >> I ended up with this code, and then my tests passed: >> >> if (cmd == real_cmd) { >> if (copy_from_user(parg, (void __user *)arg, n)) >> return -EFAULT; >> } else if (in_compat_syscall()) { >> memset(parg, 0, n); >> err = v4l2_compat_get_user(arg, parg, cmd); >> } else { >> #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) >> memset(parg, 0, n); >> switch (cmd) { >> case VIDIOC_QUERYBUF_TIME32: >> case VIDIOC_QBUF_TIME32: >> case VIDIOC_DQBUF_TIME32: >> case VIDIOC_PREPARE_BUF_TIME32: { >> struct v4l2_buffer_time32 vb32; >> struct v4l2_buffer *vb = parg; >> >> if (copy_from_user(&vb32, arg, sizeof(vb32))) >> return -EFAULT; >> >> *vb = (struct v4l2_buffer) { >> .index = vb32.index, >> .type = vb32.type, >> .bytesused = vb32.bytesused, >> .flags = vb32.flags, >> .field = vb32.field, >> .timestamp.tv_sec = vb32.timestamp.tv_sec, >> .timestamp.tv_usec = vb32.timestamp.tv_usec, >> .timecode = vb32.timecode, >> .sequence = vb32.sequence, >> .memory = vb32.memory, >> .m.userptr = vb32.m.userptr, >> .length = vb32.length, >> .request_fd = vb32.request_fd, >> }; >> break; >> } >> } >> #endif >> } >> >> /* zero out anything we don't copy from userspace */ >> if (!err && n < _IOC_SIZE(real_cmd)) >> memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); >> >> return err; > > Ok, so this version just adds the two memset(), without any other > changes. That is clearly the safest change, and I'll send it like this > in v3. > >> That said, I also ran the regression tests on a i686 VM, and there I got a >> bunch of failures, but that was *without* your patches, so I think something >> unrelated broke. I'll have to dig more into this in the next few days. >> >> But I wanted to get this out first, since this patch is clearly wrong. > > Thanks a lot for taking a look and giving it an initial test. I have > updated the git tree at > > git://git.kernel.org:/pub/scm/linux/kernel/git/arnd/playground.git > playground/v4l2-compat-ioctl > > with the changes you pointed out. Let me know when you have found > out what was going on in the VM guest, and I'll send it as v3 or integrate > additional fixes that you find necessary. Please post v3: one issue I had turned out to be due to a misconfiguration causing some of the tests to run out of memory. The other issue is with the X32 ABI: that is currently failing, with or without your series. I'm not sure why yet, from what I remember this worked fine last time I tested. But my memory may be faulty, since it is so long ago. Regards, Hans > > Arnd >
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 58df927aec7e..bf5eb07296a5 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -3122,12 +3122,23 @@ static int video_get_user(void __user *arg, void *parg, if (cmd == real_cmd) { if (copy_from_user(parg, (void __user *)arg, n)) - err = -EFAULT; - } else if (in_compat_syscall()) { - err = v4l2_compat_get_user(arg, parg, cmd); - } else { - switch (cmd) { + return -EFAULT; + + /* zero out anything we don't copy from userspace */ + if (n < _IOC_SIZE(real_cmd)) + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); + + return 0; + } + + /* zero out whole buffer first to deal with missing emulation */ + memset(parg, 0, _IOC_SIZE(real_cmd)); + + if (in_compat_syscall()) + return v4l2_compat_get_user(arg, parg, cmd); + #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME) + switch (cmd) { case VIDIOC_QUERYBUF_TIME32: case VIDIOC_QBUF_TIME32: case VIDIOC_DQBUF_TIME32: @@ -3140,28 +3151,24 @@ static int video_get_user(void __user *arg, void *parg, *vb = (struct v4l2_buffer) { .index = vb32.index, - .type = vb32.type, - .bytesused = vb32.bytesused, - .flags = vb32.flags, - .field = vb32.field, - .timestamp.tv_sec = vb32.timestamp.tv_sec, - .timestamp.tv_usec = vb32.timestamp.tv_usec, - .timecode = vb32.timecode, - .sequence = vb32.sequence, - .memory = vb32.memory, - .m.userptr = vb32.m.userptr, - .length = vb32.length, - .request_fd = vb32.request_fd, + .type = vb32.type, + .bytesused = vb32.bytesused, + .flags = vb32.flags, + .field = vb32.field, + .timestamp.tv_sec = vb32.timestamp.tv_sec, + .timestamp.tv_usec = vb32.timestamp.tv_usec, + .timecode = vb32.timecode, + .sequence = vb32.sequence, + .memory = vb32.memory, + .m.userptr = vb32.m.userptr, + .length = vb32.length, + .request_fd = vb32.request_fd, }; break; } -#endif - } } +#endif - /* zero out anything we don't copy from userspace */ - if (!err && n < _IOC_SIZE(real_cmd)) - memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); return err; }