media/v4l2-core: Fix kernel-infoleak in video_put_user()

[Peilin Ye]

#syz test: https://github.com/google/kmsan.git master

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com

Tested on:

commit: 93f54a72 instrumented.h: fix KMSAN support
git tree: https://github.com/google/kmsan.git master
kernel config: https://syzkaller.appspot.com/x/.config?x=c534a9fad6323722
dashboard link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
patch: https://syzkaller.appspot.com/x/patch.diff?x=158c5f78900000

Note: testing is done by a robot and is best-effort only.

[Peilin Ye]

video_put_user() is copying uninitialized stack memory to userspace. Fix
it by initializing `vb32` using memset().

Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Signed-off-by: Peilin Ye <yepei...@gmail.com>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 32 +++++++++++++++-------------
1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a556880f225a..08909f58dc80 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3210,21 +3210,23 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
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,
- .sequence = vb->sequence,
- .memory = vb->memory,
- .m.userptr = vb->m.userptr,
- .length = vb->length,
- .request_fd = vb->request_fd,
- };
+ struct v4l2_buffer_time32 vb32;
+
+ memset(&vb32, 0, sizeof(vb32));
+
+ vb32.index = vb->index;
+ vb32.type = vb->type;
+ vb32.bytesused = vb->bytesused;
+ vb32.flags = vb->flags;
+ vb32.field = vb->field;
+ vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
+ vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
+ vb32.timecode = vb->timecode;
+ vb32.sequence = vb->sequence;
+ vb32.memory = vb->memory;
+ vb32.m.userptr = vb->m.userptr;
+ vb32.length = vb->length;
+ vb32.request_fd = vb->request_fd;

if (copy_to_user(arg, &vb32, sizeof(vb32)))
return -EFAULT;
--
2.25.1

[Laurent Pinchart]

Hi Peilin,

Thank you for the patch.

On Sun, Jul 26, 2020 at 12:44:39PM -0400, Peilin Ye wrote:
> video_put_user() is copying uninitialized stack memory to userspace. Fix
> it by initializing `vb32` using memset().

What makes you think this will fix the issue ? When initializing a
structure at declaration time, the fields that are not explicitly
specified should be initialized to 0 by the compiler. See
https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.cbclx01/strin.htm:

If a structure variable is partially initialized, all the uninitialized
structure members are implicitly initialized to zero no matter what the
storage class of the structure variable is. See the following example:

struct one {
int a;
int b;
int c;
};

