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

[Arnd Bergmann <arnd@arndb.de>]

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