Skip to contentSkip to conversation

cdrom: information leak in cdrom_ioctl_media_changed()

PATCH In review Dan Carpenter 4 participants 4 messages Apr 18, 2018 – Apr 25, 2018

Subsystems (2)
Block layer Uniform cdrom driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 cdrom: information leak in cdrom_ioctl_media_changed()

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 [email protected]
Signed-off-by Dan Carpenter <[email protected]>
drivers/cdrom/cdrom.c +1 -1
index 8327478effd0..bfc566d3f31a 100644
@@ -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);
Reviewed-by on v1 1/1
#1 lore raw
Quoted 5 lines · 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.

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>
maintainer
#2 lore raw
In reply to Dan Carpenter
Quoted 5 lines · 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.

Thanks Dan, applied.

Signature · 1 line
Jens Axboe
#3 lore raw
In reply to Dan Carpenter
Quoted 5 lines · 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.

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.

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

I can submit a normal patch if there are no objections.