void main() {
struct one z1; // Members in z1 do not have default initial values.
static struct one z2; // z2.a=0, z2.b=0, and z2.c=0.
struct one z3 = {1}; // z3.a=1, z3.b=0, and z3.c=0.

Regards,

Laurent Pinchart

[Peilin Ye]

On Sun, Jul 26, 2020 at 08:30:44PM +0300, Laurent Pinchart wrote:
> Hi Peilin,
>
> Thank you for the patch.
>
> On Sun, Jul 26, 2020 at 12:44:39PM -0400, Peilin Ye wrote:
> > video_put_user() is copying uninitialized stack memory to userspace. Fix
> > it by initializing `vb32` using memset().
>
> What makes you think this will fix the issue ? When initializing a
> structure at declaration time, the fields that are not explicitly
> specified should be initialized to 0 by the compiler. See
> https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.cbclx01/strin.htm:

Hi Mr. Pinchart!

First of all, syzbot tested this patch, and it says it's "OK":

https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59

> If a structure variable is partially initialized, all the uninitialized
> structure members are implicitly initialized to zero no matter what the
> storage class of the structure variable is. See the following example:
>
> struct one {
> int a;
> int b;
> int c;
> };
>
> void main() {
> struct one z1; // Members in z1 do not have default initial values.
> static struct one z2; // z2.a=0, z2.b=0, and z2.c=0.
> struct one z3 = {1}; // z3.a=1, z3.b=0, and z3.c=0.
> }

Yes, I understand that. I can safely printk() all members of that struct
without triggering a KMSAN warning, which means they have been properly
initialized.

However, if I do something like:

char *p = (char *)&vb32;
int i;

for (i = 0; i < sizeof(struct vb32); i++, p++)
printk("*(p + i): %d", *(p + i));

This tries to print out `vb32` as "raw memory" one byte at a time, and
triggers a KMSAN warning somewhere in the middle (when `i` equals to 25
or 26).

According to a previous discussion with Mr. Kroah-Hartman, as well as
this LWN article:

"Structure holes and information leaks"
https://lwn.net/Articles/417989/

Initializing a struct by assigning (both partially or fully) leaves the
"padding" part of it uninitialized, thus potentially leads to kernel
information leak if the structure in question is going to be copied to
userspace.

memset() sets these "uninitialized paddings" to zero, therefore (I
think) should solve the problem.

Thank you!
Peilin Ye

[Peilin Ye]

Sorry, by this code example:

char *p = (char *)&vb32;
int i;

for (i = 0; i < sizeof(struct vb32); i++, p++)
printk("*(p + i): %d", *(p + i));

actually I meant:

char *p = (char *)&vb32;
int i;

for (i = 0; i < sizeof(struct vb32); i++)

printk("*(p + i): %d", *(p + i));

But the idea is the same.

Thank you,
Peilin Ye

[Peilin Ye]

video_put_user() is copying uninitialized stack memory to userspace. Fix

it by initializing `ev32` and `vb32` using memset().

Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Signed-off-by: Peilin Ye <yepei...@gmail.com>
---

Change in v2:
- Do the same thing for `case VIDIOC_DQEVENT_TIME32`.

drivers/media/v4l2-core/v4l2-ioctl.c | 50 +++++++++++++++-------------
1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a556880f225a..e3a25ea913ac 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3189,14 +3189,16 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
#ifdef CONFIG_COMPAT_32BIT_TIME
case VIDIOC_DQEVENT_TIME32: {
struct v4l2_event *ev = parg;
- struct v4l2_event_time32 ev32 = {
- .type = ev->type,
- .pending = ev->pending,
- .sequence = ev->sequence,
- .timestamp.tv_sec = ev->timestamp.tv_sec,
- .timestamp.tv_nsec = ev->timestamp.tv_nsec,
- .id = ev->id,
- };
+ struct v4l2_event_time32 ev32;
+
+ memset(&ev32, 0, sizeof(ev32));
+
+ ev32.type = ev->type;
+ ev32.pending = ev->pending;
+ ev32.sequence = ev->sequence;
+ ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
+ ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
+ ev32.id = ev->id;

memcpy(&ev32.u, &ev->u, sizeof(ev->u));
memcpy(&ev32.reserved, &ev->reserved, sizeof(ev->reserved));
@@ -3210,21 +3212,23 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)

[Laurent Pinchart]

Hi Peilin,

You're absolutely right. I wasn't aware the compiler wouldn't initialize
holes in the structure. Thank you for educating me :-)

For the patch,

Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>

--
Regards,

Laurent Pinchart

[Laurent Pinchart]

Hi Peilin,

Thank you for the patch.

On Sun, Jul 26, 2020 at 06:05:57PM -0400, Peilin Ye wrote:
> video_put_user() is copying uninitialized stack memory to userspace. Fix
> it by initializing `ev32` and `vb32` using memset().

How about mentioning that this is caused by the compiler not
initializing the holes ? Maybe something along the lines of

video_put_user() is copying uninitialized stack memory to userspace due
to the compiler not initializing holes in the structures declared on the
stack. Fix it by initializing the structures using memset().

Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>

Regards,

Laurent Pinchart

[Peilin Ye]

:O No no sir, I'm just rephrasing that LWN article.

Thank you for reviewing the patch!

Peilin Ye

[Peilin Ye]

On Mon, Jul 27, 2020 at 01:10:56AM +0300, Laurent Pinchart wrote:
> Hi Peilin,
>
> Thank you for the patch.
>
> On Sun, Jul 26, 2020 at 06:05:57PM -0400, Peilin Ye wrote:
> > video_put_user() is copying uninitialized stack memory to userspace. Fix
> > it by initializing `ev32` and `vb32` using memset().
>
> How about mentioning that this is caused by the compiler not
> initializing the holes ? Maybe something along the lines of
>
> video_put_user() is copying uninitialized stack memory to userspace due
> to the compiler not initializing holes in the structures declared on the
> stack. Fix it by initializing the structures using memset().
>
> Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>

I see, that makes sense. I will send a v3.

Thank you,
Peilin Ye

[Peilin Ye]

video_put_user() is copying uninitialized stack memory to userspace due
to the compiler not initializing holes in the structures declared on the

stack. Fix it by initializing `ev32` and `vb32` using memset().

Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59

Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>

Signed-off-by: Peilin Ye <yepei...@gmail.com>
---

Change in v3:
- Improve the commit description. (Suggested by Laurent Pinchart
<laurent....@ideasonboard.com>)

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <yepei...@gmail.com> wrote:
>
> video_put_user() is copying uninitialized stack memory to userspace due
> to the compiler not initializing holes in the structures declared on the
> stack. Fix it by initializing `ev32` and `vb32` using memset().
>
> Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>
> Signed-off-by: Peilin Ye <yepei...@gmail.com>

