cdrom: use struct_size() in changer info allocation

[PATCH v2] cdrom: use struct_size() in changer info allocation

Posted by Jędrzej Szoszorek 3 days, 11 hours ago

Replace the obsolete `kmalloc_obj()` pattern with the
`kzalloc(struct_size(), ...)` idiom when allocating `struct cdrom_changer_info`.

This change ensures inherent protection against integer overflow
vulnerabilities during the calculation of the allocation size, as
`struct_size()` safely computes the size of the structure combined with
its flexible array member.

This addresses memory safety concerns without altering the driver's
logic or ABI, guaranteeing zero regressions for legacy user-space tools.

Signed-off-by: Jędrzej Szoszorek <jedrzej.szoszo@gmail.com>
---
- Dropped all cosmetic and formatting changes (churn) from v1.
  - Dropped ioctl error code changes (-ENOSYS -> -ENOTTY) to prevent
    any userspace ABI breakage.
  - Dropped experimental per-device locking (mutex/spinlock) to avoid
    TOCTOU races and hardware lockout risks.
  - Kept only the critical memory safety fix (struct_size)

 drivers/cdrom/cdrom.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 62934cf4b..31e662c8b 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -276,6 +276,7 @@
 #include <linux/times.h>
 #include <linux/uaccess.h>
 #include <scsi/scsi_common.h>
+#include <linux/overflow.h>
 
 /* used to tell the module to turn on full debugging messages */
 static bool debug;
@@ -1341,7 +1342,7 @@ static int cdrom_slot_status(struct cdrom_device_info *cdi, int slot)
 	if (cdi->sanyo_slot)
 		return CDS_NO_INFO;
 	
-	info = kmalloc_obj(*info);
+	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -1370,7 +1371,7 @@ int cdrom_number_of_slots(struct cdrom_device_info *cdi)
 	/* cdrom_read_mech_status requires a valid value for capacity: */
 	cdi->capacity = 0; 
 
-	info = kmalloc_obj(*info);
+	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -1429,7 +1430,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
 		return cdrom_load_unload(cdi, -1);
 	}
 
-	info = kmalloc_obj(*info);
+	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -2334,7 +2335,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 	/* Prevent arg from speculatively bypassing the length check */
 	arg = array_index_nospec(arg, cdi->capacity);
 
-	info = kmalloc_obj(*info);
+	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
-- 
2.43.0

Re: [PATCH v2] cdrom: use struct_size() in changer info allocation

Posted by Phillip Potter 2 days, 14 hours ago

On Thu, Jun 04, 2026 at 09:08:28PM +0200, Jędrzej Szoszorek wrote:
> Replace the obsolete `kmalloc_obj()` pattern with the
> `kzalloc(struct_size(), ...)` idiom when allocating `struct cdrom_changer_info`.
> 
> This change ensures inherent protection against integer overflow
> vulnerabilities during the calculation of the allocation size, as
> `struct_size()` safely computes the size of the structure combined with
> its flexible array member.
> 
> This addresses memory safety concerns without altering the driver's
> logic or ABI, guaranteeing zero regressions for legacy user-space tools.
> 
> Signed-off-by: Jędrzej Szoszorek <jedrzej.szoszo@gmail.com>
> ---
> - Dropped all cosmetic and formatting changes (churn) from v1.
>   - Dropped ioctl error code changes (-ENOSYS -> -ENOTTY) to prevent
>     any userspace ABI breakage.
>   - Dropped experimental per-device locking (mutex/spinlock) to avoid
>     TOCTOU races and hardware lockout risks.
>   - Kept only the critical memory safety fix (struct_size)
> 
>  drivers/cdrom/cdrom.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 62934cf4b..31e662c8b 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -276,6 +276,7 @@
>  #include <linux/times.h>
>  #include <linux/uaccess.h>
>  #include <scsi/scsi_common.h>
> +#include <linux/overflow.h>
>  
>  /* used to tell the module to turn on full debugging messages */
>  static bool debug;
> @@ -1341,7 +1342,7 @@ static int cdrom_slot_status(struct cdrom_device_info *cdi, int slot)
>  	if (cdi->sanyo_slot)
>  		return CDS_NO_INFO;
>  	
> -	info = kmalloc_obj(*info);
> +	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> @@ -1370,7 +1371,7 @@ int cdrom_number_of_slots(struct cdrom_device_info *cdi)
>  	/* cdrom_read_mech_status requires a valid value for capacity: */
>  	cdi->capacity = 0; 
>  
> -	info = kmalloc_obj(*info);
> +	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> @@ -1429,7 +1430,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
>  		return cdrom_load_unload(cdi, -1);
>  	}
>  
> -	info = kmalloc_obj(*info);
> +	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> @@ -2334,7 +2335,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
>  	/* Prevent arg from speculatively bypassing the length check */
>  	arg = array_index_nospec(arg, cdi->capacity);
>  
> -	info = kmalloc_obj(*info);
> +	info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> -- 
> 2.43.0
> 

