[PATCH -next] scsi: core: fix double free in raid_component_add()

[PATCH -next] scsi: core: fix double free in raid_component_add()

[Zhu Wang]

Previous commit 04b5b5cb0136 ("scsi: core: Fix possible memory leak if
device_add() fails") introduced a double free when device_add() failed.
When device_add() failed, the put_device(&rc->dev) is called, which will
call raid_component_release(), and this function will call
put_device(rc->dev.parent) and kfree(rc), but in the error path of
raid_component_release() above two functions are called again, so 'rc' are
freed twice.

We do not just revert the patch, since the memory allocated by
dev_set_name() is not released in the error path, it should be released
through calling put_device(&rc->dev) which will call kobject_cleanup(),
and the device name will be released in it. Though the commit 04b5b5cb0136
("scsi: core: Fix possible memory leak if device_add() fails") fixed the
memory leak, it didn't consider the double free problem. We removed
put_device(rc->dev.parent) and kfree(rc) in the error path, and we moved
put_device(&rc->dev) after rd->component-count--, since after
put_device(&rc->dev) is called, 'rc' is freed, so list_del(&rc->node)
cannot be called again.

Fixes: 04b5b5cb0136 ("scsi: core: Fix possible memory leak if device_add() fails")
Signed-off-by: Zhu Wang <wangzhu9@huawei.com>
---
 drivers/scsi/raid_class.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/raid_class.c b/drivers/scsi/raid_class.c
index 711252e52d8e..86ed1f66d749 100644
--- a/drivers/scsi/raid_class.c
+++ b/drivers/scsi/raid_class.c
@@ -248,11 +248,9 @@ int raid_component_add(struct raid_template *r,struct device *raid_dev,
 	return 0;
 
 err_out:
-	put_device(&rc->dev);
 	list_del(&rc->node);
 	rd->component_count--;
-	put_device(component_dev);
-	kfree(rc);
+	put_device(&rc->dev);
 	return err;
 }
 EXPORT_SYMBOL(raid_component_add);
-- 
2.34.1

Re: [PATCH -next] scsi: core: fix double free in raid_component_add()

[Bart Van Assche]

On 8/19/23 01:03, Zhu Wang wrote:
> Previous commit 04b5b5cb0136 ("scsi: core: Fix possible memory leak if
> device_add() fails") introduced a double free when device_add() failed.
> When device_add() failed, the put_device(&rc->dev) is called, which will
> call raid_component_release(), and this function will call
> put_device(rc->dev.parent) and kfree(rc), but in the error path of
> raid_component_release() above two functions are called again, so 'rc' are
> freed twice.
> 
> We do not just revert the patch, since the memory allocated by
> dev_set_name() is not released in the error path, it should be released
> through calling put_device(&rc->dev) which will call kobject_cleanup(),
> and the device name will be released in it. Though the commit 04b5b5cb0136
> ("scsi: core: Fix possible memory leak if device_add() fails") fixed the
> memory leak, it didn't consider the double free problem. We removed
> put_device(rc->dev.parent) and kfree(rc) in the error path, and we moved
> put_device(&rc->dev) after rd->component-count--, since after
> put_device(&rc->dev) is called, 'rc' is freed, so list_del(&rc->node)
> cannot be called again.
> 
> Fixes: 04b5b5cb0136 ("scsi: core: Fix possible memory leak if device_add() fails")
> Signed-off-by: Zhu Wang <wangzhu9@huawei.com>
> ---
>   drivers/scsi/raid_class.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/raid_class.c b/drivers/scsi/raid_class.c
> index 711252e52d8e..86ed1f66d749 100644
> --- a/drivers/scsi/raid_class.c
> +++ b/drivers/scsi/raid_class.c
> @@ -248,11 +248,9 @@ int raid_component_add(struct raid_template *r,struct device *raid_dev,
>   	return 0;
>   
>   err_out:
> -	put_device(&rc->dev);
>   	list_del(&rc->node);
>   	rd->component_count--;
> -	put_device(component_dev);
> -	kfree(rc);
> +	put_device(&rc->dev);
>   	return err;
>   }
>   EXPORT_SYMBOL(raid_component_add);

Please consider replacing the above patch by the patch below:

[PATCH] scsi: Remove raid_component_add()

The raid_component_add() function was added to the kernel tree via
patch "[SCSI] embryonic RAID class" (2005). Remove this function since
it never has had any callers in the Linux kernel.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/raid_class.c  | 38 --------------------------------------
  include/linux/raid_class.h |  4 ----
  2 files changed, 42 deletions(-)

diff --git a/drivers/scsi/raid_class.c b/drivers/scsi/raid_class.c
index 898a0bdf8df6..99f806d1d317 100644
--- a/drivers/scsi/raid_class.c
+++ b/drivers/scsi/raid_class.c
@@ -218,44 +218,6 @@ static void raid_component_release(struct device *dev)
  	kfree(rc);
  }

-int raid_component_add(struct raid_template *r,struct device *raid_dev,
-		       struct device *component_dev)
-{
-	struct device *cdev =
-		attribute_container_find_class_device(&r->raid_attrs.ac,
-						      raid_dev);
-	struct raid_component *rc;
-	struct raid_data *rd = dev_get_drvdata(cdev);
-	int err;
-
-	rc = kzalloc(sizeof(*rc), GFP_KERNEL);
-	if (!rc)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(&rc->node);
-	device_initialize(&rc->dev);
-	rc->dev.release = raid_component_release;
-	rc->dev.parent = get_device(component_dev);
-	rc->num = rd->component_count++;
-
-	dev_set_name(&rc->dev, "component-%d", rc->num);
-	list_add_tail(&rc->node, &rd->component_list);
-	rc->dev.class = &raid_class.class;
-	err = device_add(&rc->dev);
-	if (err)
-		goto err_out;
-
-	return 0;
-
-err_out:
-	list_del(&rc->node);
-	rd->component_count--;
-	put_device(component_dev);
-	kfree(rc);
-	return err;
-}
-EXPORT_SYMBOL(raid_component_add);
-
  struct raid_template *
  raid_class_attach(struct raid_function_template *ft)
  {
diff --git a/include/linux/raid_class.h b/include/linux/raid_class.h
index 6a9b177d5c41..e50416ba9cd9 100644
--- a/include/linux/raid_class.h
+++ b/include/linux/raid_class.h
@@ -77,7 +77,3 @@ DEFINE_RAID_ATTRIBUTE(enum raid_state, state)
  	
  struct raid_template *raid_class_attach(struct raid_function_template *);
  void raid_class_release(struct raid_template *);
-
-int __must_check raid_component_add(struct raid_template *, struct device *,
-				    struct device *);
-

Re: Re: [PATCH -next] scsi: core: fix double free in raid_component_add()

[kernel test robot]

Hi Bart,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[cannot apply to next-20230821 jejb-scsi/for-next linus/master v6.5-rc7 next-20230822]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bart-Van-Assche/Re-PATCH-next-scsi-core-fix-double-free-in-raid_component_add/20230822-035432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/baecaad7-7124-b9ae-ab79-1b7c6fa95c98%40acm.org
patch subject: Re: [PATCH -next] scsi: core: fix double free in raid_component_add()
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230823/202308230741.jbMc6KYG-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230823/202308230741.jbMc6KYG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308230741.jbMc6KYG-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/scsi/raid_class.c:212:13: warning: 'raid_component_release' defined but not used [-Wunused-function]
     212 | static void raid_component_release(struct device *dev)
         |             ^~~~~~~~~~~~~~~~~~~~~~


vim +/raid_component_release +212 drivers/scsi/raid_class.c

b1081ea6f000de James Bottomley 2005-11-06  211  
ee959b00c335d7 Tony Jones      2008-02-22 @212  static void raid_component_release(struct device *dev)
b1081ea6f000de James Bottomley 2005-11-06  213  {
ee959b00c335d7 Tony Jones      2008-02-22  214  	struct raid_component *rc =
ee959b00c335d7 Tony Jones      2008-02-22  215  		container_of(dev, struct raid_component, dev);
ee959b00c335d7 Tony Jones      2008-02-22  216  	dev_printk(KERN_ERR, rc->dev.parent, "COMPONENT RELEASE\n");
ee959b00c335d7 Tony Jones      2008-02-22  217  	put_device(rc->dev.parent);
b1081ea6f000de James Bottomley 2005-11-06  218  	kfree(rc);
b1081ea6f000de James Bottomley 2005-11-06  219  }
61a7afa2c476a3 James Bottomley 2005-08-16  220  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki