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); }
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
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
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
Jens Axboe -
Scott Bauer