* Re: [bug report] compat_ioctl: move CDROM_SEND_PACKET handling into scsi [not found] <20200107084659.uyaucu73yd5rhim3@kili.mountain> @ 2020-01-07 15:03 ` Arnd Bergmann 2020-01-07 15:16 ` Dan Carpenter 0 siblings, 1 reply; 3+ messages in thread From: Arnd Bergmann @ 2020-01-07 15:03 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-block, Hans Verkuil, Linux Media Mailing List On Tue, Jan 7, 2020 at 9:49 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Hello Arnd Bergmann, > > The patch f3ee6e63a9df: "compat_ioctl: move CDROM_SEND_PACKET > handling into scsi" from Nov 28, 2019, leads to the following static > checker warning: > > block/scsi_ioctl.c:703 scsi_put_cdrom_generic_arg() > warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction') > > block/scsi_ioctl.c > 686 static int scsi_put_cdrom_generic_arg(const struct cdrom_generic_command *cgc, > 687 void __user *arg) > 688 { > 689 #ifdef CONFIG_COMPAT > 690 if (in_compat_syscall()) { > 691 struct compat_cdrom_generic_command cgc32 = { > 692 .buffer = (uintptr_t)(cgc->buffer), > 693 .buflen = cgc->buflen, > 694 .stat = cgc->stat, > 695 .sense = (uintptr_t)(cgc->sense), > 696 .data_direction = cgc->data_direction, > 697 .quiet = cgc->quiet, > 698 .timeout = cgc->timeout, > 699 .reserved[0] = (uintptr_t)(cgc->reserved[0]), > 700 }; > > It's possible that initializations like this don't clear out the struct > hole but I haven't seen a compiler which is affected. So maybe it's > fine? I thlought we already rely on this to initialize the entire structure, but trying out a test case shows that it does happen: int g(void *); int f(void) { struct x { char a __attribute__((aligned(4096))); } x = { .a = 23 }; return g(&x); } 0000000000000000 <f>: 0: 55 push %rbp 1: 48 89 e5 mov %rsp,%rbp 4: 48 81 e4 00 f0 ff ff and $0xfffffffffffff000,%rsp b: 48 81 ec 00 10 00 00 sub $0x1000,%rsp 12: 48 89 e7 mov %rsp,%rdi 15: c6 04 24 17 movb $0x17,(%rsp) 19: e8 00 00 00 00 callq 1e <f+0x1e> 1a: R_X86_64_PC32 g-0x4 1e: c9 leaveq 1f: c3 retq This patch should reliably fix it, right? diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index b4e73d5dd5c2..83a709290b18 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -647,6 +647,7 @@ struct compat_cdrom_generic_command { compat_int_t stat; compat_caddr_t sense; unsigned char data_direction; + char __pad[3]; compat_int_t quiet; compat_int_t timeout; compat_caddr_t reserved[1]; > 701 memcpy(&cgc32.cmd, &cgc->cmd, CDROM_PACKET_SIZE); > 702 > 703 if (copy_to_user(arg, &cgc32, sizeof(cgc32))) > 704 return -EFAULT; > 705 > 706 return 0; > 707 } > 708 #endif > 709 if (copy_to_user(arg, cgc, sizeof(*cgc))) > 710 return -EFAULT; > 711 > 712 return 0; > 713 } > > See also: > drivers/media/v4l2-core/v4l2-ioctl.c:3140 video_put_user() warn: check that 'ev32' doesn't leak information (struct has a hole after 'type') > drivers/media/v4l2-core/v4l2-ioctl.c:3165 video_put_user() warn: check that 'vb32' doesn't leak information (struct has a hole after 'memory') These get a bit uglier when we try not to change the layout: diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index 291ec28cdf6b..6556e8eeb65e 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -735,6 +735,7 @@ long int video_ioctl2(struct file *file, */ struct v4l2_event_time32 { __u32 type; + __u8 pad[__alignof(__u64) - __alignof(__u32)]; union { struct v4l2_event_vsync vsync; struct v4l2_event_ctrl ctrl; @@ -764,6 +765,7 @@ struct v4l2_buffer_time32 { /* memory location */ __u32 memory; + __u8 pad[__alignof(void *) - __alignof(__u32)]; union { __u32 offset; unsigned long userptr; I liked the way the current solution looks, but maybe it's better to use an explicit memset and compound expression to force the compiler to do the right thing here. This should also be safe and lead to the same object code in most cases, right? diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index b68ff06009cd..cb1b66463402 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -3125,7 +3125,10 @@ 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 = { + struct v4l2_event_time32 ev32; + + memset(ev32, 0, sizeof(ev32)); + ev32 = (struct v4l2_event_time32){ .type = ev->type, .pending = ev->pending, .sequence = ev->sequence, Arnd ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [bug report] compat_ioctl: move CDROM_SEND_PACKET handling into scsi 2020-01-07 15:03 ` [bug report] compat_ioctl: move CDROM_SEND_PACKET handling into scsi Arnd Bergmann @ 2020-01-07 15:16 ` Dan Carpenter 2020-01-10 15:21 ` Arnd Bergmann 0 siblings, 1 reply; 3+ messages in thread From: Dan Carpenter @ 2020-01-07 15:16 UTC (permalink / raw) To: Arnd Bergmann; +Cc: linux-block, Hans Verkuil, Linux Media Mailing List On Tue, Jan 07, 2020 at 04:03:12PM +0100, Arnd Bergmann wrote: > On Tue, Jan 7, 2020 at 9:49 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > Hello Arnd Bergmann, > > > > The patch f3ee6e63a9df: "compat_ioctl: move CDROM_SEND_PACKET > > handling into scsi" from Nov 28, 2019, leads to the following static > > checker warning: > > > > block/scsi_ioctl.c:703 scsi_put_cdrom_generic_arg() > > warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction') > > > > block/scsi_ioctl.c > > 686 static int scsi_put_cdrom_generic_arg(const struct cdrom_generic_command *cgc, > > 687 void __user *arg) > > 688 { > > 689 #ifdef CONFIG_COMPAT > > 690 if (in_compat_syscall()) { > > 691 struct compat_cdrom_generic_command cgc32 = { > > 692 .buffer = (uintptr_t)(cgc->buffer), > > 693 .buflen = cgc->buflen, > > 694 .stat = cgc->stat, > > 695 .sense = (uintptr_t)(cgc->sense), > > 696 .data_direction = cgc->data_direction, > > 697 .quiet = cgc->quiet, > > 698 .timeout = cgc->timeout, > > 699 .reserved[0] = (uintptr_t)(cgc->reserved[0]), > > 700 }; > > > > It's possible that initializations like this don't clear out the struct > > hole but I haven't seen a compiler which is affected. So maybe it's > > fine? > > I thlought we already rely on this to initialize the entire structure, but > trying out a test case shows that it does happen: There aren't that many cases where we rely on it to happen. Under 20 so far as Smatch can detect. I'm not really certain what the correct approach is to deal with them... I think they pretty much all work fine with existing compilers. regards, dan carpenter ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] compat_ioctl: move CDROM_SEND_PACKET handling into scsi 2020-01-07 15:16 ` Dan Carpenter @ 2020-01-10 15:21 ` Arnd Bergmann 0 siblings, 0 replies; 3+ messages in thread From: Arnd Bergmann @ 2020-01-10 15:21 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-block, Hans Verkuil, Linux Media Mailing List On Tue, Jan 7, 2020 at 4:17 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Jan 07, 2020 at 04:03:12PM +0100, Arnd Bergmann wrote: > > On Tue, Jan 7, 2020 at 9:49 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > > Hello Arnd Bergmann, > > > > > > The patch f3ee6e63a9df: "compat_ioctl: move CDROM_SEND_PACKET > > > handling into scsi" from Nov 28, 2019, leads to the following static > > > checker warning: > > > > > > block/scsi_ioctl.c:703 scsi_put_cdrom_generic_arg() > > > warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction') > > > > > > block/scsi_ioctl.c > > > 686 static int scsi_put_cdrom_generic_arg(const struct cdrom_generic_command *cgc, > > > 687 void __user *arg) > > > 688 { > > > 689 #ifdef CONFIG_COMPAT > > > 690 if (in_compat_syscall()) { > > > 691 struct compat_cdrom_generic_command cgc32 = { > > > 692 .buffer = (uintptr_t)(cgc->buffer), > > > 693 .buflen = cgc->buflen, > > > 694 .stat = cgc->stat, > > > 695 .sense = (uintptr_t)(cgc->sense), > > > 696 .data_direction = cgc->data_direction, > > > 697 .quiet = cgc->quiet, > > > 698 .timeout = cgc->timeout, > > > 699 .reserved[0] = (uintptr_t)(cgc->reserved[0]), > > > 700 }; > > > > > > It's possible that initializations like this don't clear out the struct > > > hole but I haven't seen a compiler which is affected. So maybe it's > > > fine? > > > > I thlought we already rely on this to initialize the entire structure, but > > trying out a test case shows that it does happen: > > There aren't that many cases where we rely on it to happen. Under 20 > so far as Smatch can detect. I'm not really certain what the correct > approach is to deal with them... I think they pretty much all work > fine with existing compilers. After looking a bit more into this, I'm now fairly convinced this is a real problem. On gcc, this is prevented from causing too much harm by the structleak plugin, but that is not always enabled. I'll send fixes for the ones I recently introduced. Can you send me a list of the other instances that smatch finds? Maybe I can take a look at those as well. Arnd ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-10 15:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200107084659.uyaucu73yd5rhim3@kili.mountain>
2020-01-07 15:03 ` [bug report] compat_ioctl: move CDROM_SEND_PACKET handling into scsi Arnd Bergmann
2020-01-07 15:16 ` Dan Carpenter
2020-01-10 15:21 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox