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

[Phillip Potter <phil@philpotter.co.uk>]

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