[PATCH] cdrom: information leak in cdrom_ioctl_media_changed()

[PATCH] cdrom: information leak in cdrom_ioctl_media_changed()

[Dan Carpenter]

This cast is wrong.  "cdi->capacity" is an int and "arg" is an unsigned
long.  The way the check is written now, if one of the high 32 bits is
set then we could read outside the info->slots[] array.

This bug is pretty old and it predates git.

Cc: stable@vger.kernel.org
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 8327478effd0..bfc566d3f31a 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2371,7 +2371,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 	if (!CDROM_CAN(CDC_SELECT_DISC) || arg == CDSL_CURRENT)
 		return media_changed(cdi, 1);
 
-	if ((unsigned int)arg >= cdi->capacity)
+	if (arg >= cdi->capacity)
 		return -EINVAL;
 
 	info = kmalloc(sizeof(*info), GFP_KERNEL);

Re: [PATCH] cdrom: information leak in cdrom_ioctl_media_changed()

[Christoph Hellwig]

On Wed, Apr 18, 2018 at 12:51:31PM +0300, Dan Carpenter wrote:
> This cast is wrong.  "cdi->capacity" is an int and "arg" is an unsigned
> long.  The way the check is written now, if one of the high 32 bits is
> set then we could read outside the info->slots[] array.
> 
> This bug is pretty old and it predates git.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] cdrom: information leak in cdrom_ioctl_media_changed()

[Jens Axboe]

On 4/18/18 3:51 AM, Dan Carpenter wrote:
> This cast is wrong.  "cdi->capacity" is an int and "arg" is an unsigned
> long.  The way the check is written now, if one of the high 32 bits is
> set then we could read outside the info->slots[] array.
> 
> This bug is pretty old and it predates git.

Thanks Dan, applied.

-- 
Jens Axboe

Re: [PATCH] cdrom: information leak in cdrom_ioctl_media_changed()

[Scott Bauer]

On Wed, Apr 18, 2018 at 12:51:31PM +0300, Dan Carpenter wrote:
> This cast is wrong.  "cdi->capacity" is an int and "arg" is an unsigned
> long.  The way the check is written now, if one of the high 32 bits is
> set then we could read outside the info->slots[] array.
> 
> This bug is pretty old and it predates git.


There seems to be another one in this file too. We can send an arg that when type'd to int will be negative, or like the above bug the upper 32 bits will be cast-away.


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 can submit a normal patch if there are no objections.