Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user()
@ 2020-07-26 16:44 Peilin Ye
  2020-07-26 17:30 ` Laurent Pinchart
  2020-07-26 22:05 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  0 siblings, 2 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 16:44 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Peilin Ye, Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil,
	Sakari Ailus, Arnd Bergmann, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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

Reported-and-tested-by: syzbot+79d751604cb6f29fbf59@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Signed-off-by: Peilin Ye <yepeilin.cs@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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 16:44 [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user() Peilin Ye
@ 2020-07-26 17:30 ` Laurent Pinchart
  2020-07-26 18:07   ` Peilin Ye
  2020-07-26 18:12   ` Peilin Ye
  2020-07-26 22:05 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  1 sibling, 2 replies; 33+ messages in thread
From: Laurent Pinchart @ 2020-07-26 17:30 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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.
}

> Reported-and-tested-by: syzbot+79d751604cb6f29fbf59@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> Signed-off-by: Peilin Ye <yepeilin.cs@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;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 17:30 ` Laurent Pinchart
@ 2020-07-26 18:07   ` Peilin Ye
  2020-07-26 22:08     ` Laurent Pinchart
  2020-07-26 18:12   ` Peilin Ye
  1 sibling, 1 reply; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 18:07 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 17:30 ` Laurent Pinchart
  2020-07-26 18:07   ` Peilin Ye
@ 2020-07-26 18:12   ` Peilin Ye
  1 sibling, 0 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 18:12 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Linux-kernel-mentees] [PATCH v2] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 16:44 [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user() Peilin Ye
  2020-07-26 17:30 ` Laurent Pinchart
@ 2020-07-26 22:05 ` Peilin Ye
  2020-07-26 22:10   ` Laurent Pinchart
  2020-07-26 22:27   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
  1 sibling, 2 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 22:05 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Peilin Ye, Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil,
	Sakari Ailus, Arnd Bergmann, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

video_put_user() is copying uninitialized stack memory to userspace. 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
Signed-off-by: Peilin Ye <yepeilin.cs@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)
 	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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 18:07   ` Peilin Ye
@ 2020-07-26 22:08     ` Laurent Pinchart
  2020-07-26 22:15       ` Peilin Ye
  0 siblings, 1 reply; 33+ messages in thread
From: Laurent Pinchart @ 2020-07-26 22:08 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 22:05 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
@ 2020-07-26 22:10   ` Laurent Pinchart
  2020-07-26 22:16     ` Peilin Ye
  2020-07-26 22:27   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
  1 sibling, 1 reply; 33+ messages in thread
From: Laurent Pinchart @ 2020-07-26 22:10 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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.pinchart@ideasonboard.com>

> Reported-and-tested-by: syzbot+79d751604cb6f29fbf59@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> Signed-off-by: Peilin Ye <yepeilin.cs@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)
>  	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;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 22:08     ` Laurent Pinchart
@ 2020-07-26 22:15       ` Peilin Ye
  0 siblings, 0 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 22:15 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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@ideasonboard.com>

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

Thank you for reviewing the patch! 

Peilin Ye

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 22:10   ` Laurent Pinchart
@ 2020-07-26 22:16     ` Peilin Ye
  0 siblings, 0 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 22:16 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Arnd Bergmann, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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.pinchart@ideasonboard.com>

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

Thank you,
Peilin Ye

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 22:05 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  2020-07-26 22:10   ` Laurent Pinchart
@ 2020-07-26 22:27   ` Peilin Ye
  2020-07-27  7:25     ` Arnd Bergmann
  2020-07-27  8:00     ` [Linux-kernel-mentees] [PATCH v4] " Peilin Ye
  1 sibling, 2 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-26 22:27 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Peilin Ye, Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil,
	Sakari Ailus, Arnd Bergmann, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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>
---
Change in v3:
    - Improve the commit description. (Suggested by Laurent Pinchart
      <laurent.pinchart@ideasonboard.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)
 	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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 22:27   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
@ 2020-07-27  7:25     ` Arnd Bergmann
  2020-07-27  7:56       ` Peilin Ye
  2020-07-27 13:16       ` Dan Carpenter
  2020-07-27  8:00     ` [Linux-kernel-mentees] [PATCH v4] " Peilin Ye
  1 sibling, 2 replies; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-27  7:25 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

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.

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

Cc: stable@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 <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27  7:25     ` Arnd Bergmann
@ 2020-07-27  7:56       ` Peilin Ye
  2020-07-27 13:16       ` Dan Carpenter
  1 sibling, 0 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-27  7:56 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

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.

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: stable@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 <arnd@arndb.de>

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

Peilin Ye

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Linux-kernel-mentees] [PATCH v4] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-26 22:27   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
  2020-07-27  7:25     ` Arnd Bergmann
@ 2020-07-27  8:00     ` Peilin Ye
  1 sibling, 0 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-27  8:00 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Peilin Ye, Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil,
	Sakari Ailus, Arnd Bergmann, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	linux-media, linux-kernel

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: stable@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+79d751604cb6f29fbf59@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Change in v4:
    - Add `Cc:` and `Fixes:` tags. (Suggested by Arnd Bergmann <arnd@arndb.de>)

Change in v3:
    - Improve the commit description. (Suggested by Laurent Pinchart
      <laurent.pinchart@ideasonboard.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)
 	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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27  7:25     ` Arnd Bergmann
  2020-07-27  7:56       ` Peilin Ye
@ 2020-07-27 13:16       ` Dan Carpenter
  2020-07-27 14:05         ` Arnd Bergmann
                           ` (2 more replies)
  1 sibling, 3 replies; 33+ messages in thread
From: Dan Carpenter @ 2020-07-27 13:16 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Peilin Ye, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel

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.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 13:16       ` Dan Carpenter
@ 2020-07-27 14:05         ` Arnd Bergmann
  2020-07-27 14:14           ` Peilin Ye
  2020-07-27 14:43           ` Dan Carpenter
  2020-07-27 22:04         ` Peilin Ye
  2020-07-28 12:22         ` Linus Walleij
  2 siblings, 2 replies; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-27 14:05 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Peilin Ye, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

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?

> 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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 14:05         ` Arnd Bergmann
@ 2020-07-27 14:14           ` Peilin Ye
  2020-07-27 14:20             ` Arnd Bergmann
  2020-07-27 14:46             ` Dan Carpenter
  2020-07-27 14:43           ` Dan Carpenter
  1 sibling, 2 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-27 14:14 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dan Carpenter, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

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?
> 
> > 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.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 14:14           ` Peilin Ye
@ 2020-07-27 14:20             ` Arnd Bergmann
  2020-07-27 14:46             ` Dan Carpenter
  1 sibling, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-27 14:20 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Dan Carpenter, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

On Mon, Jul 27, 2020 at 4:14 PM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> On Mon, Jul 27, 2020 at 04:05:38PM +0200, Arnd Bergmann wrote:
> > 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.
>
> 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.

Sounds good, thanks!

     Arnd

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 14:05         ` Arnd Bergmann
  2020-07-27 14:14           ` Peilin Ye
@ 2020-07-27 14:43           ` Dan Carpenter
  2020-07-27 14:55             ` Arnd Bergmann
  1 sibling, 1 reply; 33+ messages in thread
From: Dan Carpenter @ 2020-07-27 14:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Peilin Ye, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 14:14           ` Peilin Ye
  2020-07-27 14:20             ` Arnd Bergmann
@ 2020-07-27 14:46             ` Dan Carpenter
  2020-07-27 15:30               ` Peilin Ye
  1 sibling, 1 reply; 33+ messages in thread
From: Dan Carpenter @ 2020-07-27 14:46 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Arnd Bergmann, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 14:43           ` Dan Carpenter
@ 2020-07-27 14:55             ` Arnd Bergmann
  0 siblings, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-27 14:55 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Peilin Ye, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

On Mon, Jul 27, 2020 at 4:43 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 14:46             ` Dan Carpenter
@ 2020-07-27 15:30               ` Peilin Ye
  0 siblings, 0 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-27 15:30 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Arnd Bergmann, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel,
	Linus Walleij

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:

	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')

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 13:16       ` Dan Carpenter
  2020-07-27 14:05         ` Arnd Bergmann
@ 2020-07-27 22:04         ` Peilin Ye
  2020-07-28  9:00           ` Arnd Bergmann
  2020-07-28 10:02           ` Dan Carpenter
  2020-07-28 12:22         ` Linus Walleij
  2 siblings, 2 replies; 33+ messages in thread
From: Peilin Ye @ 2020-07-27 22:04 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Arnd Bergmann, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 22:04         ` Peilin Ye
@ 2020-07-28  9:00           ` Arnd Bergmann
  2020-07-28 10:02           ` Dan Carpenter
  1 sibling, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-28  9:00 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Dan Carpenter, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel

