| Message ID | 20180426175108.29280-1-scott.bauer@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
regards,
dan carpenter
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
Sorry, I responded to this patch that this wasn't a real bug, but then Scott corrected me that it was. Anyway, it is a bug and we haven't applied this patch yet. regards, dan carpenter On Thu, Apr 26, 2018 at 11:51:08AM -0600, Scott Bauer wrote: > Like d88b6d04: "cdrom: information leak in cdrom_ioctl_media_changed()" > > There is another cast from unsigned long to int which causes > a bounds check to fail with specially crafted input. The value is > then used as an index in the slot array in cdrom_slot_status(). > > Signed-off-by: Scott Bauer <scott.bauer@intel.com> > Signed-off-by: Scott Bauer <sbauer@plzdonthack.me> > Cc: stable@vger.kernel.org > --- > drivers/cdrom/cdrom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c > index bfc566d3f31a..8cfa10ab7abc 100644 > --- a/drivers/cdrom/cdrom.c > +++ b/drivers/cdrom/cdrom.c > @@ -2542,7 +2542,7 @@ static int cdrom_ioctl_drive_status(struct cdrom_device_info *cdi, > if (!CDROM_CAN(CDC_SELECT_DISC) || > (arg == CDSL_CURRENT || arg == CDSL_NONE)) > return cdi->ops->drive_status(cdi, CDSL_CURRENT); > - if (((int)arg >= cdi->capacity)) > + if (arg >= cdi->capacity) > return -EINVAL; > return cdrom_slot_status(cdi, arg); > } > -- > 2.14.1
On 8/29/18 6:48 AM, Dan Carpenter wrote: > Sorry, I responded to this patch that this wasn't a real bug, but then > Scott corrected me that it was. > > Anyway, it is a bug and we haven't applied this patch yet. It is now, apparently I lost track of it too. Thanks Scott.
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index bfc566d3f31a..8cfa10ab7abc 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -2542,7 +2542,7 @@ static int cdrom_ioctl_drive_status(struct cdrom_device_info *cdi, if (!CDROM_CAN(CDC_SELECT_DISC) || (arg == CDSL_CURRENT || arg == CDSL_NONE)) return cdi->ops->drive_status(cdi, CDSL_CURRENT); - if (((int)arg >= cdi->capacity)) + if (arg >= cdi->capacity) return -EINVAL; return cdrom_slot_status(cdi, arg); }