video_put_user() is copying uninitialized stack memory to userspace. Fix
it by initializing `vb32` using memset().
Reported-and-tested-by: [email protected]
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Signed-off-by: Peilin Ye <[email protected]>
---
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
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: [email protected]
> Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> 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
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
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
video_put_user() is copying uninitialized stack memory to userspace. Fix
it by initializing `ev32` and `vb32` using memset().
Reported-and-tested-by: [email protected]
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Signed-off-by: Peilin Ye <[email protected]>
---
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
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 <[email protected]>
--
Regards,
Laurent Pinchart
Hi Peilin,
Thank you for the patch.
On Sun, Jul 26, 2020 at 06:05:57PM -0400, Peilin Ye wrote:
> video_put_user() is copying uninitialized stack memory to userspace. Fix
> it by initializing `ev32` and `vb32` using memset().
How about mentioning that this is caused by the compiler not
initializing the holes ? Maybe something along the lines of
video_put_user() is copying uninitialized stack memory to userspace due
to the compiler not initializing holes in the structures declared on the
stack. Fix it by initializing the structures using memset().
Reviewed-by: Laurent Pinchart <[email protected]>
> Reported-and-tested-by: [email protected]
> Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> 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
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 <[email protected]>
:O No no sir, I'm just rephrasing that LWN article.
Thank you for reviewing the patch!
Peilin Ye
On Mon, Jul 27, 2020 at 01:10:56AM +0300, Laurent Pinchart wrote:
> Hi Peilin,
>
> Thank you for the patch.
>
> On Sun, Jul 26, 2020 at 06:05:57PM -0400, Peilin Ye wrote:
> > video_put_user() is copying uninitialized stack memory to userspace. Fix
> > it by initializing `ev32` and `vb32` using memset().
>
> How about mentioning that this is caused by the compiler not
> initializing the holes ? Maybe something along the lines of
>
> video_put_user() is copying uninitialized stack memory to userspace due
> to the compiler not initializing holes in the structures declared on the
> stack. Fix it by initializing the structures using memset().
>
> Reviewed-by: Laurent Pinchart <[email protected]>
I see, that makes sense. I will send a v3.
Thank you,
Peilin Ye
video_put_user() is copying uninitialized stack memory to userspace due
to the compiler not initializing holes in the structures declared on the
stack. Fix it by initializing `ev32` and `vb32` using memset().
Reported-and-tested-by: [email protected]
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Change in v3:
- Improve the commit description. (Suggested by Laurent Pinchart
<[email protected]>)
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
On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <[email protected]> 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: [email protected]
> Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> Reviewed-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
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: [email protected]
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 <[email protected]>
On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote:
> On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <[email protected]> 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: [email protected]
> > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
>
> 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: [email protected]
> 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 <[email protected]>
Sure, I will send a v4 soon. Thank you for reviewing the patch.
Peilin Ye
video_put_user() is copying uninitialized stack memory to userspace due
to the compiler not initializing holes in the structures declared on the
stack. Fix it by initializing `ev32` and `vb32` using memset().
Cc: [email protected]
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: [email protected]
Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Change in v4:
- Add `Cc:` and `Fixes:` tags. (Suggested by Arnd Bergmann <[email protected]>)
Change in v3:
- Improve the commit description. (Suggested by Laurent Pinchart
<[email protected]>)
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
On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote:
> On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye <[email protected]> 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: [email protected]
> > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
>
> 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
On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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: [email protected]
> > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > Signed-off-by: Peilin Ye <[email protected]>
> >
> > 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
On Mon, Jul 27, 2020 at 04:05:38PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > > Signed-off-by: Peilin Ye <[email protected]>
> > >
> > > 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
On Mon, Jul 27, 2020 at 4:14 PM Peilin Ye <[email protected]> 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
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
On Mon, Jul 27, 2020 at 04:05:38PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > > Signed-off-by: Peilin Ye <[email protected]>
> > >
> > > 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
On Mon, Jul 27, 2020 at 4:43 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59
> > > > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > > > Signed-off-by: Peilin Ye <[email protected]>
> > > >
> > > > 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
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')
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
On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> drivers/block/floppy.c:3132 raw_cmd_copyout() warn: check that 'cmd' doesn't leak information (struct has a hole after 'flags')
(Removed some Cc: recipients from the list.)
I'm not very sure, but I think this one is also a false positive.
Here Smatch is complaining about a linked list called `my_raw_cmd`
defined in raw_cmd_ioctl():
drivers/block/floppy.c:3249:
ret = raw_cmd_copyin(cmd, param, &my_raw_cmd);
In raw_cmd_copyin(), each element of the linked list is allocated by
kmalloc() then copied from user:
drivers/block/floppy.c:3180:
loop:
ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
^^^^^^^
if (!ptr)
return -ENOMEM;
*rcmd = ptr;
ret = copy_from_user(ptr, param, sizeof(*ptr));
^^^^^^^^^^^^^^
I think copy_from_user() is filling in the paddings inside `struct
floppy_raw_cmd`?
Thank you,
Peilin Ye
On Tue, Jul 28, 2020 at 12:05 AM Peilin Ye <[email protected]> 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
On Tue, Jul 28, 2020 at 12:34 AM Peilin Ye <[email protected]> wrote:
>
> On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> > drivers/block/floppy.c:3132 raw_cmd_copyout() warn: check that 'cmd' doesn't leak information (struct has a hole after 'flags')
>
> (Removed some Cc: recipients from the list.)
>
> I'm not very sure, but I think this one is also a false positive.
>
> Here Smatch is complaining about a linked list called `my_raw_cmd`
> defined in raw_cmd_ioctl():
>
> drivers/block/floppy.c:3249:
>
> ret = raw_cmd_copyin(cmd, param, &my_raw_cmd);
>
> In raw_cmd_copyin(), each element of the linked list is allocated by
> kmalloc() then copied from user:
>
> drivers/block/floppy.c:3180:
>
> loop:
> ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
> ^^^^^^^
> if (!ptr)
> return -ENOMEM;
> *rcmd = ptr;
> ret = copy_from_user(ptr, param, sizeof(*ptr));
> ^^^^^^^^^^^^^^
>
> I think copy_from_user() is filling in the paddings inside `struct
> floppy_raw_cmd`?
I am not completely sure about this one either. copy_from_user()
would indeed fill the pad bytes in the structure, but there is another
problem:
struct floppy_raw_cmd cmd = *ptr;
cmd.next = NULL;
cmd.kernel_data = NULL;
ret = copy_to_user(param, &cmd, sizeof(cmd));
IIRC the struct assignment is allowed to be done per member
and skip the padding, so the on-stack copy can then again
contain a data leak. The compiler is likely to turn a struct
assignment into a memcpy(), but as the code then goes on
to set two members individually, I suppose doing a per-member
copy would not be unreasonable behavior either and doing
a memcpy() instead of an assignment would be the safe
choice.
If someone has a clearer understanding of what the compiler
is actually allowed to do here, please let us know.
Arnd
On Mon, Jul 27, 2020 at 06:33:57PM -0400, Peilin Ye wrote:
> On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> > drivers/block/floppy.c:3132 raw_cmd_copyout() warn: check that 'cmd' doesn't leak information (struct has a hole after 'flags')
>
> (Removed some Cc: recipients from the list.)
>
> I'm not very sure, but I think this one is also a false positive.
No, it's a potential bug. You're over thinking what Smatch is
complaining about. Arnd is right.
3123 static int raw_cmd_copyout(int cmd, void __user *param,
3124 struct floppy_raw_cmd *ptr)
3125 {
3126 int ret;
3127
3128 while (ptr) {
3129 struct floppy_raw_cmd cmd = *ptr;
^^^^^^^^^^
The compiler can either do this assignment as an memcpy() or as a
series of struct member assignments. So the assignment can leave the
struct hole uninitialized.
3130 cmd.next = NULL;
3131 cmd.kernel_data = NULL;
3132 ret = copy_to_user(param, &cmd, sizeof(cmd));
^^^^
potential info leak.
3133 if (ret)
3134 return -EFAULT;
3135 param += sizeof(struct floppy_raw_cmd);
3136 if ((ptr->flags & FD_RAW_READ) && ptr->buffer_length) {
3137 if (ptr->length >= 0 &&
3138 ptr->length <= ptr->buffer_length) {
3139 long length = ptr->buffer_length - ptr->length;
3140 ret = fd_copyout(ptr->data, ptr->kernel_data,
3141 length);
3142 if (ret)
3143 return ret;
3144 }
3145 }
3146 ptr = ptr->next;
3147 }
3148
3149 return 0;
3150 }
regards,
dan carpenter
On Mon, Jul 27, 2020 at 06:04:56PM -0400, Peilin Ye wrote:
> On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> > drivers/char/hpet.c:675 hpet_ioctl() warn: check that 'info' doesn't leak information (struct has a hole after 'hi_timer')
>
> This one seems like a false positive.
Yep.
>
> drivers/char/hpet.c:670:
>
> mutex_lock(&hpet_mutex);
> err = hpet_ioctl_common(file->private_data, cmd, arg, &info);
> mutex_unlock(&hpet_mutex);
>
> if ((cmd == HPET_INFO) && !err &&
> (copy_to_user((void __user *)arg, &info, sizeof(info))))
> err = -EFAULT;
When Smatch parses hpet_ioctl_common() it says there are two success
paths:
drivers/char/hpet.c | hpet_ioctl_common | 170 | 0 | PARAM_LIMIT | 1 | $ | 26625 |
The first success path is for when cmd is HPET_IE_ON. We don't care
about that.
drivers/char/hpet.c | hpet_ioctl_common | 185 | 0 | PARAM_LIMIT | 1 | $ | 26626,26628-26629,1074292742,2149083139 |
The second success path is for when cmd is HPET_IE_OFF, HPET_INFO,
HPET_EPI, HPET_DPI, or HPET_IRQFREQ. If Smatch tracked the HPET_INFO
by itself then this wouldn't print a warning, but since Smatch groups
all those cmds together then it does print a warning.
It's not impossible to make Smatch split apart the success paths some
more but it's complicated because it means storing more data and slows
down the parsing. The cross function database is already 27GB on my
system.
regards,
dan carpenter
On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <[email protected]> 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
On Tue, Jul 28, 2020 at 02:22:29PM +0200, Linus Walleij wrote:
> On Mon, Jul 27, 2020 at 3:17 PM Dan Carpenter <[email protected]> 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
On Tue, Jul 28, 2020 at 12:47:07PM +0300, Dan Carpenter wrote:
> On Mon, Jul 27, 2020 at 06:33:57PM -0400, Peilin Ye wrote:
> > On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> > > drivers/block/floppy.c:3132 raw_cmd_copyout() warn: check that 'cmd' doesn't leak information (struct has a hole after 'flags')
> >
> > (Removed some Cc: recipients from the list.)
> >
> > I'm not very sure, but I think this one is also a false positive.
>
> No, it's a potential bug. You're over thinking what Smatch is
> complaining about. Arnd is right.
>
> 3123 static int raw_cmd_copyout(int cmd, void __user *param,
> 3124 struct floppy_raw_cmd *ptr)
> 3125 {
> 3126 int ret;
> 3127
> 3128 while (ptr) {
> 3129 struct floppy_raw_cmd cmd = *ptr;
> ^^^^^^^^^^
> The compiler can either do this assignment as an memcpy() or as a
> series of struct member assignments. So the assignment can leave the
> struct hole uninitialized.
I see, I didn't realize this line could cause the issue. Thank you for
pointing this out, I will do this then send a patch:
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 09079aee8dc4..398c261fd174 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3126,7 +3126,8 @@ static int raw_cmd_copyout(int cmd, void __user *param,
int ret;
while (ptr) {
- struct floppy_raw_cmd cmd = *ptr;
+ struct floppy_raw_cmd cmd;
+ memcpy(&cmd, ptr, sizeof(cmd));
cmd.next = NULL;
cmd.kernel_data = NULL;
ret = copy_to_user(param, &cmd, sizeof(cmd));
Thank you,
Peilin Ye
> 3130 cmd.next = NULL;
> 3131 cmd.kernel_data = NULL;
> 3132 ret = copy_to_user(param, &cmd, sizeof(cmd));
> ^^^^
> potential info leak.
>
> 3133 if (ret)
> 3134 return -EFAULT;
> 3135 param += sizeof(struct floppy_raw_cmd);
> 3136 if ((ptr->flags & FD_RAW_READ) && ptr->buffer_length) {
> 3137 if (ptr->length >= 0 &&
> 3138 ptr->length <= ptr->buffer_length) {
> 3139 long length = ptr->buffer_length - ptr->length;
> 3140 ret = fd_copyout(ptr->data, ptr->kernel_data,
> 3141 length);
> 3142 if (ret)
> 3143 return ret;
> 3144 }
> 3145 }
> 3146 ptr = ptr->next;
> 3147 }
> 3148
> 3149 return 0;
> 3150 }
>
> regards,
> dan carpenter
On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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
On Thu, Jul 30, 2020 at 10:07 AM Bartosz Golaszewski <[email protected]> wrote:
>
> On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <[email protected]> wrote:
> >
> > On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <[email protected]> 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
On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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
On Thu, Jul 30, 2020 at 10:15:24AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 30, 2020 at 10:07 AM Bartosz Golaszewski <[email protected]> wrote:
> >
> > On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <[email protected]> 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
On Thu, Jul 30, 2020 at 10:38 AM Andy Shevchenko
<[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 3:58 PM Arnd Bergmann <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 28, 2020 at 3:06 PM Dan Carpenter <[email protected]> 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
On Thu, Jul 30, 2020 at 11:18:04AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 30, 2020 at 10:38 AM Andy Shevchenko
> <[email protected]> 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 <[email protected]> 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
On Thu, Jul 30, 2020 at 1:48 PM Andy Shevchenko
<[email protected]> 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
On Mon, Jul 27, 2020 at 04:16:08PM +0300, Dan Carpenter wrote:
> 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')
I think these are safe. Before getting to copy_siginfo_to_user(), all
`info` run through:
do_sigtimedwait()
V
dequeue_signal()
V
__dequeue_signal()
V
collect_signal()
Where it either gets memcpy()'d by copy_siginfo(), or memset()'d by
clear_siginfo().
The only exception is when next_signal() fails in __dequeue_signal(),
but that will cause do_sigtimedwait() to return an error, and we never
reach copy_siginfo_to_user() in such a case.
Thank you,
Peilin Ye