| Message ID | 20250516174020.118229-1-ant.v.moryakov@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | drivers: scsi: fix: memory leak in do_scsi_scan_one() | expand |
On Fri, May 16, 2025 at 08:40:20PM +0300, ant.v.moryakov@gmail.com wrote: > From: Anton Moryakov <ant.v.moryakov@gmail.com> > > Free allocated name buffer when blk_create_devicef() fails to prevent > memory leak. After successful device creation, the name ownership is > transferred to the device structure and should not be freed manually. > > Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>" > --- > drivers/scsi/scsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index cd0b84c0622..a9e364d3fdb 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -522,8 +522,10 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) > return log_msg_ret("pro", ret); > > ret = bootdev_setup_for_sibling_blk(bdev, "scsi_bootdev"); > - if (ret) > + if (ret) { > + free(name); > return log_msg_ret("bd", ret); > + } > > if (verbose) { > printf(" Device %d: ", bdesc->devnum); Your commit message and code changes do not match up. The free() here is at the end and not by the call to blk_create_devicef(). That said, looking at blk_create_devicef() and all of the callers, what we really need I think is to audit all of the callers and update/correct them. The third parameter, "name" is only used to print to the next string that's created, so an on-stack str[X], snprintf(...) is fine and what's usually done. The places calling sprintf(...) not snprintf should be updated for safety. The scsi case of allocating on stack and then strdup'ing that should be changed to just on stack. I would check with Heinrich and Ilias about the lib/efi_driver/efi_block_device.c::efi_bl_create_block_device() case to be clear that there's a good reason it's not on-stack. Thanks!
Am 4. Juni 2025 20:24:01 MESZ schrieb Tom Rini <trini@konsulko.com>: >On Fri, May 16, 2025 at 08:40:20PM +0300, ant.v.moryakov@gmail.com wrote: > >> From: Anton Moryakov <ant.v.moryakov@gmail.com> >> >> Free allocated name buffer when blk_create_devicef() fails to prevent >> memory leak. After successful device creation, the name ownership is >> transferred to the device structure and should not be freed manually. >> >> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>" >> --- >> drivers/scsi/scsi.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c >> index cd0b84c0622..a9e364d3fdb 100644 >> --- a/drivers/scsi/scsi.c >> +++ b/drivers/scsi/scsi.c >> @@ -522,8 +522,10 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) >> return log_msg_ret("pro", ret); >> >> ret = bootdev_setup_for_sibling_blk(bdev, "scsi_bootdev"); >> - if (ret) >> + if (ret) { >> + free(name); >> return log_msg_ret("bd", ret); >> + } >> >> if (verbose) { >> printf(" Device %d: ", bdesc->devnum); > >Your commit message and code changes do not match up. The free() here is >at the end and not by the call to blk_create_devicef(). That said, >looking at blk_create_devicef() and all of the callers, what we really >need I think is to audit all of the callers and update/correct them. The >third parameter, "name" is only used to print to the next string that's >created, so an on-stack str[X], snprintf(...) is fine and what's usually >done. The places calling sprintf(...) not snprintf should be updated for >safety. The scsi case of allocating on stack and then strdup'ing that >should be changed to just on stack. I would check with Heinrich and >Ilias about the >lib/efi_driver/efi_block_device.c::efi_bl_create_block_device() case to >be clear that there's a good reason it's not on-stack. Thanks! > We should start with a proper documentation of blk_create_devicef() in blk.h describing the intended content of name and how it is used in the function. Afterwards we should fix efi_driver: use blk_create_devicef() <https://github.com/trini/u-boot/commit/640c6c6cbaafa1b049118d431cf218d9dce3cdd8> Best regards Heinrich
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index cd0b84c0622..a9e364d3fdb 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -522,8 +522,10 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) return log_msg_ret("pro", ret); ret = bootdev_setup_for_sibling_blk(bdev, "scsi_bootdev"); - if (ret) + if (ret) { + free(name); return log_msg_ret("bd", ret); + } if (verbose) { printf(" Device %d: ", bdesc->devnum);