On Tue, Jan 7, 2020 at 9:49 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
