[PATCH] ALSA: jack: Fix control leak on jack allocation failure

[PATCH] ALSA: jack: Fix control leak on jack allocation failure

[Sreeraj S Kurup]

snd_jack_new() creates an ALSA jack control before allocating the
jack structure.

If the jack structure allocation fails, the function returns
-ENOMEM without removing the previously created control.

Remove the control using snd_ctl_remove() to release the control
and its private data before returning -ENOMEM.

Initialize the control list before removal so that the
private_free callback can safely remove the list entry.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 sound/core/jack.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/core/jack.c b/sound/core/jack.c
index af2dc2cfbe19..92febcb42a8d 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -555,8 +555,14 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 	}
 
 	jack = kzalloc_obj(struct snd_jack);
-	if (jack == NULL)
+	if (!jack) {
+		if (jack_kctl) {
+			INIT_LIST_HEAD(&jack_kctl->list);
+			snd_ctl_remove(card, jack_kctl->kctl);
+		}
+
 		return -ENOMEM;
+	}
 
 	jack->id = kstrdup(id, GFP_KERNEL);
 	if (jack->id == NULL) {
-- 
2.55.0

Re: [PATCH] ALSA: jack: Fix control leak on jack allocation failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: jack: incomplete jack_kctl cleanup on error paths in
  snd_jack_new()
- [Low] ALSA: jack: missing Fixes trailer in commit message for
  snd_jack_new() fix
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] ALSA: jack: NULL pointer dereference in
  snd_jack_kctl_private_free() [1]
--

commit 6c7b3553bfb569b822fa9be95c5962addb3dfa31
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

> ALSA: jack: Fix control leak on jack allocation failure
>
> snd_jack_new() creates an ALSA jack control before allocating the
> jack structure.
>
> If the jack structure allocation fails, the function returns
> -ENOMEM without removing the previously created control.
>
> Remove the control using snd_ctl_remove() to release the control
> and its private data before returning -ENOMEM.
>
> Initialize the control list before removal so that the
> private_free callback can safely remove the list entry.
>
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
Should this include a Fixes tag for the commit that moved initial control
creation ahead of the jack allocation?

  Fixes: 4e3f0dc65883 ("ALSA: jack: extend snd_jack_new to support phantom jack")

> diff --git a/sound/core/jack.c b/sound/core/jack.c
> index af2dc2cfbe197..92febcb42a8df 100644
> --- a/sound/core/jack.c
> +++ b/sound/core/jack.c
> @@ -555,8 +555,14 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>  	}
>  
>  	jack = kzalloc_obj(struct snd_jack);
> -	if (jack == NULL)
> +	if (!jack) {
> +		if (jack_kctl) {
> +			INIT_LIST_HEAD(&jack_kctl->list);
> +			snd_ctl_remove(card, jack_kctl->kctl);
> +		}
> +
>  		return -ENOMEM;
> +	}

[Severity: Medium]
What happens to jack_kctl if input_allocate_device() or snd_device_new()
fails later in snd_jack_new() and jumps to fail_input?

Only the struct snd_jack allocation failure branch initializes
jack_kctl->list and removes jack_kctl->kctl.  A failure at fail_input still
leaves the control attached to card->controls with jack_kctl->list
zero-initialized, which leaks the control on error return and triggers a
NULL pointer dereference in snd_jack_kctl_private_free() -> list_del() when
the card is later freed.

Initializing jack_kctl->list in snd_jack_kctl_new() and routing all failure
paths in snd_jack_new() through shared cleanup labels that call
snd_ctl_remove(card, jack_kctl->kctl) would handle all error paths
uniformly.

>  
>  	jack->id = kstrdup(id, GFP_KERNEL);
>  	if (jack->id == NULL) {

[Severity: High]
This problem wasn't introduced by this patch, but does a kstrdup() failure
here [1] also leave jack_kctl attached to the card with an
uninitialized list head?

sound/core/jack.c:snd_jack_new() {
    ...
	jack->id = kstrdup(id, GFP_KERNEL);
	if (jack->id == NULL) {
		kfree(jack);
		return -ENOMEM;
	}
    ...
}

When initial_kctl is true and kstrdup() returns NULL, snd_jack_new() frees
jack and returns -ENOMEM without calling snd_ctl_remove(card,
jack_kctl->kctl) or reaching snd_jack_kctl_add().  During subsequent card
teardown, snd_ctl_free_one() invokes snd_jack_kctl_private_free(), which
calls list_del(&jack_kctl->list) on NULL list pointers and crashes.

[1] https://sashiko.dev/#/bug/linux-b6f08fef-7427-40d4-a8d0-ec2dd4aa9cfa

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115101949.2483-1-sreekuttan2156239@gmail.com?part=1

[PATCH v2] ALSA: jack: Fix jack control cleanup on error paths

[Sreeraj S Kurup]

snd_jack_new() creates an ALSA jack control before allocating
the jack structure.

If a later allocation or device registration fails, the
function returns without removing the previously created
control. This leaves the control attached to the sound card
and can cause invalid list operations during card teardown.

Initialize the control list in snd_jack_kctl_new() so that
the private_free callback can safely remove the list entry.

Route all failure paths in snd_jack_new() through common
cleanup labels to remove the control and free allocated
resources.

Fixes: 4e3f0dc65883 ("ALSA: jack: extend snd_jack_new to support phantom jack")
Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v2:
 - Initialize the jack control list to prevent invalid list operations during cleanup.
 - Handle cleanup of the jack control on all error paths in "snd_jack_new()".

 sound/core/jack.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sound/core/jack.c b/sound/core/jack.c
index af2dc2cfbe19..c35a92390527 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -487,6 +487,7 @@ static struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const cha
 
 	jack_kctl->kctl = kctl;
 	jack_kctl->mask_bits = mask;
+	INIT_LIST_HEAD(&jack_kctl->list);
 
 	kctl->private_data = jack_kctl;
 	kctl->private_free = snd_jack_kctl_private_free;
@@ -555,13 +556,15 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 	}
 
 	jack = kzalloc_obj(struct snd_jack);
-	if (jack == NULL)
-		return -ENOMEM;
+	if (!jack) {
+		err = -ENOMEM;
+		goto fail_kctl;
+	}
 
 	jack->id = kstrdup(id, GFP_KERNEL);
 	if (jack->id == NULL) {
-		kfree(jack);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto fail_jack;
 	}
 
 #ifdef CONFIG_SND_JACK_INPUT_DEV
@@ -607,8 +610,13 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 #ifdef CONFIG_SND_JACK_INPUT_DEV
 	input_free_device(jack->input_dev);
 #endif
+fail_jack:
 	kfree(jack->id);
 	kfree(jack);
+fail_kctl:
+	if (jack_kctl)
+		snd_ctl_remove(card, jack_kctl->kctl);
+
 	return err;
 }
 EXPORT_SYMBOL(snd_jack_new);
-- 
2.55.0