Thanks a lot for addressing this! I now see that I actually created a similar
bugfix for it back in January, but for some reason that got stuck in my
backlog and I never wrote a proper description for it or sent it out to the
list, sorry about that. I would hope we could find a way to have either
the compiler or sparse warn if we copy uninitialized data to user space,
but we now don't even check for that within the kernel any more.

I would suggest adding these tags to the patch, to ensure it gets backported
to stable kernels as needed:

Cc: sta...@vger.kernel.org
Fixes: 1a6c0b36dd19 ("media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI")
Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for
time64 ABI")

In addition to

Reviewed-by: Arnd Bergmann <ar...@arndb.de>

[Peilin Ye]

On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote:
> On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <yepei...@gmail.com> wrote:
> >
> > video_put_user() is copying uninitialized stack memory to userspace due
> > to the compiler not initializing holes in the structures declared on the
> > stack. Fix it by initializing `ev32` and `vb32` using memset().
> >
> > Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>
> > Signed-off-by: Peilin Ye <yepei...@gmail.com>
>
> Thanks a lot for addressing this! I now see that I actually created a similar
> bugfix for it back in January, but for some reason that got stuck in my
> backlog and I never wrote a proper description for it or sent it out to the
> list, sorry about that. I would hope we could find a way to have either
> the compiler or sparse warn if we copy uninitialized data to user space,
> but we now don't even check for that within the kernel any more.

I am glad to be of help!

> I would suggest adding these tags to the patch, to ensure it gets backported
> to stable kernels as needed:
>
> Cc: sta...@vger.kernel.org
> Fixes: 1a6c0b36dd19 ("media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI")
> Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for
> time64 ABI")
>
> In addition to
>
> Reviewed-by: Arnd Bergmann <ar...@arndb.de>

Sure, I will send a v4 soon. Thank you for reviewing the patch.

Peilin Ye

[Peilin Ye]

video_put_user() is copying uninitialized stack memory to userspace due
to the compiler not initializing holes in the structures declared on the
stack. Fix it by initializing `ev32` and `vb32` using memset().

Cc: sta...@vger.kernel.org
Fixes: 1a6c0b36dd19 ("media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI")
Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for time64 ABI")

Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>

Reviewed-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Peilin Ye <yepei...@gmail.com>
---

Change in v4:
- Add `Cc:` and `Fixes:` tags. (Suggested by Arnd Bergmann <ar...@arndb.de>)

[Dan Carpenter]

On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote:

> On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <yepei...@gmail.com> wrote:
> >
> > video_put_user() is copying uninitialized stack memory to userspace due
> > to the compiler not initializing holes in the structures declared on the
> > stack. Fix it by initializing `ev32` and `vb32` using memset().
> >
> > Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>
> > Signed-off-by: Peilin Ye <yepei...@gmail.com>
>
> Thanks a lot for addressing this! I now see that I actually created a similar
> bugfix for it back in January, but for some reason that got stuck in my
> backlog and I never wrote a proper description for it or sent it out to the
> list, sorry about that. I would hope we could find a way to have either
> the compiler or sparse warn if we copy uninitialized data to user space,
> but we now don't even check for that within the kernel any more.

Here are my latest warnings on linux-next from Friday.

block/scsi_ioctl.c:707 scsi_put_cdrom_generic_arg() warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction')
drivers/input/misc/uinput.c:743 uinput_ff_upload_to_user() warn: check that 'ff_up_compat' doesn't leak information (struct has a hole after 'replay')
drivers/input/misc/uinput.c:958 uinput_ioctl_handler() warn: check that 'ff_up' doesn't leak information (struct has a hole after 'replay')
drivers/firewire/core-cdev.c:463 ioctl_get_info() warn: check that 'bus_reset' doesn't leak information (struct has a hole after 'generation')
drivers/scsi/megaraid/megaraid_mm.c:824 kioc_to_mimd() warn: check that 'cinfo.base' doesn't leak information
drivers/gpio/gpiolib-cdev.c:473 lineevent_read() warn: check that 'ge' doesn't leak information (struct has a hole after 'id')
drivers/gpu/drm/i915/i915_query.c:136 query_engine_info() warn: check that 'query.num_engines' doesn't leak information
drivers/gpu/drm/drm_bufs.c:1357 copy_one_buf() warn: check that 'v' doesn't leak information (struct has a hole after 'flags')
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c:785 amdgpu_info_ioctl() warn: check that 'dev_info' doesn't leak information (struct has a hole after 'pa_sc_tile_steering_override')
drivers/block/floppy.c:3132 raw_cmd_copyout() warn: check that 'cmd' doesn't leak information (struct has a hole after 'flags')
drivers/char/hpet.c:675 hpet_ioctl() warn: check that 'info' doesn't leak information (struct has a hole after 'hi_timer')
drivers/media/v4l2-core/v4l2-ioctl.c:3204 video_put_user() warn: check that 'ev32' doesn't leak information (struct has a hole after 'type')
drivers/media/v4l2-core/v4l2-ioctl.c:3229 video_put_user() warn: check that 'vb32' doesn't leak information (struct has a hole after 'memory')
drivers/net/wan/sbni.c:1320 sbni_ioctl() warn: check that 'flags' doesn't leak information (struct has a hole after 'rxl')
drivers/infiniband/hw/qedr/verbs.c:1816 qedr_create_user_qp() warn: check that 'uresp' doesn't leak information (struct has a hole after 'sq_icid')
drivers/infiniband/hw/cxgb4/provider.c:107 c4iw_alloc_ucontext() warn: check that 'uresp.reserved' doesn't leak information
drivers/tty/vt/vt_ioctl.c:1218 vt_compat_ioctl() warn: check that 'op' doesn't leak information (struct has a hole after 'charcount')
net/smc/smc_diag.c:181 __smc_diag_dump() warn: check that 'dinfo' doesn't leak information (struct has a hole after 'linkid')
net/sched/act_ife.c:638 tcf_ife_dump() warn: check that 'opt' doesn't leak information (struct has a hole after 'flags')
net/sched/act_skbmod.c:232 tcf_skbmod_dump() warn: check that 'opt' doesn't leak information (struct has a hole after 'bindcnt')
net/sched/act_connmark.c:187 tcf_connmark_dump() warn: check that 'opt' doesn't leak information (struct has a hole after 'zone')
net/openvswitch/conntrack.c:311 ovs_ct_put_key() warn: check that 'orig' doesn't leak information (struct has a hole after 'ipv4_proto')
net/openvswitch/conntrack.c:322 ovs_ct_put_key() warn: check that 'orig' doesn't leak information (struct has a hole after 'ipv6_proto')
net/rds/recv.c:492 rds_notify_queue_get() warn: check that 'cmsg' doesn't leak information (struct has a hole after 'status')
net/xdp/xsk.c:870 xsk_getsockopt() warn: check that 'stats.rx_ring_full' doesn't leak information
kernel/signal.c:3524 __do_sys_rt_sigtimedwait() warn: check that 'info' doesn't leak information (struct has a hole after 'si_code')
kernel/signal.c:3524 __do_sys_rt_sigtimedwait() warn: check that 'info' doesn't leak information (struct has a hole after 'si_code')
kernel/signal.c:3556 __do_sys_rt_sigtimedwait_time32() warn: check that 'info' doesn't leak information (struct has a hole after 'si_code')
kernel/signal.c:3556 __do_sys_rt_sigtimedwait_time32() warn: check that 'info' doesn't leak information (struct has a hole after 'si_code')
kernel/signal.c:4055 __do_sys_sigaltstack() warn: check that 'old' doesn't leak information (struct has a hole after 'ss_flags')
kernel/signal.c:4055 __do_sys_sigaltstack() warn: check that 'old' doesn't leak information (struct has a hole after 'ss_flags')
kernel/ptrace.c:998 ptrace_get_syscall_info() warn: check that 'info' doesn't leak information (struct has a hole after 'op')

regards,
dan carpenter

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter <dan.ca...@oracle.com> wrote:
>
> On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote:
> > On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <yepei...@gmail.com> wrote:
> > >
> > > video_put_user() is copying uninitialized stack memory to userspace due
> > > to the compiler not initializing holes in the structures declared on the
> > > stack. Fix it by initializing `ev32` and `vb32` using memset().
> > >
> > > Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
> > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > > Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>
> > > Signed-off-by: Peilin Ye <yepei...@gmail.com>
> >
> > Thanks a lot for addressing this! I now see that I actually created a similar
> > bugfix for it back in January, but for some reason that got stuck in my
> > backlog and I never wrote a proper description for it or sent it out to the
> > list, sorry about that. I would hope we could find a way to have either
> > the compiler or sparse warn if we copy uninitialized data to user space,
> > but we now don't even check for that within the kernel any more.
>
> Here are my latest warnings on linux-next from Friday.

