Re: [PATCH] cdrom: Fix info leak/OOB read in cdrom_ioctl_drive_status

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Apr 27, 2018 at 08:43:03AM -0600, Scott Bauer wrote:
> 
> 
> On 04/27/2018 06:41 AM, Dan Carpenter wrote:
> > I sent you an email to send this patch, but reviewing it now it's not
> > actually a run time bug.  The cdrom_slot_status() function takes an
> > integer argument so it works.
> 
> It's still� runtime bug... I should reword the commit a bit to reflect that it's not
> like the upper 32 bit issue that you had found. Look at it this way, ints can be negative, right?
>

Oh.  Yeah.  Duh...

> The check is as follows:
> 
> 2545:	if (((int)arg >= cdi->capacity))
> 		return -EINVAL <https://elixir.bootlin.com/linux/v4.17-rc2/ident/EINVAL>;
> 	return cdrom_slot_status <https://elixir.bootlin.com/linux/v4.17-rc2/ident/cdrom_slot_status>(cdi, arg); so if (-65536 >= cdi->capacity) it's not so we don't return -einval. And we pass a negative index into cdrom_slot_status.
> 
> 
> where we do the following (https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/cdrom/cdrom.c#L1336):
> 
> 1336:
> 	if (info->slots <https://elixir.bootlin.com/linux/v4.17-rc2/ident/slots>[slot <https://elixir.bootlin.com/linux/v4.17-rc2/ident/slot>].disc_present)
> 		ret = CDS_DISC_OK <https://elixir.bootlin.com/linux/v4.17-rc2/ident/CDS_DISC_OK>;
> 
> 
> 
> >
> > I'm working on a static checker warning for these kinds of bugs:
> >
> > drivers/cdrom/cdrom.c:2444 cdrom_ioctl_select_disc() warn: truncated comparison 'arg' 'u64max' to 's32max'
> >
> > drivers/cdrom/cdrom.c
> >   2435  static int cdrom_ioctl_select_disc(struct cdrom_device_info *cdi,
> >   2436                  unsigned long arg)
> >   2437  {
> >   2438          cd_dbg(CD_DO_IOCTL, "entering CDROM_SELECT_DISC\n");
> >   2439  
> >   2440          if (!CDROM_CAN(CDC_SELECT_DISC))
> >   2441                  return -ENOSYS;
> >   2442  
> >   2443          if (arg != CDSL_CURRENT && arg != CDSL_NONE) {
> >   2444                  if ((int)arg >= cdi->capacity)
> >                             ^^^^^^^^^^^^^^^^^^^^^^^^^
> >   2445                          return -EINVAL;
> >   2446          }
> >   2447  
> >   2448          /*
> >   2449           * ->select_disc is a hook to allow a driver-specific way of
> >   2450           * seleting disc.  However, since there is no equivalent hook for
> >   2451           * cdrom_slot_status this may not actually be useful...
> >   2452           */
> >   2453          if (cdi->ops->select_disc)
> >   2454                  return cdi->ops->select_disc(cdi, arg);
> >                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > ->select_disc() also take an int so it's fine (plus there is no such
> > function so it's dead code).
> >
> >   2455  
> >   2456          cd_dbg(CD_CHANGER, "Using generic cdrom_select_disc()\n");
> >   2457          return cdrom_select_disc(cdi, arg);
> >                                               ^^^
> > Also an int.
> >
> >   2458  }
> >
> > So I think it's a good idea to fix these just for cleanliness and to
> > silence the static checker warnings but it doesn't affect runtime.
> 
> Yeah, this one was "fine" aside from being messy, that's why I didn't send a patch for it.
> 

I'm not convinced any more.  Could you patch it and resend?  We could
end up sending invalid commands to the cdrom firmware when we do
cdrom_load_unload() at the end of the cdrom_select_disc() function.
Proably there is no impact but we may as well fix it.  Here is my
analysis if you are curious:

  1371  /* If SLOT < 0, unload the current slot.  Otherwise, try to load SLOT. */
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
CDSL_CURRENT is INT_MAX and CDSL_NONE is "INT_MAX - 1" but
cdrom_select_disc() calls this with slot set to -1.

  1372  static int cdrom_load_unload(struct cdrom_device_info *cdi, int slot) 
  1373  {
  1374          struct packet_command cgc;
  1375  
  1376          cd_dbg(CD_CHANGER, "entering cdrom_load_unload()\n");
  1377          if (cdi->sanyo_slot && slot < 0)
  1378                  return 0;
  1379  
  1380          init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
  1381          cgc.cmd[0] = GPCMD_LOAD_UNLOAD;
  1382          cgc.cmd[4] = 2 + (slot >= 0);
                ^^^^^^^^^^
So cmd[4] is 2.

  1383          cgc.cmd[8] = slot;
                ^^^^^^^^^^^^^^^^^
Here were setting cmd[8] to any u8 value we choose.

  1384          cgc.timeout = 60 * HZ;
  1385  
  1386          /* The Sanyo 3 CD changer uses byte 7 of the 
  1387          GPCMD_TEST_UNIT_READY to command to switch CDs instead of
  1388          using the GPCMD_LOAD_UNLOAD opcode. */
  1389          if (cdi->sanyo_slot && -1 < slot) {
  1390                  cgc.cmd[0] = GPCMD_TEST_UNIT_READY;
  1391                  cgc.cmd[7] = slot;
  1392                  cgc.cmd[4] = cgc.cmd[8] = 0;
  1393                  cdi->sanyo_slot = slot ? slot : 3;
  1394          }
  1395  
  1396          return cdi->ops->generic_packet(cdi, &cgc);
  1397  }

> P.S. Is your static analysis tooling available for the general public to look at?

Sure.  I've been dorking with it for a couple days and I haven't tested
the latest version except on drivers/cdrom/cdrom.c so let me do some
more testing and then I'll post it.

regards,
dan carpenter