linux-kernel - RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Mar 2023 10:38:08 +0900
From:   "Bumwoo Lee" <bw365.lee@...sung.com>
To:     <myungjoo.ham@...sung.com>,
        "'Chanwoo Choi'" <cw00.choi@...sung.com>,
        <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify
 extcon register function

Hello.

As you can see, edev->cables are freed if extcon_alloc_cables() function return error handling in extcon_dev_register()
Other added functions are also same.

Because it's functionalized, apart from this, do you want to mention that it should be freed within the function? 
Please let me know your opinion.

extcon_dev_register(struct extcon_dev *edev){
...

	ret = extcon_alloc_cables(edev);
	if (ret)
		goto err_alloc_cables;

...

err_alloc_cables:
 	if (edev->max_supported)
 		kfree(edev->cables);


Regards,
Bumwoo

-----Original Message-----
From: MyungJoo Ham <myungjoo.ham@...sung.com> 
Sent: Friday, February 24, 2023 7:03 PM
To: Bumwoo Lee <bw365.lee@...sung.com>; Chanwoo Choi <cw00.choi@...sung.com>; linux-kernel@...r.kernel.org
Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

>--------- Original Message ---------
>Sender : 이범우 <bw365.lee@...sung.com>Product S/W Lab(VD)/삼성전자 Date : 
>2023-02-20 14:45 (GMT+9) Title : [PATCH v2 2/4] extcon: Added 
>extcon_alloc_cables to simplify extcon register function
> 
>The cable allocation part is functionalized from extcon_dev_register.
>
>Signed-off-by: Bumwoo Lee <bw365.lee@...sung.com>
>---
> drivers/extcon/extcon.c | 104 +++++++++++++++++++++++-----------------
> 1 file changed, 59 insertions(+), 45 deletions(-)
>
>diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index 
>adcf01132f70..3c2f540785e8 100644
>--- a/drivers/extcon/extcon.c
>+++ b/drivers/extcon/extcon.c
>@@ -1070,6 +1070,61 @@ void extcon_dev_free(struct extcon_dev *edev)  }  
>EXPORT_SYMBOL_GPL(extcon_dev_free);
> 
>+/**
>+ * extcon_alloc_cables() - alloc the cables for extcon device
>+ * @edev:        extcon device which has cables
>+ *
>+ * Returns 0 if success or error number if fail.
>+ */
>+static int extcon_alloc_cables(struct extcon_dev *edev) {
>+        int index;
>+        char *str;
>+        struct extcon_cable *cable;
>+
>+        if (!edev->max_supported)
>+                return 0;
>+
>+        edev->cables = kcalloc(edev->max_supported,
>+                               sizeof(struct extcon_cable),
>+                               GFP_KERNEL);
>+        if (!edev->cables)
>+                return -ENOMEM;
>+
>+        for (index = 0; index < edev->max_supported; index++) {
>+                cable = &edev->cables[index];
>+
>+                str = kasprintf(GFP_KERNEL, "cable.%d", index);
>+                if (!str) {
>+                        for (index--; index >= 0; index--) {
>+                                cable = &edev->cables[index];
>+                                kfree(cable->attr_g.name);
>+                        }
>+                        return -ENOMEM;

You have a memory leak.
edev->cables is allocated and
you are not freeing it.

In the previous code, it was freed by
having different err-goto labels.

Please check if you have similar errors
in other patches of this series.

...

>@@ -1282,7 +1296,7 @@ int extcon_dev_register(struct extcon_dev *edev)
> err_alloc_cables:
>         if (edev->max_supported)
>                 kfree(edev->cables);
>-err_sysfs_alloc:
>+
>         return ret;
> }
> EXPORT_SYMBOL_GPL(extcon_dev_register);
>--
>2.35.1
>
>

Cheers,
MyungJoo.