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]

Date:   Sun, 26 Jul 2020 18:15:12 -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 Mon, Jul 27, 2020 at 01:08:23AM +0300, Laurent Pinchart wrote:
> Hi Peilin,
> 
> On Sun, Jul 26, 2020 at 02:07:52PM -0400, Peilin Ye wrote:
> > 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.
> 
> 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.pinchart@...asonboard.com>

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

Thank you for reviewing the patch! 

Peilin Ye