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

[Jedrzej Sz]

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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > - 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