Hi Jędrzej,

Sorry, but this patch is just not correct. There is no flexible array
member in 'struct cdrom_changer_info', which is what all four of these
calls allocate. The slots array in that structure is sized from a
constant as quoted below:

> /* The SCSI spec says there could be 256 slots. */
> #define CDROM_MAX_SLOTS	256

The code thus incorrectly allocates this structure with the changes
(which to me is just as relevant as whether that extra space is actually
used or not).

In my view, these calls are fine as they are and I see no need to
replace them therefore, particularly for the reasons stated. There are
no "memory safety concerns" as listed in your commit description.

Regards,
Phil

Re: [PATCH v2] cdrom: use struct_size() in changer info allocation

Posted by Jedrzej Sz 2 days, 10 hours ago

Hi Phil,

Thanks for the review.

I will drop this patch.

You're right, I missed the fixed-size slots array in
struct cdrom_changer_info and incorrectly assumed a dynamically-sized
allocation pattern. In that case the memory safety rationale does not
apply.

I have a few other cleanup ideas for the driver, but I'll keep those
separate from this series.

Thanks,
Jędrzej

pt., 5 cze 2026 o 18:11 Phillip Potter <phil@philpotter.co.uk> napisał(a):
>
> On Thu, Jun 04, 2026 at 09:08:28PM +0200, Jędrzej Szoszorek wrote:
> > Replace the obsolete `kmalloc_obj()` pattern with the
> > `kzalloc(struct_size(), ...)` idiom when allocating `struct cdrom_changer_info`.
> >
> > This change ensures inherent protection against integer overflow
> > vulnerabilities during the calculation of the allocation size, as
> > `struct_size()` safely computes the size of the structure combined with
> > its flexible array member.
> >
> > This addresses memory safety concerns without altering the driver's
> > logic or ABI, guaranteeing zero regressions for legacy user-space tools.
> >
> > Signed-off-by: Jędrzej Szoszorek <jedrzej.szoszo@gmail.com>
> > ---
> > - Dropped all cosmetic and formatting changes (churn) from v1.
> >   - Dropped ioctl error code changes (-ENOSYS -> -ENOTTY) to prevent
> >     any userspace ABI breakage.
> >   - Dropped experimental per-device locking (mutex/spinlock) to avoid
> >     TOCTOU races and hardware lockout risks.
> >   - Kept only the critical memory safety fix (struct_size)
> >
> >  drivers/cdrom/cdrom.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> > index 62934cf4b..31e662c8b 100644
> > --- a/drivers/cdrom/cdrom.c
> > +++ b/drivers/cdrom/cdrom.c
> > @@ -276,6 +276,7 @@
> >  #include <linux/times.h>
> >  #include <linux/uaccess.h>
> >  #include <scsi/scsi_common.h>
> > +#include <linux/overflow.h>
> >
> >  /* used to tell the module to turn on full debugging messages */
> >  static bool debug;
> > @@ -1341,7 +1342,7 @@ static int cdrom_slot_status(struct cdrom_device_info *cdi, int slot)
> >       if (cdi->sanyo_slot)
> >               return CDS_NO_INFO;
> >
> > -     info = kmalloc_obj(*info);
> > +     info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
> >       if (!info)
> >               return -ENOMEM;
> >
> > @@ -1370,7 +1371,7 @@ int cdrom_number_of_slots(struct cdrom_device_info *cdi)
> >       /* cdrom_read_mech_status requires a valid value for capacity: */
> >       cdi->capacity = 0;
> >
> > -     info = kmalloc_obj(*info);
> > +     info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
> >       if (!info)
> >               return -ENOMEM;
> >
> > @@ -1429,7 +1430,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
> >               return cdrom_load_unload(cdi, -1);
> >       }
> >
> > -     info = kmalloc_obj(*info);
> > +     info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
> >       if (!info)
> >               return -ENOMEM;
> >
> > @@ -2334,7 +2335,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> >       /* Prevent arg from speculatively bypassing the length check */
> >       arg = array_index_nospec(arg, cdi->capacity);
> >
> > -     info = kmalloc_obj(*info);
> > +     info = kzalloc(struct_size(info, slots, cdi->capacity), GFP_KERNEL);
> >       if (!info)
> >               return -ENOMEM;
> >
> > --
> > 2.43.0
> >
>
> Hi Jędrzej,
>
> Sorry, but this patch is just not correct. There is no flexible array
> member in 'struct cdrom_changer_info', which is what all four of these
> calls allocate. The slots array in that structure is sized from a
> constant as quoted below:
>
> > /* The SCSI spec says there could be 256 slots. */
> > #define CDROM_MAX_SLOTS       256
>
> The code thus incorrectly allocates this structure with the changes
> (which to me is just as relevant as whether that extra space is actually
> used or not).
>
> In my view, these calls are fine as they are and I see no need to
> replace them therefore, particularly for the reasons stated. There are
> no "memory safety concerns" as listed in your commit description.
>
> Regards,
> Phil