On Tue, Jul 28, 2020 at 12:05 AM Peilin Ye <yepeilin.cs@gmail.com> 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.
>
> 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));
>                         ^^^^^^

Yes, makes sense.

      Arnd

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 22:04         ` Peilin Ye
  2020-07-28  9:00           ` Arnd Bergmann
@ 2020-07-28 10:02           ` Dan Carpenter
  1 sibling, 0 replies; 33+ messages in thread
From: Dan Carpenter @ 2020-07-28 10:02 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Arnd Bergmann, Mauro Carvalho Chehab, Greg Kroah-Hartman,
	syzkaller-bugs, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
	Vandana BN, Ezequiel Garcia, Niklas Söderlund,
	linux-kernel-mentees, Linux Media Mailing List, linux-kernel

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-27 13:16       ` Dan Carpenter
  2020-07-27 14:05         ` Arnd Bergmann
  2020-07-27 22:04         ` Peilin Ye
@ 2020-07-28 12:22         ` Linus Walleij
  2020-07-28 13:06           ` Dan Carpenter
  2 siblings, 1 reply; 33+ messages in thread
From: Linus Walleij @ 2020-07-28 12:22 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Arnd Bergmann, Peilin Ye, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil, Sakari Ailus,
	Laurent Pinchart, Vandana BN, Ezequiel Garcia,
	Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <dan.carpenter@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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-28 12:22         ` Linus Walleij
@ 2020-07-28 13:06           ` Dan Carpenter
  2020-07-28 13:58             ` Arnd Bergmann
  0 siblings, 1 reply; 33+ messages in thread
From: Dan Carpenter @ 2020-07-28 13:06 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Arnd Bergmann, Peilin Ye, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil, Sakari Ailus,
	Laurent Pinchart, Vandana BN, Ezequiel Garcia,
	Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

On Tue, Jul 28, 2020 at 02:22:29PM +0200, Linus Walleij wrote:
> On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <dan.carpenter@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.

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-28 13:06           ` Dan Carpenter
@ 2020-07-28 13:58             ` Arnd Bergmann
  2020-07-30  8:07               ` Bartosz Golaszewski
  0 siblings, 1 reply; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-28 13:58 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Linus Walleij, Peilin Ye, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil, Sakari Ailus,
	Laurent Pinchart, Vandana BN, Ezequiel Garcia,
	Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jul 28, 2020 at 02:22:29PM +0200, Linus Walleij wrote:
> > On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <dan.carpenter@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.
>
> 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...

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-28 13:58             ` Arnd Bergmann
@ 2020-07-30  8:07               ` Bartosz Golaszewski
  2020-07-30  8:15                 ` Arnd Bergmann
  0 siblings, 1 reply; 33+ messages in thread
From: Bartosz Golaszewski @ 2020-07-30  8:07 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dan Carpenter, Linus Walleij, Peilin Ye, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil, Sakari Ailus,
	Laurent Pinchart, Vandana BN, Ezequiel Garcia,
	Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel, Andy Shevchenko

On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Jul 28, 2020 at 02:22:29PM +0200, Linus Walleij wrote:
> > > On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <dan.carpenter@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.
> >
> > 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...
>
> 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

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-30  8:07               ` Bartosz Golaszewski
@ 2020-07-30  8:15                 ` Arnd Bergmann
  2020-07-30  8:38                   ` Andy Shevchenko
  0 siblings, 1 reply; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-30  8:15 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Dan Carpenter, Linus Walleij, Peilin Ye, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, syzkaller-bugs, Hans Verkuil, Sakari Ailus,
	Laurent Pinchart, Vandana BN, Ezequiel Garcia,
	Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel, Andy Shevchenko

