Re: [bug report] compat_ioctl: move CDROM_SEND_PACKET handling into scsi

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux