cdrom: Fix info leak/OOB read in cdrom_ioctl_drive_status

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.

Quoted 3 lines · Dan Carpenter

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?

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>;

Quoted 37 lines

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.

Quoted 4 lines

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.

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

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 <[email protected]>

Signed-off-by Scott Bauer <[email protected]>

Cc [email protected]

---
 drivers/cdrom/cdrom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cdrom/cdrom.c +1 -1

index bfc566d3f31a..8cfa10ab7abc 100644
@@ -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);
 }

Signature · 1 line

2.14.1

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.