| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK8P3a1TvFCJf8t9T1yOXjsp088s9dbEOKLVDPinfwJe2B-27g@mail.gmail.com>
Date: Fri, 13 Dec 2019 16:08:11 +0100
From: Arnd Bergmann <arnd@...db.de>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Linux Media Mailing List <linux-media@...r.kernel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
y2038 Mailman List <y2038@...ts.linaro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for
time64 ABI
On Thu, Dec 12, 2019 at 4:43 PM Hans Verkuil <hverkuil@...all.nl> wrote:
>
> On 11/26/19 5:18 PM, Arnd Bergmann wrote:
> >
> > switch (cmd) {
> > +#ifdef COMPAT_32BIT_TIME
>
> COMPAT_32BIT_TIME -> CONFIG_COMPAT_32BIT_TIME
Fixed.
> > + *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,
>
> You forgot to copy sequence.
>
> > + .memory = vb32.memory,
> > + .m.userptr = vb32.m.usercopy,
>
> usercopy -> userptr
Fixed.
> > + .length = vb32.length,
> > + .request_fd = vb32.request_fd,
> > + };
> > +
> > + if (cmd == VIDIOC_QUERYBUF_TIME32)
> > + memset(&vb->length, 0, sizeof(*vb) -
> > + offsetof(struct v4l2_buffer, length));
>
> It's from the field AFTER vb->length that this needs to be zeroed. It's best to
> use the CLEAR_AFTER_FIELD macro here.
I'm a bit lost about this one: the fields that are not explicitly
uninitialized here are already set to zero by the assignment
above. Should this simply be a
if (cmd == VIDIOC_QUERYBUF_TIME32)
vb->request_fd = 0;
then? I don't remember where that memset() originally came
from or why request_fd has to be cleared here.
> > @@ -3100,6 +3141,30 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
> > return -EFAULT;
> > break;
> > }
> > + case VIDIOC_QUERYBUF_TIME32:
> > + case VIDIOC_QBUF_TIME32:
> > + case VIDIOC_DQBUF_TIME32:
> > + case VIDIOC_PREPARE_BUF_TIME32: {
> > + struct v4l2_buffer *vb = parg;
> > + struct v4l2_buffer_time32 vb32 = {
> > + .index = vb->index,
> > + .type = vb->type,
> > + .bytesused = vb->bytesused,
> > + .flags = vb->flags,
> > + .field = vb->field,
> > + .timestamp.tv_sec = vb->timestamp.tv_sec,
> > + .timestamp.tv_usec = vb->timestamp.tv_usec,
> > + .timecode = vb->timecode,
>
> You forgot to copy sequence.
Fixed.
> With these changes this patch series passed both the 64 and 32 bit compliance
> tests (in fact, all the issues mentioned above were found with these compliance
> tests).
Yay compliance tests!
> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs,
> and there doesn't appear to be an easy way to compile a C++ program with musl.
>
> If you happen to have a test environment where you can compile C++ with musl,
> then let me know and I can give instructions on how to run the compliance tests.
>
> If you can't test that, then I can merge this regardless, and hope for the best
> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested.
I've heard good things about the prebuilt toolchains from http://musl.cc/.
These seems to come with a libstdc++, but I have not tried that myself.
I've folded the change below into this patch in my y2038-v4l2-v6 branch
but have not been able to update the copy on git.kernel.org yet because of
server-side issues today.
Arnd
8<-----
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index c416870a3166..667225712343 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3055,7 +3055,7 @@ static int video_get_user(void __user *arg, void
*parg, unsigned int cmd,
}
switch (cmd) {
-#ifdef COMPAT_32BIT_TIME
+#ifdef CONFIG_COMPAT_32BIT_TIME
case VIDIOC_QUERYBUF_TIME32:
case VIDIOC_QBUF_TIME32:
case VIDIOC_DQBUF_TIME32:
@@ -3075,15 +3075,15 @@ static int video_get_user(void __user *arg,
void *parg, unsigned int cmd,
.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.usercopy,
+ .m.userptr = vb32.m.userptr,
.length = vb32.length,
.request_fd = vb32.request_fd,
};
if (cmd == VIDIOC_QUERYBUF_TIME32)
- memset(&vb->length, 0, sizeof(*vb) -
- offsetof(struct v4l2_buffer, length));
+ vb->request_fd = 0;
break;
}
@@ -3155,6 +3155,7 @@ static int video_put_user(void __user *arg, void
*parg, unsigned int cmd)
.timestamp.tv_sec = vb->timestamp.tv_sec,
.timestamp.tv_usec = vb->timestamp.tv_usec,
.timecode = vb->timecode,
+ .sequence = vb->sequence,
.memory = vb->memory,
.m.userptr = vb->m.userptr,
.length = vb->length,
Powered by blists - more mailing lists