Re: [PATCH v3] ALSA: usx2y: Release the URBs when the DSP load fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: usx2y: NULL deref of in04_urb after usx2y_create_alsa_devices() fails
- [High] ALSA: usx2y: race between usx2y_free_urbs() and snd_usx2y_disconnect()
--

commit 45a7e14f54ac22210b87cf753196e744d24a4e6a
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

ALSA: usx2y: Release the URBs when the DSP load fails

This commit adds usx2y_free_urbs() to release the asynchronous sequence URBs
and the interrupt URB when usx2y_in04_init() or usx2y_create_alsa_devices()
fails in snd_usx2y_hwdep_dsp_load(). It also sets USX2Y_STAT_CHIP_HUP if
usx2y_create_alsa_devices() fails so that subsequent DSP load requests are
refused with -ENODEV.

> diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c
> index 7f153a2fcdd81..366167c62575d 100644
> --- a/sound/usb/usx2y/usX2Yhwdep.c
> +++ b/sound/usb/usx2y/usX2Yhwdep.c

[ ... ]

> @@ -201,11 +204,14 @@ static int snd_usx2y_hwdep_dsp_load(struct snd_hwdep *hw,
>  		err = usx2y_in04_init(priv);
>  		if (err) {
>  			dev_err(&dev->dev, "usx2y_in04_init error\n");
> +			usx2y_free_urbs(priv);
>  			return err;
>  		}
>  		err = usx2y_create_alsa_devices(hw->card);
>  		if (err) {
>  			dev_err(&dev->dev, "usx2y_create_alsa_devices error %i\n", err);
> +			usx2y_free_urbs(priv);
> +			priv->chip_status |= USX2Y_STAT_CHIP_HUP;
>  			return err;
>  		}

[Severity: High]
As reported by Sashiko on v2 [1], can freeing priv->in04_urb here cause a
null pointer dereference in usx2y_format_set() if snd_card_register() fails
after registering the PCM devices?

In sound/usb/usx2y/usX2Yhwdep.c:usx2y_create_alsa_devices(), the driver
creates the rawmidi, PCM, and hwdep PCM devices and then calls
snd_card_register(card):

	err = usx2y_create_usbmidi(card);
	if (err < 0) {
		...
		return err;
	}
	err = usx2y_audio_create(card);
	if (err < 0)
		return err;
	err = usx2y_hwdep_pcm_new(card);
	if (err < 0)
		return err;
	err = snd_card_register(card);
	if (err < 0)
		return err;

Because snd_device_new() orders card->devices by enum snd_device_type,
SNDRV_DEV_PCM precedes SNDRV_DEV_RAWMIDI and SNDRV_DEV_HWDEP. When
snd_card_register() runs snd_device_register_all(), it registers the PCM
devices before registering the rawmidi device, the second hwdep device, or the
procfs entries in snd_info_card_register(). If any of those later registration
steps fail, snd_card_register() returns an error without unregistering the
already-registered PCM device nodes.

On that failure path, snd_usx2y_hwdep_dsp_load() now calls
usx2y_free_urbs(priv), which frees priv->in04_urb and sets it to NULL, and
then sets USX2Y_STAT_CHIP_HUP. However, neither snd_usx2y_pcm_open() nor
snd_usx2y_pcm_prepare() in sound/usb/usx2y/usbusx2yaudio.c checks
USX2Y_STAT_CHIP_HUP or USX2Y_STAT_CHIP_INIT.

If user space opens the registered PCM device and issues
SNDRV_PCM_IOCTL_PREPARE, snd_usx2y_pcm_prepare() calls usx2y_format_set(),
which unconditionally dereferences usx2y->in04_urb:

	usx2y->in04_urb->dev = usx2y->dev;
	err = usb_submit_urb(usx2y->in04_urb, GFP_KERNEL);

> diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c
> index 108f9bddf9aa3..8a551abe47dc6 100644
> --- a/sound/usb/usx2y/usbusx2y.c
> +++ b/sound/usb/usx2y/usbusx2y.c
> @@ -349,6 +349,22 @@ static void usx2y_unlinkseq(struct snd_usx2y_async_seq *s)
>  	s->buffer = NULL;
>  }
>  
> +/*
> + * Release the URBs set up by usx2y_async_seq04_init() and usx2y_in04_init().
> + * Used when a later step of the DSP loading fails.
> + */
> +void usx2y_free_urbs(struct usx2ydev *usx2y)
> +{
> +	if (usx2y->in04_urb) {
> +		usb_kill_urb(usx2y->in04_urb);
> +		usb_free_urb(usx2y->in04_urb);
> +		usx2y->in04_urb = NULL;
> +		kfree(usx2y->in04_buf);
> +		usx2y->in04_buf = NULL;
> +	}
> +	usx2y_unlinkseq(&usx2y->as04);
> +}

[Severity: High]
As reported by Sashiko on v2 [2], does usx2y_free_urbs() race with
snd_usx2y_disconnect() when the USB device disconnects during DSP loading?

When dsp->index == 1 in snd_usx2y_hwdep_dsp_load(), if the USB device is
disconnected while usx2y_in04_init() or usx2y_create_alsa_devices() is
running (for example during the wait_event_timeout() in usx2y_rate_set()),
that initialization fails and calls usx2y_free_urbs(priv) under hw->dsp_mutex.

At the same time, the USB disconnect handler runs
sound/usb/usx2y/usbusx2y.c:snd_usx2y_disconnect() without holding
hw->dsp_mutex:

	usx2y->chip_status = USX2Y_STAT_CHIP_HUP;
	usx2y_unlinkseq(&usx2y->as04);
	usb_kill_urb(usx2y->in04_urb);

Previously, usx2y->in04_urb was only freed later in
snd_usx2y_card_private_free() after snd_usx2y_disconnect() had finished. With
usx2y_free_urbs() freeing usx2y->in04_urb before clearing the pointer, can
snd_usx2y_disconnect() pass a freed usx2y->in04_urb to usb_kill_urb()?

Additionally, both usx2y_free_urbs() and snd_usx2y_disconnect() call
usx2y_unlinkseq(&usx2y->as04) concurrently:

static void usx2y_unlinkseq(struct snd_usx2y_async_seq *s)
{
	int	i;

	for (i = 0; i < URBS_ASYNC_SEQ; ++i) {
		if (!s->urb[i])
			continue;
		usb_kill_urb(s->urb[i]);
		usb_free_urb(s->urb[i]);
		s->urb[i] = NULL;
	}
	kfree(s->buffer);
	s->buffer = NULL;
}

Since usb_kill_urb() sleeps and s->urb[i] and s->buffer are only set to NULL
after usb_free_urb() and kfree(), can concurrent execution of
usx2y_unlinkseq() cause a use-after-free and double-free on s->urb[i] and
s->buffer?

[1] https://lore.kernel.org/r/20261031171612.8148-1-25031212351@stu.xidian.edu.cn
[2] https://lore.kernel.org/r/20261031171612.8148-1-25031212351@stu.xidian.edu.cn

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031185619.18601-1-25031212351@stu.xidian.edu.cn?part=1