Ah, I forgot you had that kind of list already, thanks for checking!

> block/scsi_ioctl.c:707 scsi_put_cdrom_generic_arg() warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction')

I see no padding in this one, should be fine AFAICT. Any idea why you
get a warning
for this instance?

> drivers/input/misc/uinput.c:743 uinput_ff_upload_to_user() warn: check that 'ff_up_compat' doesn't leak information (struct has a hole after 'replay')

This one hs padding in it and looks broken.

> drivers/input/misc/uinput.c:958 uinput_ioctl_handler() warn: check that 'ff_up' doesn't leak information (struct has a hole after 'replay')

hard to tell.

> drivers/firewire/core-cdev.c:463 ioctl_get_info() warn: check that 'bus_reset' doesn't leak information (struct has a hole after 'generation')

broken, trivial to fix

> drivers/scsi/megaraid/megaraid_mm.c:824 kioc_to_mimd() warn: check that 'cinfo.base' doesn't leak information

Seems fine due to __packed annotation.

> drivers/gpio/gpiolib-cdev.c:473 lineevent_read() warn: check that 'ge' doesn't leak information (struct has a hole after 'id')

The driver seems to initialize the elements correctly before putting
them into the kfifo, so there is no infoleak. However the structure layout
of "struct gpioevent_data" is incompatible between x86-32 and x86-64
calling conventions, so this is likely broken in x86 compat mode,
unless user space can explicitly deal with the difference.

> drivers/gpu/drm/i915/i915_query.c:136 query_engine_info() warn: check that 'query.num_engines' doesn't leak information

I don't think this leaks any state, as it just copies data to user space that
it copied from there originally.

Stopping here for now.

Peilin Ye, is this something you are interested in fixing for the other drivers
as well? I'd be happy to help review any further patches if you Cc me.

Arnd

[Peilin Ye]

Yes, I would like to! I will start from:

drivers/firewire/core-cdev.c:463
drivers/input/misc/uinput.c:743

...as you mentioned above.

Thank you!
Peilin Ye

[Arnd Bergmann]

Sounds good, thanks!

Arnd

[Dan Carpenter]

On Mon, Jul 27, 2020 at 04:05:38PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter <dan.ca...@oracle.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote:
> > > On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <yepei...@gmail.com> wrote:
> > > >
> > > > video_put_user() is copying uninitialized stack memory to userspace due
> > > > to the compiler not initializing holes in the structures declared on the
> > > > stack. Fix it by initializing `ev32` and `vb32` using memset().
> > > >
> > > > Reported-and-tested-by: syzbot+79d751...@syzkaller.appspotmail.com
> > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > > > Reviewed-by: Laurent Pinchart <laurent....@ideasonboard.com>
> > > > Signed-off-by: Peilin Ye <yepei...@gmail.com>
> > >
> > > Thanks a lot for addressing this! I now see that I actually created a similar
> > > bugfix for it back in January, but for some reason that got stuck in my
> > > backlog and I never wrote a proper description for it or sent it out to the
> > > list, sorry about that. I would hope we could find a way to have either
> > > the compiler or sparse warn if we copy uninitialized data to user space,
> > > but we now don't even check for that within the kernel any more.
> >
> > Here are my latest warnings on linux-next from Friday.
>
> Ah, I forgot you had that kind of list already, thanks for checking!
>
> > block/scsi_ioctl.c:707 scsi_put_cdrom_generic_arg() warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction')
>
> I see no padding in this one, should be fine AFAICT. Any idea why you
> get a warning
> for this instance?
>

The warning message only prints the first struct hole or uninitialized
struct memeber. ->data_direction in this case.

block/scsi_ioctl.c
646 #ifdef CONFIG_COMPAT
647 struct compat_cdrom_generic_command {
648 unsigned char cmd[CDROM_PACKET_SIZE];
649 compat_caddr_t buffer;
650 compat_uint_t buflen;
651 compat_int_t stat;
652 compat_caddr_t sense;
653 unsigned char data_direction;

There is going to be 3 bytes of padding between this char and the next
int.

654 compat_int_t quiet;
655 compat_int_t timeout;
656 compat_caddr_t reserved[1];
657 };
658 #endif

The bug seems real, but it depends on the compiler of course.

> > drivers/input/misc/uinput.c:743 uinput_ff_upload_to_user() warn: check that 'ff_up_compat' doesn't leak information (struct has a hole after 'replay')
>
> This one hs padding in it and looks broken.