On Thu, Jul 30, 2020 at 10:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 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
>
> 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.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-30  8:15                 ` Arnd Bergmann
@ 2020-07-30  8:38                   ` Andy Shevchenko
  2020-07-30  9:18                     ` Arnd Bergmann
  0 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2020-07-30  8:38 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bartosz Golaszewski, Dan Carpenter, Linus Walleij, Peilin Ye,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

On Thu, Jul 30, 2020 at 10:15:24AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 30, 2020 at 10:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > 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
> >
> > 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.
> 
> 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.

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



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-30  8:38                   ` Andy Shevchenko
@ 2020-07-30  9:18                     ` Arnd Bergmann
  2020-07-30 11:48                       ` Andy Shevchenko
  0 siblings, 1 reply; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-30  9:18 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Bartosz Golaszewski, Dan Carpenter, Linus Walleij, Peilin Ye,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

On Thu, Jul 30, 2020 at 10:38 AM Andy Shevchenko
<andriy.shevchenko@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 <brgl@bgdev.pl> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > >
> > > > On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > 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
> > >
> > > 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.
> >
> > 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?
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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-30  9:18                     ` Arnd Bergmann
@ 2020-07-30 11:48                       ` Andy Shevchenko
  2020-07-30 13:49                         ` Arnd Bergmann
  0 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2020-07-30 11:48 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bartosz Golaszewski, Dan Carpenter, Linus Walleij, Peilin Ye,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

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.shevchenko@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 <brgl@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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user()
  2020-07-30 11:48                       ` Andy Shevchenko
@ 2020-07-30 13:49                         ` Arnd Bergmann
  0 siblings, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2020-07-30 13:49 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Bartosz Golaszewski, Dan Carpenter, Linus Walleij, Peilin Ye,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, syzkaller-bugs,
	Hans Verkuil, Sakari Ailus, Laurent Pinchart, Vandana BN,
	Ezequiel Garcia, Niklas Söderlund, linux-kernel-mentees,
	Linux Media Mailing List, linux-kernel

On Thu, Jul 30, 2020 at 1:48 PM Andy Shevchenko
<andriy.shevchenko@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

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, back to index

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-26 16:44 [Linux-kernel-mentees] [PATCH] media/v4l2-core: Fix kernel-infoleak in video_put_user() Peilin Ye
2020-07-26 17:30 ` Laurent Pinchart
2020-07-26 18:07   ` Peilin Ye
2020-07-26 22:08     ` Laurent Pinchart
2020-07-26 22:15       ` 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:10   ` Laurent Pinchart
2020-07-26 22:16     ` Peilin Ye
2020-07-26 22:27   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
2020-07-27  7:25     ` Arnd Bergmann
2020-07-27  7:56       ` Peilin Ye
2020-07-27 13:16       ` Dan Carpenter
2020-07-27 14:05         ` Arnd Bergmann
2020-07-27 14:14           ` Peilin Ye
2020-07-27 14:20             ` Arnd Bergmann
2020-07-27 14:46             ` Dan Carpenter
2020-07-27 15:30               ` Peilin Ye
2020-07-27 14:43           ` Dan Carpenter
2020-07-27 14:55             ` Arnd Bergmann
2020-07-27 22:04         ` Peilin Ye
2020-07-28  9:00           ` Arnd Bergmann
2020-07-28 10:02           ` Dan Carpenter
2020-07-28 12:22         ` Linus Walleij
2020-07-28 13:06           ` Dan Carpenter
2020-07-28 13:58             ` Arnd Bergmann
2020-07-30  8:07               ` Bartosz Golaszewski
2020-07-30  8:15                 ` Arnd Bergmann
2020-07-30  8:38                   ` Andy Shevchenko
2020-07-30  9:18                     ` Arnd Bergmann
2020-07-30 11:48                       ` Andy Shevchenko
2020-07-30 13:49                         ` Arnd Bergmann
2020-07-27  8:00     ` [Linux-kernel-mentees] [PATCH v4] " Peilin Ye

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git