[PATCH 0/2] ALSA: fix a user-triggerable WARN on repeated DSP load

[PATCH 0/2] ALSA: fix a user-triggerable WARN on repeated DSP load

[YuanZhe Liu]

snd_hwdep_dsp_load() checks hw->dsp_loaded and then calls the driver's
dsp_load callback without any serialization.  A hwdep file descriptor can
be shared among threads, so two SNDRV_HWDEP_IOCTL_DSP_LOAD requests for
the same index can both pass the check and run the callback concurrently.

The snd-usb-usx2y driver then enters usx2y_async_seq04_init() twice, which
hits WARN_ON(usx2y->as04.buffer) and may also allocate the US-428
sequence URBs twice.

Patch 1 adds a per-device mutex around the check, the callback and the
bookkeeping.  Patch 2 removes the WARN_ON() assertions from the usx2y
initialization helpers: a repeated request after a partially failed load
is a valid sequence from user space, so the driver should return -EBUSY
instead of warning.

YuanZhe Liu (2):
  ALSA: hwdep: Serialize DSP load requests
  ALSA: usx2y: Avoid WARN on a repeated DSP load

 include/sound/hwdep.h      | 1 +
 sound/core/hwdep.c         | 2 ++
 sound/usb/usx2y/usbusx2y.c | 4 ++--
 3 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] ALSA: hwdep: Serialize DSP load requests

[YuanZhe Liu]

The check of the loaded DSP indices and the call of the driver's
dsp_load callback in snd_hwdep_dsp_load() are not serialized.  A hwdep
file descriptor can be shared among threads, or inherited across fork,
so two SNDRV_HWDEP_IOCTL_DSP_LOAD requests for the same index can both
observe the index as unloaded and enter the driver's callback
concurrently, even though the device itself is opened exclusively.

This breaks the assumption behind hw->dsp_loaded that a firmware index
is loaded only once.  For example, snd-usb-usx2y hits
WARN_ON(usx2y->as04.buffer) in usx2y_async_seq04_init() and may
allocate the US-428 sequence URBs twice.

Serialize the check, the callback invocation and the bookkeeping with a
per-hwdep mutex, so that the second request returns -EBUSY.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
 include/sound/hwdep.h | 1 +
 sound/core/hwdep.c    | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/sound/hwdep.h b/include/sound/hwdep.h
index b0da633184cd..7357727f0660 100644
--- a/include/sound/hwdep.h
+++ b/include/sound/hwdep.h
@@ -57,6 +57,7 @@ struct snd_hwdep {
 
 	struct mutex open_mutex;
 	int used;			/* reference counter */
+	struct mutex dsp_mutex;		/* serializes DSP load requests */
 	unsigned int dsp_loaded;	/* bit fields of loaded dsp indices */
 	unsigned int exclusive:1;	/* exclusive access mode */
 };
diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
index c6de5345e946..fb9034897c56 100644
--- a/sound/core/hwdep.c
+++ b/sound/core/hwdep.c
@@ -212,6 +212,7 @@ static int snd_hwdep_dsp_load(struct snd_hwdep *hw,
 		return -ENXIO;
 	if (info->index >= 32)
 		return -EINVAL;
+	guard(mutex)(&hw->dsp_mutex);
 	/* check whether the dsp was already loaded */
 	if (hw->dsp_loaded & (1u << info->index))
 		return -EBUSY;
@@ -380,6 +381,7 @@ int snd_hwdep_new(struct snd_card *card, char *id, int device,
 
 	init_waitqueue_head(&hwdep->open_wait);
 	mutex_init(&hwdep->open_mutex);
+	mutex_init(&hwdep->dsp_mutex);
 	hwdep->card = card;
 	hwdep->device = device;
 	if (id)
-- 
2.34.1

Re: [PATCH 1/2] ALSA: hwdep: Serialize DSP load requests

[Takashi Iwai]

On Thu, 29 Oct 2026 09:48:09 +0100,
YuanZhe Liu wrote:
> 
> The check of the loaded DSP indices and the call of the driver's
> dsp_load callback in snd_hwdep_dsp_load() are not serialized.  A hwdep
> file descriptor can be shared among threads, or inherited across fork,
> so two SNDRV_HWDEP_IOCTL_DSP_LOAD requests for the same index can both
> observe the index as unloaded and enter the driver's callback
> concurrently, even though the device itself is opened exclusively.
> 
> This breaks the assumption behind hw->dsp_loaded that a firmware index
> is loaded only once.  For example, snd-usb-usx2y hits
> WARN_ON(usx2y->as04.buffer) in usx2y_async_seq04_init() and may
> allocate the US-428 sequence URBs twice.
> 
> Serialize the check, the callback invocation and the bookkeeping with a
> per-hwdep mutex, so that the second request returns -EBUSY.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Co-developed-by: Bin Yu <byu@xidian.edu.cn>
> Signed-off-by: Bin Yu <byu@xidian.edu.cn>
> Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

Applied this one.  Thanks.


Takashi

[PATCH 2/2] ALSA: usx2y: Avoid WARN on a repeated DSP load

[YuanZhe Liu]

snd_usx2y_hwdep_dsp_load() sets up the asynchronous sequence URBs and
the interrupt URB when the FPGA firmware image (index 1) is loaded.  If
a later step of the same request fails, for example the creation of the
ALSA devices, a following DSP load request from user space enters the
initialization functions for a second time.

usx2y_async_seq04_init() and usx2y_in04_init() assert with WARN_ON()
that they are called only once, so such a request triggers a kernel
warning before returning -EBUSY, although nothing is corrupted: the
functions bail out without allocating again and the leak that the check
was added for is still prevented.

Return -EBUSY without the warning.

Fixes: 02d382af1c4e ("ALSA: usx2y: Fix potential memory leaks")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
 sound/usb/usx2y/usbusx2y.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c
index 108f9bddf9aa..14498ce84440 100644
--- a/sound/usb/usx2y/usbusx2y.c
+++ b/sound/usb/usx2y/usbusx2y.c
@@ -268,7 +268,7 @@ int usx2y_async_seq04_init(struct usx2ydev *usx2y)
 {
 	int	err = 0, i;
 
-	if (WARN_ON(usx2y->as04.buffer))
+	if (usx2y->as04.buffer)
 		return -EBUSY;
 
 	usx2y->as04.buffer = kmalloc_array(URBS_ASYNC_SEQ,
@@ -300,7 +300,7 @@ int usx2y_in04_init(struct usx2ydev *usx2y)
 {
 	int err;
 
-	if (WARN_ON(usx2y->in04_urb))
+	if (usx2y->in04_urb)
 		return -EBUSY;
 
 	usx2y->in04_urb = usb_alloc_urb(0, GFP_KERNEL);
-- 
2.34.1

Re: [PATCH 2/2] ALSA: usx2y: Avoid WARN on a repeated DSP load

[Takashi Iwai]

On Thu, 29 Oct 2026 09:48:10 +0100,
YuanZhe Liu wrote:
> 
> snd_usx2y_hwdep_dsp_load() sets up the asynchronous sequence URBs and
> the interrupt URB when the FPGA firmware image (index 1) is loaded.  If
> a later step of the same request fails, for example the creation of the
> ALSA devices, a following DSP load request from user space enters the
> initialization functions for a second time.
> 
> usx2y_async_seq04_init() and usx2y_in04_init() assert with WARN_ON()
> that they are called only once, so such a request triggers a kernel
> warning before returning -EBUSY, although nothing is corrupted: the
> functions bail out without allocating again and the leak that the check
> was added for is still prevented.
> 
> Return -EBUSY without the warning.
> 
> Fixes: 02d382af1c4e ("ALSA: usx2y: Fix potential memory leaks")
> Cc: stable@vger.kernel.org
> Co-developed-by: Bin Yu <byu@xidian.edu.cn>
> Signed-off-by: Bin Yu <byu@xidian.edu.cn>
> Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

IMO, it's better to fix the error path in snd_usx2y_hwdep_dsp_load()
instead.  The WARN_ON() was meant that it shouldn't happen, and if it
happens, it's something to be fixed, instead.


thanks,

Takashi

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

[YuanZhe Liu]

snd_usx2y_hwdep_dsp_load() sets up the asynchronous sequence URBs and
the interrupt URB when the FPGA firmware image with index 1 is loaded,
and then creates the ALSA devices.  If usx2y_in04_init() or
usx2y_create_alsa_devices() fails, the URBs that were already set up
are left in place.

The core keeps the index unloaded when the callback fails, so user
space may issue the same request again.  The second attempt then enters
usx2y_async_seq04_init() or usx2y_in04_init() with the URBs still
allocated and trips WARN_ON(usx2y->as04.buffer) or
WARN_ON(usx2y->in04_urb).

Release the URBs on these error paths instead.  A retried request then
starts from a clean state and the assertions keep their meaning.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
v2: release the URBs on the failing paths of snd_usx2y_hwdep_dsp_load()
    instead of removing the WARN_ON()s, as suggested by Takashi Iwai.

 sound/usb/usx2y/usX2Yhwdep.c |  2 ++
 sound/usb/usx2y/usbusx2y.c   | 16 ++++++++++++++++
 sound/usb/usx2y/usbusx2y.h   |  1 +
 3 files changed, 19 insertions(+)

diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c
index 7f153a2fcdd8..13995e81d2ae 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;
 		}
 		priv->chip_status |= USX2Y_STAT_CHIP_INIT;
diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c
index 108f9bddf9aa..8a551abe47dc 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);
+}
+
 static const struct usb_device_id snd_usx2y_usb_id_table[] = {
 	{
 		.match_flags =	USB_DEVICE_ID_MATCH_DEVICE,
diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
index 7b6deed17d4b..e5e26ac8dc04 100644
--- a/sound/usb/usx2y/usbusx2y.h
+++ b/sound/usb/usx2y/usbusx2y.h
@@ -110,6 +110,7 @@ int usx2y_audio_create(struct snd_card *card);
 
 int usx2y_async_seq04_init(struct usx2ydev *usx2y);
 int usx2y_in04_init(struct usx2ydev *usx2y);
+void usx2y_free_urbs(struct usx2ydev *usx2y);
 
 #define NAME_ALLCAPS "US-X2Y"
 
-- 
2.34.1

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

[sashiko-bot]

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

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

[YuanZhe Liu]

snd_usx2y_hwdep_dsp_load() sets up the asynchronous sequence URBs and
the interrupt URB when the FPGA firmware image with index 1 is loaded,
and then creates the ALSA devices.  If usx2y_in04_init() or
usx2y_create_alsa_devices() fails, the URBs that were already set up
are left in place.

The core keeps the index unloaded when the callback fails, so user
space may issue the same request again.  The second attempt then enters
usx2y_async_seq04_init() or usx2y_in04_init() with the URBs still
allocated and trips WARN_ON(usx2y->as04.buffer) or
WARN_ON(usx2y->in04_urb).

Release the URBs on these error paths instead, so that a retried
request starts from a clean state and the assertions keep their
meaning.

Releasing the URBs also lets a retried request reach
usx2y_create_alsa_devices() again, on a card that keeps the MIDI and
PCM devices created by the failed attempt.  For the US-428 the audio
streams would then index usx2y->subs[] past its end, so do not
initialise a partially created card a second time: mark the chip as
hung when the ALSA devices could not be created, and refuse further DSP
load requests.  A failure of usx2y_in04_init() leaves the card
untouched and remains retryable.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
v3: do not initialise a partially created card a second time.  A failed
    usx2y_create_alsa_devices() now marks the chip as hung, so a retried
    DSP load request returns -ENODEV instead of running the ALSA device
    creation again.

 sound/usb/usx2y/usX2Yhwdep.c |  6 ++++++
 sound/usb/usx2y/usbusx2y.c   | 16 ++++++++++++++++
 sound/usb/usx2y/usbusx2y.h   |  1 +
 3 files changed, 23 insertions(+)

diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c
index 7f153a2fcdd8..366167c62575 100644
--- a/sound/usb/usx2y/usX2Yhwdep.c
+++ b/sound/usb/usx2y/usX2Yhwdep.c
@@ -179,6 +179,9 @@ static int snd_usx2y_hwdep_dsp_load(struct snd_hwdep *hw,
 	int lret, err;
 	char *buf;
 
+	if (priv->chip_status & USX2Y_STAT_CHIP_HUP)
+		return -ENODEV;
+
 	buf = memdup_user(dsp->image, dsp->length);
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
@@ -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;
 		}
 		priv->chip_status |= USX2Y_STAT_CHIP_INIT;
diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c
index 108f9bddf9aa..8a551abe47dc 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);
+}
+
 static const struct usb_device_id snd_usx2y_usb_id_table[] = {
 	{
 		.match_flags =	USB_DEVICE_ID_MATCH_DEVICE,
diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
index 7b6deed17d4b..e5e26ac8dc04 100644
--- a/sound/usb/usx2y/usbusx2y.h
+++ b/sound/usb/usx2y/usbusx2y.h
@@ -110,6 +110,7 @@ int usx2y_audio_create(struct snd_card *card);
 
 int usx2y_async_seq04_init(struct usx2ydev *usx2y);
 int usx2y_in04_init(struct usx2ydev *usx2y);
+void usx2y_free_urbs(struct usx2ydev *usx2y);
 
 #define NAME_ALLCAPS "US-X2Y"
 
-- 
2.34.1

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

[sashiko-bot]

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

[PATCH v4] ALSA: usx2y: Refuse a retried DSP load after a failure

[YuanZhe Liu]

snd_usx2y_hwdep_dsp_load() sets up the asynchronous sequence URBs and
the interrupt URB when the FPGA firmware image with index 1 is loaded,
and then creates the ALSA devices.  If usx2y_in04_init() or
usx2y_create_alsa_devices() fails, the URBs that were already set up
are left in place.

The core keeps the index unloaded when the callback fails, so user
space may issue the same request again.  The second attempt then enters
usx2y_async_seq04_init() or usx2y_in04_init() with the URBs still
allocated and trips WARN_ON(usx2y->as04.buffer) or
WARN_ON(usx2y->in04_urb).

The URBs cannot be released at this point: the card may already have
registered PCM devices that user space can still open, and the
disconnect handler accesses the URBs without holding the DSP lock.
Mark the chip as hung instead, so that a retried request is refused
with -ENODEV and the assertions keep their meaning.  The URBs are
released by snd_usx2y_card_private_free() when the card is freed.

A failing usx2y_async_seq04_init() still releases its own URBs, so that
path remains retryable.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
v4: drop the release of the URBs on the error paths.  The card may already
    have registered PCM devices, and the disconnect handler accesses the
    URBs without the DSP lock; mark the chip as hung instead and let
    snd_usx2y_card_private_free() release the URBs.

 sound/usb/usx2y/usX2Yhwdep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c
index 7f153a2fcdd8..3800c5b5887c 100644
--- a/sound/usb/usx2y/usX2Yhwdep.c
+++ b/sound/usb/usx2y/usX2Yhwdep.c
@@ -179,6 +179,9 @@ static int snd_usx2y_hwdep_dsp_load(struct snd_hwdep *hw,
 	int lret, err;
 	char *buf;
 
+	if (priv->chip_status & USX2Y_STAT_CHIP_HUP)
+		return -ENODEV;
+
 	buf = memdup_user(dsp->image, dsp->length);
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
@@ -201,11 +204,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");
+			priv->chip_status |= USX2Y_STAT_CHIP_HUP;
 			return err;
 		}
 		err = usx2y_create_alsa_devices(hw->card);
 		if (err) {
 			dev_err(&dev->dev, "usx2y_create_alsa_devices error %i\n", err);
+			priv->chip_status |= USX2Y_STAT_CHIP_HUP;
 			return err;
 		}
 		priv->chip_status |= USX2Y_STAT_CHIP_INIT;
-- 
2.34.1

Re: [PATCH 0/2] ALSA: fix a user-triggerable WARN on repeated DSP load

[YuanZhe Liu]

Hi Takashi,

Could you please review this patch series?

  [PATCH 0/2] ALSA: fix a user-triggerable WARN on repeated DSP load

It serializes concurrent hwdep DSP load requests and returns EBUSY
instead of triggering a WARN on repeated usx2y initialization.

The series is unchanged. I am copying this address for visibility.

Original submission:
https://lore.kernel.org/r/20261029084810.21910-1-25031212351@stu.xidian.edu.cn

Thanks,
YuanZhe