No. This a bug in smatch. It's memcpy() over the hole, but the check
isn't capturing that. The code is slightly weird... I could try
silence the warning but it would only silence this one false positive so
I haven't investigated it.

>
> > drivers/input/misc/uinput.c:958 uinput_ioctl_handler() warn: check that 'ff_up' doesn't leak information (struct has a hole after 'replay')
>
> hard to tell.
>

Looks ok, I think.

> > drivers/firewire/core-cdev.c:463 ioctl_get_info() warn: check that 'bus_reset' doesn't leak information (struct has a hole after 'generation')
>
> broken, trivial to fix
>
> > drivers/scsi/megaraid/megaraid_mm.c:824 kioc_to_mimd() warn: check that 'cinfo.base' doesn't leak information
>
> Seems fine due to __packed annotation.
>

It's not related __packed. Smatch is saying that cinfo.base isn't
necessarily initialized.

drivers/scsi/megaraid/megaraid_mm.c
816
817 case MEGAIOC_QADAPINFO:
818
819 hinfo = (mraid_hba_info_t *)(unsigned long)
820 kioc->buf_vaddr;
821
822 hinfo_to_cinfo(hinfo, &cinfo);

hinfo_to_cinfo() is a no-op if hinfo is NULL. Smatch can't tell if
"hinfo" is non-NULL.

823
824 if (copy_to_user(kmimd.data, &cinfo, sizeof(cinfo)))
825 return (-EFAULT);
826
827 return 0;
828

> > drivers/gpio/gpiolib-cdev.c:473 lineevent_read() warn: check that 'ge' doesn't leak information (struct has a hole after 'id')
>
> The driver seems to initialize the elements correctly before putting
> them into the kfifo, so there is no infoleak. However the structure layout
> of "struct gpioevent_data" is incompatible between x86-32 and x86-64
> calling conventions, so this is likely broken in x86 compat mode,
> unless user space can explicitly deal with the difference.

Smatch isn't parsing kfifo_out() correctly. It turns out that
kfifo_out() is slightly complicated for Smatch.

>
> > drivers/gpu/drm/i915/i915_query.c:136 query_engine_info() warn: check that 'query.num_engines' doesn't leak information
>
> I don't think this leaks any state, as it just copies data to user space that
> it copied from there originally.

Yeah. copy_query_item() isn't parsed correctly. I've added this one
to my TODO list because it should parse this correctly.

regards,
dan carpenter

[Dan Carpenter]

On Mon, Jul 27, 2020 at 10:14:16AM -0400, Peilin Ye wrote:
> Yes, I would like to! I will start from:
>
> drivers/firewire/core-cdev.c:463

My prefered fix for this would be to add a memset at the start of
fill_bus_reset_event().

memset(event, 0, sizeof(*event));

spin_lock_irq(&card->lock);

event->closure = client->bus_reset_closure;


> drivers/input/misc/uinput.c:743

I don't think this is a bug.

regards,
dan carpenter

[Arnd Bergmann]

Right, I misread the single 'char' in there.

> > > drivers/input/misc/uinput.c:743 uinput_ff_upload_to_user() warn: check that 'ff_up_compat' doesn't leak information (struct has a hole after 'replay')
> >
> > This one hs padding in it and looks broken.
>
> No. This a bug in smatch. It's memcpy() over the hole, but the check
> isn't capturing that. The code is slightly weird... I could try
> silence the warning but it would only silence this one false positive so
> I haven't investigated it.

Ah right, and the structure it copies in turn comes from user space
originally.

> > > drivers/scsi/megaraid/megaraid_mm.c:824 kioc_to_mimd() warn: check that 'cinfo.base' doesn't leak information
> >
> > Seems fine due to __packed annotation.
> >
>
> It's not related __packed. Smatch is saying that cinfo.base isn't
> necessarily initialized.
>
> drivers/scsi/megaraid/megaraid_mm.c
> 816
> 817 case MEGAIOC_QADAPINFO:
> 818
> 819 hinfo = (mraid_hba_info_t *)(unsigned long)
> 820 kioc->buf_vaddr;
> 821
> 822 hinfo_to_cinfo(hinfo, &cinfo);
>
> hinfo_to_cinfo() is a no-op if hinfo is NULL. Smatch can't tell if
> "hinfo" is non-NULL.

Ok, that sounds fair, I couldn't easily tell either ;-)

Arnd

[Peilin Ye]

