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

[sashiko-bot@kernel.org]

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