linux-kernel - Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200726180752.GA49356@PWN>
Date:   Sun, 26 Jul 2020 14:07:52 -0400
From:   Peilin Ye <yepeilin.cs@...il.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        syzkaller-bugs@...glegroups.com,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Arnd Bergmann <arnd@...db.de>,
        Vandana BN <bnvandana@...il.com>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Niklas Söderlund 
        <niklas.soderlund+renesas@...natech.se>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix
 kernel-infoleak in video_put_user()

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