On Mon, Jul 27, 2020 at 05:46:09PM +0300, Dan Carpenter wrote:
> On Mon, Jul 27, 2020 at 10:14:16AM -0400, Peilin Ye wrote:
> > Yes, I would like to! I will start from:
> >
> > drivers/firewire/core-cdev.c:463
>
> My prefered fix for this would be to add a memset at the start of
> fill_bus_reset_event().
>
> memset(event, 0, sizeof(*event));
>
> spin_lock_irq(&card->lock);
>
> event->closure = client->bus_reset_closure;
>
>
> > drivers/input/misc/uinput.c:743

I just sent a patch to fix this as you suggested.

> I don't think this is a bug.

I see. I am now fixing:

[Peilin Ye]

On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> drivers/char/hpet.c:675 hpet_ioctl() warn: check that 'info' doesn't leak information (struct has a hole after 'hi_timer')

This one seems like a false positive.

drivers/char/hpet.c:670:

mutex_lock(&hpet_mutex);
err = hpet_ioctl_common(file->private_data, cmd, arg, &info);
mutex_unlock(&hpet_mutex);

if ((cmd == HPET_INFO) && !err &&
(copy_to_user((void __user *)arg, &info, sizeof(info))))
err = -EFAULT;

`info` is only being copied to userspace when `cmd` is `HPET_INFO`.
However, hpet_ioctl_common() is already doing memset() on `info` in
`case HPET_INFO`:

drivers/char/hpet.c:612:

case HPET_INFO:
{
memset(info, 0, sizeof(*info));
^^^^^^

Thank you,
Peilin Ye

[Arnd Bergmann]

Yes, makes sense.

Arnd

[Dan Carpenter]

On Mon, Jul 27, 2020 at 06:04:56PM -0400, Peilin Ye wrote:
> On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> > drivers/char/hpet.c:675 hpet_ioctl() warn: check that 'info' doesn't leak information (struct has a hole after 'hi_timer')
>
> This one seems like a false positive.

Yep.

>
> drivers/char/hpet.c:670:
>
> mutex_lock(&hpet_mutex);
> err = hpet_ioctl_common(file->private_data, cmd, arg, &info);
> mutex_unlock(&hpet_mutex);
>
> if ((cmd == HPET_INFO) && !err &&
> (copy_to_user((void __user *)arg, &info, sizeof(info))))
> err = -EFAULT;

When Smatch parses hpet_ioctl_common() it says there are two success
paths:

drivers/char/hpet.c | hpet_ioctl_common | 170 | 0 | PARAM_LIMIT | 1 | $ | 26625 |

The first success path is for when cmd is HPET_IE_ON. We don't care
about that.

drivers/char/hpet.c | hpet_ioctl_common | 185 | 0 | PARAM_LIMIT | 1 | $ | 26626,26628-26629,1074292742,2149083139 |

The second success path is for when cmd is HPET_IE_OFF, HPET_INFO,
HPET_EPI, HPET_DPI, or HPET_IRQFREQ. If Smatch tracked the HPET_INFO
by itself then this wouldn't print a warning, but since Smatch groups
all those cmds together then it does print a warning.

It's not impossible to make Smatch split apart the success paths some
more but it's complicated because it means storing more data and slows
down the parsing. The cross function database is already 27GB on my
system.

regards,
dan carpenter

[Linus Walleij]

On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <dan.ca...@oracle.com> wrote:

> Here are my latest warnings on linux-next from Friday.

Thanks for sharing this Dan, very interesting findings.

> drivers/gpio/gpiolib-cdev.c:473 lineevent_read() warn: check that 'ge' doesn't leak information (struct has a hole after 'id')

We are revamping the ABI for 64bit compatibility so we are now running
pahole on our stuff. I suppose we need to think about mending this old ABI
as well.

Yours,
Linus Walleij

[Dan Carpenter]

Yeah... But this one is a false positive. It's not super hard for me
to silence it actually. I'll take care of it. It could be a while
before I push this to the public repository though...

regards,
dan carpenter

[Arnd Bergmann]

The lineevent_read() function still needs to be fixed to support
32-bit compat mode on x86, which is independent of the warning.

Something like

static int lineevent_put_data(void __user *uptr, struct gpioevent_data *ge)
{
#ifdef __x86_64__
/* i386 has no padding after 'id' */
if (in_ia32_syscall()) {
struct {
compat_u64 timestamp __packed;
u32 id;
} compat_ge = { ge->timestamp, ge->id };

if (copy_to_user(uptr, &compat_ge, sizeof(compat_ge)))
return -EFAULT;

return sizeof(compat_ge);
}
#endif

if (copy_to_user(uptr, ge, sizeof(*ge))
return -EFAULT;

return sizeof(*ge);
}

Arnd

[Bartosz Golaszewski]

Hi Arnd,

Andy actually had a patch for that but since this isn't a regression
(it never worked), we decided to leave it as it is and get it right in
v2 API.

Bartosz

[Arnd Bergmann]

I would argue that it needs to be fixed anyway, unless you also want
to remove the v1 interface for native mode. If this works on 32-bit
kernels, on 64-bit kernels with 64-bit user space and on compat
32-bit user space on 64-bit non-x86 architectures, I see no reason
to leave it broken specifically on x86 compat user space. There are
still reasons to use 32-bit x86 user space for low-memory machines
even though native i386 kernels are getting increasingly silly.

Arnd

[Andy Shevchenko]

It was possible to "fix" (mitigate to some extent) before libgpiod got support
for several events in a request. Now it seems to be impossible to fix. AFAIU we
must discard any request to more than one event in it.

However I'm not an expert in compat IOCTL code (you are :-) and perhaps you may
provide ideas better than mine.

--
With Best Regards,
Andy Shevchenko

[Arnd Bergmann]

Any reason why the workaround I suggested above would not work?
The in_ia32_syscall() check should be completely reliable in telling whether
we are called from read() by an ia32 task or not, and we use the same
logic for input_event, which has a similar problem (on all compat architectures,
not just x86).

> However I'm not an expert in compat IOCTL code (you are :-) and perhaps you may
> provide ideas better than mine.

What makes this interface tricky is that this is actually a read() call, not
ioctl() which is usually easier because it encodes the data length in the
command code. As far as I could tell from skimming the interface, the
ioctls are actually fine here.

Arnd

[Andy Shevchenko]

On Thu, Jul 30, 2020 at 11:18:04AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 30, 2020 at 10:38 AM Andy Shevchenko
> <andriy.s...@linux.intel.com> wrote:
> > On Thu, Jul 30, 2020 at 10:15:24AM +0200, Arnd Bergmann wrote:
> > > On Thu, Jul 30, 2020 at 10:07 AM Bartosz Golaszewski <br...@bgdev.pl> wrote:

...

> > > I would argue that it needs to be fixed anyway, unless you also want
> > > to remove the v1 interface for native mode. If this works on 32-bit
> > > kernels, on 64-bit kernels with 64-bit user space and on compat
> > > 32-bit user space on 64-bit non-x86 architectures, I see no reason
> > > to leave it broken specifically on x86 compat user space. There are
> > > still reasons to use 32-bit x86 user space for low-memory machines
> > > even though native i386 kernels are getting increasingly silly.
> >
> > It was possible to "fix" (mitigate to some extent) before libgpiod got support
> > for several events in a request. Now it seems to be impossible to fix. AFAIU we
> > must discard any request to more than one event in it.
>
> Any reason why the workaround I suggested above would not work?

That is the question to somebody who has better understanding. If it works,
I vote up to get it fixed with little effort. I would be glad to test patches!

> The in_ia32_syscall() check should be completely reliable in telling whether
> we are called from read() by an ia32 task or not, and we use the same
> logic for input_event, which has a similar problem (on all compat architectures,
> not just x86).

By the way any reason why we have to have in_ia32_syscall() instead of
in_compat_syscall()?

> > However I'm not an expert in compat IOCTL code (you are :-) and perhaps you may
> > provide ideas better than mine.
>
> What makes this interface tricky is that this is actually a read() call, not
> ioctl() which is usually easier because it encodes the data length in the
> command code. As far as I could tell from skimming the interface, the
> ioctls are actually fine here.

Right.

[Arnd Bergmann]

On Thu, Jul 30, 2020 at 1:48 PM Andy Shevchenko
<andriy.s...@linux.intel.com> wrote:
> On Thu, Jul 30, 2020 at 11:18:04AM +0200, Arnd Bergmann wrote:
> > The in_ia32_syscall() check should be completely reliable in telling whether
> > we are called from read() by an ia32 task or not, and we use the same
> > logic for input_event, which has a similar problem (on all compat architectures,
> > not just x86).
>
> By the way any reason why we have to have in_ia32_syscall() instead of
> in_compat_syscall()?

x86 is the only architecture that has different struct alignment between 32-bit
and 64-bit processes, so others don't have this particular problem. On top of
that, x86 also has two different 32-bit ABIs and only one of them needs the
workaround, while the other (x32) uses the same struct layout as x86-64 and
must use the normal code path.

Arnd