Re: [PATCH v2] 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:
- [Critical] ALSA: usx2y: missing device rollback in snd_usx2y_hwdep_dsp_load()
- [High] ALSA: usx2y: unsynchronized URB free in usx2y_free_urbs()
--

commit 8f4b5a194c16ab573b5c1441a177381d9389e8b1
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

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

This commit adds usx2y_free_urbs() and calls it on the error paths of
snd_usx2y_hwdep_dsp_load() when usx2y_in04_init() or
usx2y_create_alsa_devices() fails, so a retried DSP load request does not
trip WARN_ON() on already allocated URBs.

> diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c
> index 7f153a2fcdd81..13995e81d2ae9 100644
> --- a/sound/usb/usx2y/usX2Yhwdep.c
> +++ b/sound/usb/usx2y/usX2Yhwdep.c
> @@ -201,11 +201,13 @@ 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);
>  			return err;
>  		}

[Severity: Critical]
When usx2y_create_alsa_devices() fails halfway through (for example, if
usx2y_rate_set() times out in usx2y_audio_create(), or if
usx2y_hwdep_pcm_new() or snd_card_register() fails), the MIDI and PCM
devices created earlier in usx2y_create_alsa_devices() are not rolled back:

sound/usb/usx2y/usX2Yhwdep.c:usx2y_create_alsa_devices() {
	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;
}

Previously, priv->as04.buffer remained non-NULL, which blocked any retry of
SNDRV_HWDEP_IOCTL_DSP_LOAD at usx2y_async_seq04_init() with -EBUSY. With
usx2y_free_urbs(priv) clearing priv->as04.buffer and priv->in04_urb here, a
retried DSP load for index 1 will now re-enter usx2y_create_alsa_devices()
on the uncleaned card state.

In usx2y_audio_create() -> usx2y_audio_stream_new(), each call indexes into
usx2y(card)->subs[4] using 2 * usx2y(card)->pcm_devs and increments
pcm_devs:

sound/usb/usx2y/usbusx2yaudio.c:usx2y_audio_stream_new() {
	struct snd_usx2y_substream **usx2y_substream =
		usx2y(card)->subs + 2 * usx2y(card)->pcm_devs;

	for (i = playback_endpoint ? SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE;
	     i <= SNDRV_PCM_STREAM_CAPTURE; ++i) {
		usx2y_substream[i] = kzalloc_obj(struct snd_usx2y_substream);
		...
	}
	...
	usx2y(card)->pcm_devs++;
}

Since struct usx2ydev only has 4 elements in subs[4], won't retrying
SNDRV_HWDEP_IOCTL_DSP_LOAD once pcm_devs reaches 2 (on the first retry for
US-428, or the second retry for US-122/US-224) write out of bounds past
usx2y->subs[4] into prepare_subs, prepare_wait_queue, midi_list, and
pcm_devs?

In addition, if snd_card_register() fails after snd_device_register_all()
has already registered the SNDRV_DEV_PCM devices (which also happens on
retry when the duplicate rawmidi device 0 fails registration with -EBUSY),
those PCM device nodes remain exposed to userspace while usx2y_free_urbs()
sets usx2y->in04_urb to NULL. If userspace opens the PCM device and invokes
SNDRV_PCM_IOCTL_PREPARE, won't snd_usx2y_pcm_prepare() ->
usx2y_format_set() dereference a NULL usx2y->in04_urb at:

	usb_kill_urb(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]
Can usx2y_free_urbs() race with snd_usx2y_disconnect() or concurrent
SNDRV_HWDEP_IOCTL_DSP_LOAD calls?

snd_hwdep_ioctl() invokes snd_hwdep_dsp_load() ->
snd_usx2y_hwdep_dsp_load() without holding a driver lock, and
snd_usx2y_hwdep_dsp_load() sleeps in msleep(250), usx2y_in04_init(), and
usx2y_create_alsa_devices(). If the USB device disconnects during DSP
loading, the initialization step fails and calls usx2y_free_urbs(priv) at
the same time as the USB hub thread runs snd_usx2y_disconnect():

sound/usb/usx2y/usbusx2y.c:snd_usx2y_disconnect() {
	...
	usx2y_unlinkseq(&usx2y->as04);
	usb_kill_urb(usx2y->in04_urb);
	snd_card_disconnect(card);
	...
}

Since neither path is serialized, couldn't snd_usx2y_disconnect() call
usb_kill_urb(usx2y->in04_urb) while or after usx2y_free_urbs() calls
usb_free_urb(usx2y->in04_urb), and couldn't both paths enter
usx2y_unlinkseq(&usx2y->as04) concurrently (or two threads sharing the
hwdep fd race in SNDRV_HWDEP_IOCTL_DSP_LOAD), resulting in a use-after-free
or double-free of in04_urb, as04.urb[i], and as04.buffer?

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