linux-kernel - Re: [RESEND PATCH] memstick: fix memory leak if card device is never registered

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:   Mon, 3 Apr 2023 16:14:55 +0200
From:   Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-mmc@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Maxim Levitsky <maximlevitsky@...il.com>,
        Alex Dubov <oakad@...oo.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Kay Sievers <kay.sievers@...y.org>, stable <stable@...nel.org>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>
Subject: Re: [RESEND PATCH] memstick: fix memory leak if card device is never
 registered

On 1.4.2023. 22:03, Greg Kroah-Hartman wrote:
> When calling dev_set_name() memory is allocated for the name for the
> struct device.  Once that structure device is registered, or attempted
> to be registerd, with the driver core, the driver core will handle
> cleaning up that memory when the device is removed from the system.
> 
> Unfortunatly for the memstick code, there is an error path that causes
> the struct device to never be registered, and so the memory allocated in
> dev_set_name will be leaked.  Fix that leak by manually freeing it right
> before the memory for the device is freed.
> 
> Cc: Maxim Levitsky <maximlevitsky@...il.com>
> Cc: Alex Dubov <oakad@...oo.com>
> Cc: Ulf Hansson <ulf.hansson@...aro.org>
> Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> Cc: Hans de Goede <hdegoede@...hat.com>
> Cc: Kay Sievers <kay.sievers@...y.org>
> Cc: linux-mmc@...r.kernel.org
> Fixes: 0252c3b4f018 ("memstick: struct device - replace bus_id with dev_name(),
> Cc: stable <stable@...nel.org>
> Co-developed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Co-developed-by: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
> ---
> RESEND as the first version had a corrupted message id and never made it
> to the mailing lists or lore.kernel.org
> 
>   drivers/memstick/core/memstick.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> index bf7667845459..bbfaf6536903 100644
> --- a/drivers/memstick/core/memstick.c
> +++ b/drivers/memstick/core/memstick.c
> @@ -410,6 +410,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>   	return card;
>   err_out:
>   	host->card = old_card;
> +	kfree_const(card->dev.kobj.name);
>   	kfree(card);
>   	return NULL;
>   }
> @@ -468,8 +469,10 @@ static void memstick_check(struct work_struct *work)
>   				put_device(&card->dev);
>   				host->card = NULL;
>   			}
> -		} else
> +		} else {
> +			kfree_const(card->dev.kobj.name);
>   			kfree(card);
> +		}
>   	}
>   
>   out_power_off:

FYI, the applied patch tested OK, no memstick leaks:

[root@...mtodorov kernel]# uname -rms
Linux 6.3.0-rc5-mt-20230401-00007-g268a637be362 x86_64
[root@...mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@...mtodorov kernel]# cat /sys/kernel/debug/kmemleak

What was applied is:

mtodorov@...ac:~/linux/kernel/linux_torvalds$ git diff master..origin/master | head -24
diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index bbfaf6536903..bf7667845459 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -410,7 +410,6 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
         return card;
  err_out:
         host->card = old_card;
-       kfree_const(card->dev.kobj.name);
         kfree(card);
         return NULL;
  }
@@ -469,10 +468,8 @@ static void memstick_check(struct work_struct *work)
                                 put_device(&card->dev);
                                 host->card = NULL;
                         }
-               } else {
-                       kfree_const(card->dev.kobj.name);
+               } else
                         kfree(card);
-               }
         }

  out_power_off:

APPENDIX

However, please note that the patch SHA-1's truncated to 12 chars might not
be the same on the other systems, so the build ID says nothing about which
mainline kernel had the patches been applied against. So `uname -rms` is
telling pretty nothing about which kernel I'm running, except that it helps
me distinguish between the builds.

Nothing to prove that:

- I am not testing the wrong kernel and
- that the right patches have been applied.

Changing CONFIG_LOCALVERSION causes copious recompilations, even with ccache
it takes about 4x the time needed to compile CONFIG_LOCALVERSION_AUTO=y.

Do I make any sense with this?

I am adding Cc: to Mr. Bagas, for we spoke already about kernel versioning
in the case of manually applied patches.

Regards,
Mirsad

-- 
Mirsad Todorovac
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb
Republic of Croatia, the European Union

Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu