From: Dan Carpenter <dan.carpenter@oracle.com> To: Arnd Bergmann <arnd@arndb.de> Cc: "Peilin Ye" <yepeilin.cs@gmail.com>, "Mauro Carvalho Chehab" <mchehab@kernel.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, syzkaller-bugs <syzkaller-bugs@googlegroups.com>, "Hans Verkuil" <hverkuil-cisco@xs4all.nl>, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>, "Vandana BN" <bnvandana@gmail.com>, "Ezequiel Garcia" <ezequiel@collabora.com>, "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>, linux-kernel-mentees@lists.linuxfoundation.org, "Linux Media Mailing List" <linux-media@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Linus Walleij" <linus.walleij@linaro.org> Subject: Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user() Date: Mon, 27 Jul 2020 17:43:35 +0300 [thread overview] Message-ID: <20200727144335.GB2571@kadam> (raw) In-Reply-To: <CAK8P3a3+9Gr6G6KDWu=iW3316O9cPH+XupBBajJaxrq20xQcyQ@mail.gmail.com> 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.carpenter@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 <yepeilin.cs@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+79d751604cb6f29fbf59@syzkaller.appspotmail.com > > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59 > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Peilin Ye <yepeilin.cs@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
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com> To: Arnd Bergmann <arnd@arndb.de> Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>, "Linus Walleij" <linus.walleij@linaro.org>, syzkaller-bugs <syzkaller-bugs@googlegroups.com>, "Linux Media Mailing List" <linux-media@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Vandana BN" <bnvandana@gmail.com>, "Hans Verkuil" <hverkuil-cisco@xs4all.nl>, "Mauro Carvalho Chehab" <mchehab@kernel.org>, "Ezequiel Garcia" <ezequiel@collabora.com>, "Peilin Ye" <yepeilin.cs@gmail.com>, linux-kernel-mentees@lists.linuxfoundation.org Subject: Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user() Date: Mon, 27 Jul 2020 17:43:35 +0300 [thread overview] Message-ID: <20200727144335.GB2571@kadam> (raw) In-Reply-To: <CAK8P3a3+9Gr6G6KDWu=iW3316O9cPH+XupBBajJaxrq20xQcyQ@mail.gmail.com> 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.carpenter@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 <yepeilin.cs@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+79d751604cb6f29fbf59@syzkaller.appspotmail.com > > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59 > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Peilin Ye <yepeilin.cs@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 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-07-27 14:44 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-26 16:44 [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user() Peilin Ye 2020-07-26 16:44 ` Peilin Ye 2020-07-26 17:30 ` Laurent Pinchart 2020-07-26 17:30 ` Laurent Pinchart 2020-07-26 18:07 ` Peilin Ye 2020-07-26 18:07 ` Peilin Ye 2020-07-26 22:08 ` Laurent Pinchart 2020-07-26 22:08 ` Laurent Pinchart 2020-07-26 22:15 ` Peilin Ye 2020-07-26 22:15 ` Peilin Ye 2020-07-26 18:12 ` Peilin Ye 2020-07-26 18:12 ` Peilin Ye 2020-07-26 22:05 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye 2020-07-26 22:05 ` Peilin Ye 2020-07-26 22:10 ` Laurent Pinchart 2020-07-26 22:10 ` Laurent Pinchart 2020-07-26 22:16 ` Peilin Ye 2020-07-26 22:16 ` Peilin Ye 2020-07-26 22:27 ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye 2020-07-26 22:27 ` Peilin Ye 2020-07-27 7:25 ` Arnd Bergmann 2020-07-27 7:25 ` Arnd Bergmann 2020-07-27 7:56 ` Peilin Ye 2020-07-27 7:56 ` Peilin Ye 2020-07-27 13:16 ` Dan Carpenter 2020-07-27 13:16 ` Dan Carpenter 2020-07-27 14:05 ` Arnd Bergmann 2020-07-27 14:05 ` Arnd Bergmann 2020-07-27 14:14 ` Peilin Ye 2020-07-27 14:14 ` Peilin Ye 2020-07-27 14:20 ` Arnd Bergmann 2020-07-27 14:20 ` Arnd Bergmann 2020-07-27 14:46 ` Dan Carpenter 2020-07-27 14:46 ` Dan Carpenter 2020-07-27 15:30 ` Peilin Ye 2020-07-27 15:30 ` Peilin Ye 2020-07-27 14:43 ` Dan Carpenter [this message] 2020-07-27 14:43 ` Dan Carpenter 2020-07-27 14:55 ` Arnd Bergmann 2020-07-27 14:55 ` Arnd Bergmann 2020-07-27 22:04 ` Peilin Ye 2020-07-27 22:04 ` Peilin Ye 2020-07-28 9:00 ` Arnd Bergmann 2020-07-28 9:00 ` Arnd Bergmann 2020-07-28 10:02 ` Dan Carpenter 2020-07-28 10:02 ` Dan Carpenter 2020-07-27 22:33 ` Peilin Ye 2020-07-27 22:33 ` Peilin Ye 2020-07-28 9:10 ` Arnd Bergmann 2020-07-28 9:10 ` Arnd Bergmann 2020-07-28 9:47 ` Dan Carpenter 2020-07-28 9:47 ` Dan Carpenter 2020-07-28 13:13 ` Peilin Ye 2020-07-28 13:13 ` Peilin Ye 2020-07-28 12:22 ` Linus Walleij 2020-07-28 12:22 ` Linus Walleij 2020-07-28 13:06 ` Dan Carpenter 2020-07-28 13:06 ` Dan Carpenter 2020-07-28 13:58 ` Arnd Bergmann 2020-07-28 13:58 ` Arnd Bergmann 2020-07-30 8:07 ` Bartosz Golaszewski 2020-07-30 8:07 ` Bartosz Golaszewski 2020-07-30 8:15 ` Arnd Bergmann 2020-07-30 8:15 ` Arnd Bergmann 2020-07-30 8:38 ` Andy Shevchenko 2020-07-30 8:38 ` Andy Shevchenko 2020-07-30 9:18 ` Arnd Bergmann 2020-07-30 9:18 ` Arnd Bergmann 2020-07-30 11:48 ` Andy Shevchenko 2020-07-30 11:48 ` Andy Shevchenko 2020-07-30 13:49 ` Arnd Bergmann 2020-07-30 13:49 ` Arnd Bergmann 2020-08-02 16:55 ` Peilin Ye 2020-08-02 16:55 ` Peilin Ye 2020-07-27 8:00 ` [Linux-kernel-mentees] [PATCH v4] " Peilin Ye 2020-07-27 8:00 ` Peilin Ye
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200727144335.GB2571@kadam \ --to=dan.carpenter@oracle.com \ --cc=arnd@arndb.de \ --cc=bnvandana@gmail.com \ --cc=ezequiel@collabora.com \ --cc=gregkh@linuxfoundation.org \ --cc=hverkuil-cisco@xs4all.nl \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linus.walleij@linaro.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=niklas.soderlund+renesas@ragnatech.se \ --cc=sakari.ailus@linux.intel.com \ --cc=syzkaller-bugs@googlegroups.com \ --cc=yepeilin.cs@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.