linux-kernel - Re: [PATCH v3] most: core: fix resource leak in most_register

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] [day] [month] [year] [list]

Message-ID: <aSfxKwd7akeWllAr@stanley.mountain>
Date: Thu, 27 Nov 2025 09:35:23 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Navaneeth K <knavaneeth786@...il.com>
Cc: parthiban.veerasooran@...rochip.com, christian.gromm@...rochip.com,
	gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org, abdun.nihaal@...il.com
Subject: Re: [PATCH v3] most: core: fix resource leak in
 most_register_interface error paths

On Wed, Nov 26, 2025 at 10:08:56PM +0000, Navaneeth K wrote:
> The function most_register_interface() did not correctly release resources
> if it failed early (before device_register). In these cases, it returned
> an error code immediately, leaking the memory allocated for the interface.
> 
> Fix this by initializing the device early via device_initialize() and
> calling put_device() on all error paths. This ensures the release
> callback is triggered to free memory.
> 
> Switch to using device_add() instead of device_register() to handle
> the split initialization.
> 
> Acked-by: Abdun Nihaal <abdun.nihaal@...il.com>
> Signed-off-by: Navaneeth K <knavaneeth786@...il.com>
> ---

Could we add this information to the commit message?

"The most_register_interface() is expected to call put_device() on
error which frees the resources allocated in the caller.  The
put_device() either calls release_mdev() or dim2_release(),
depending on the caller."

> Changes in v3:
>  - Dropped the USB cleanup patch as it was already applied upstream.
>  - Added Reported-by and Acked-by tags.
> 
>  drivers/most/core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/most/core.c b/drivers/most/core.c
> index da319d108ea1d..8635fd08035e9 100644
> --- a/drivers/most/core.c
> +++ b/drivers/most/core.c
> @@ -1283,18 +1283,23 @@ int most_register_interface(struct most_interface *iface)
>  	struct most_channel *c;
>  
>  	if (!iface || !iface->enqueue || !iface->configure ||
> -	    !iface->poison_channel || (iface->num_channels > MAX_CHANNELS))
> +	    !iface->poison_channel || (iface->num_channels > MAX_CHANNELS) ||
> +	    !iface->dev)
>  		return -EINVAL;

Don't add this NULL check.  We never pass a NULL pointer here.  It's
just confusing to check for it.

>  
> +	device_initialize(iface->dev);
> +
>  	id = ida_alloc(&mdev_id, GFP_KERNEL);
>  	if (id < 0) {
>  		dev_err(iface->dev, "Failed to allocate device ID\n");
> +		put_device(iface->dev);
>  		return id;
>  	}
>  
>  	iface->p = kzalloc(sizeof(*iface->p), GFP_KERNEL);
>  	if (!iface->p) {
>  		ida_free(&mdev_id, id);
> +		put_device(iface->dev);
>  		return -ENOMEM;

This is an annoying thing because the function uses a mix of direct
returns and an unwind ladder to clean up.  It should just use an
unwind ladder.  So this should be a "goto err_free_ida;".  That's
unrelated to this patch so don't mix the two but if someone wanted to
fix it, that would be great.

KTODO: cleanup error handling in most_register_interface()

>  	}
>  
> @@ -1304,7 +1309,7 @@ int most_register_interface(struct most_interface *iface)
>  	iface->dev->bus = &mostbus;
>  	iface->dev->groups = interface_attr_groups;
>  	dev_set_drvdata(iface->dev, iface);
> -	if (device_register(iface->dev)) {
> +	if (device_add(iface->dev)) {

Unrelated and minor, but this should really be:

	ret = device_add(iface->dev);
	if (ret) {

>  		dev_err(iface->dev, "Failed to register interface device\n");
>  		kfree(iface->p);
>  		put_device(iface->dev);

Again, unrelated to your patch, but there is still a bug in the
error handling because if the if (device_register(&c->dev)) { call
fails then we don't call list_del(&c->list) so it's a use after free.

The way to review these is, does the unwind ladder match the release
function, most_deregister_interface()?  In this case the release
function calls c->pipe0.comp->disconnect_channel() which is not
necessary when we're cleaning up from probe() so that's fine.  But
the list_del(&c->list) part is necessary.

Another very minor thing, but if I were writing this, I would move
the put_device(&c->dev) before the goto.  It's better to clean up
partial iterations inside the loop, without doing gotos there.  That
way you can add new rungs to the top of the unwind ladder.

I have a blog about this:
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter