[PATCH] usb: gadget: f_uac1_legacy: remove broken string configfs attributes

[PATCH] usb: gadget: f_uac1_legacy: remove broken string configfs attributes

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

The UAC1_STR_ATTRIBUTE macro defines configfs show/store handlers for
the fn_play, fn_cap, and fn_cntl string options. The store function
contains an inverted null check on the kstrndup() return value.

This means every write attempt returns -ENOMEM on success and
dereferences a NULL pointer on allocation failure. The attributes
have been broken and unused for many years.

Remove the UAC1_STR_ATTRIBUTE macro and the three attributes it
generated. The internal defaults (FILE_PCM_PLAYBACK, FILE_PCM_CAPTURE,
FILE_CONTROL) set in f_audio_alloc_inst() are unaffected.

Fixes: 0854611a19ae ("usb: gadget: f_uac1: add configfs support")
Link: https://lore.kernel.org/linux-usb/20260625113154.1954813-1-xu.yang_2@oss.nxp.com/
Assisted-by: Claude:claude-sonnet-4.6
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 .../testing/configfs-usb-gadget-uac1_legacy   |  3 --
 drivers/usb/gadget/function/f_uac1_legacy.c   | 50 -------------------
 2 files changed, 53 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy b/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
index b2eaefd9bc49..6a681d219f43 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
@@ -5,8 +5,5 @@ Description:
 		The attributes:
 
 		audio_buf_size - audio buffer size
-		fn_cap - capture pcm device file name
-		fn_cntl - control device file name
-		fn_play - playback pcm device file name
 		req_buf_size - ISO OUT endpoint request buffer size
 		req_count - ISO OUT endpoint request count
diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
index 5d201a2e30e7..b15f5e074f85 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -888,60 +888,10 @@ UAC1_INT_ATTRIBUTE(req_buf_size);
 UAC1_INT_ATTRIBUTE(req_count);
 UAC1_INT_ATTRIBUTE(audio_buf_size);
 
-#define UAC1_STR_ATTRIBUTE(name)					\
-static ssize_t f_uac1_opts_##name##_show(struct config_item *item,	\
-					 char *page)			\
-{									\
-	struct f_uac1_legacy_opts *opts = to_f_uac1_opts(item);		\
-	int result;							\
-									\
-	mutex_lock(&opts->lock);					\
-	result = sprintf(page, "%s\n", opts->name);			\
-	mutex_unlock(&opts->lock);					\
-									\
-	return result;							\
-}									\
-									\
-static ssize_t f_uac1_opts_##name##_store(struct config_item *item,	\
-					  const char *page, size_t len)	\
-{									\
-	struct f_uac1_legacy_opts *opts = to_f_uac1_opts(item);		\
-	int ret = -EBUSY;						\
-	char *tmp;							\
-									\
-	mutex_lock(&opts->lock);					\
-	if (opts->refcnt)						\
-		goto end;						\
-									\
-	tmp = kstrndup(page, len, GFP_KERNEL);				\
-	if (tmp) {							\
-		ret = -ENOMEM;						\
-		goto end;						\
-	}								\
-	if (opts->name##_alloc)						\
-		kfree(opts->name);					\
-	opts->name##_alloc = true;					\
-	opts->name = tmp;						\
-	ret = len;							\
-									\
-end:									\
-	mutex_unlock(&opts->lock);					\
-	return ret;							\
-}									\
-									\
-CONFIGFS_ATTR(f_uac1_opts_, name)
-
-UAC1_STR_ATTRIBUTE(fn_play);
-UAC1_STR_ATTRIBUTE(fn_cap);
-UAC1_STR_ATTRIBUTE(fn_cntl);
-
 static struct configfs_attribute *f_uac1_attrs[] = {
 	&f_uac1_opts_attr_req_buf_size,
 	&f_uac1_opts_attr_req_count,
 	&f_uac1_opts_attr_audio_buf_size,
-	&f_uac1_opts_attr_fn_play,
-	&f_uac1_opts_attr_fn_cap,
-	&f_uac1_opts_attr_fn_cntl,
 	NULL,
 };
 
-- 
2.34.1

Re: [PATCH] usb: gadget: f_uac1_legacy: remove broken string configfs attributes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:

New issues:
- [Low] The commit removes dynamic string configfs attributes but leaves deallocation logic and state tracking intact, resulting in dead code.
- [Low] The patch leaves stale references to the removed configfs attributes in user documentation.

Pre-existing issues:
- [Critical] Failure to allocate a new audio buffer in the completion handler leaves a dangling pointer in audio->copy_buf, leading to List Corruption and Use-After-Free.
- [Critical] Handling of alt == 0 fails to stop the endpoint or clear audio->copy_buf, leading to Use-After-Free and Double Free.
- [Critical] Failed buffer allocation assigns an error pointer to audio->copy_buf, which is later dereferenced as a valid pointer.
- [High] Missing cancel_work_sync() during gadget unbind leads to Use-After-Free in the playback workqueue.
- [High] Allocated usb_request objects and their buffers are never freed, causing a severe memory leak.
- [High] Unprotected list modification in f_audio_set_alt.
- [High] Premature hardirq enable in USB endpoint completion handler.
- [High] Missing queue drain loop in f_audio_playback_work causes unbounded list growth.
- [High] Unvalidated audio_buf_size and req_buf_size configfs parameters allow a heap buffer overflow.
--

--- Patch [1]: [PATCH] usb: gadget: f_uac1_legacy: remove broken string configfs attributes ---
commit 133e7d78a83d2788e14ca436593367567a36c8b9
Author: Xu Yang <xu.yang_2@nxp.com>

usb: gadget: f_uac1_legacy: remove broken string configfs attributes

The UAC1_STR_ATTRIBUTE macro defines configfs show/store handlers for
the fn_play, fn_cap, and fn_cntl string options. The attributes have been
broken and unused for many years, so remove them.

Link: https://lore.kernel.org/linux-usb/20260625113154.1954813-1-xu.yang_2@oss.nxp.com/

> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy b/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
> index b2eaefd9bc498..6a681d219f439 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
> @@ -5,8 +5,5 @@ Description:
>  		The attributes:
>  
>  		audio_buf_size - audio buffer size
> -		fn_cap - capture pcm device file name
> -		fn_cntl - control device file name
> -		fn_play - playback pcm device file name

[Severity: Low]
Does this patch leave stale references to the removed configfs attributes
in user documentation?

Documentation/usb/gadget-testing.rst:
    ...
	fn_cap		capture pcm device file name
	fn_cntl		control device file name
	fn_play		playback pcm device file name
    ...

>  		req_buf_size - ISO OUT endpoint request buffer size
>  		req_count - ISO OUT endpoint request count
> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 5d201a2e30e7f..b15f5e074f856 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -888,60 +888,10 @@ UAC1_INT_ATTRIBUTE(req_buf_size);
>  UAC1_INT_ATTRIBUTE(req_count);
>  UAC1_INT_ATTRIBUTE(audio_buf_size);
>  
> -#define UAC1_STR_ATTRIBUTE(name)					\

[ ... ]

> -UAC1_STR_ATTRIBUTE(fn_play);
> -UAC1_STR_ATTRIBUTE(fn_cap);
> -UAC1_STR_ATTRIBUTE(fn_cntl);

[Severity: Low]
With these attributes removed, does this leave dead code blocks in
f_audio_free_inst() that check the now-always-false allocation flags?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_free_inst() {
    ...
	opts = container_of(f, struct f_uac1_legacy_opts, func_inst);
	if (opts->fn_play_alloc)
		kfree(opts->fn_play);
	if (opts->fn_cap_alloc)
		kfree(opts->fn_cap);
	if (opts->fn_cntl_alloc)
		kfree(opts->fn_cntl);
    ...
}

And does it also leave permanently unused bitfields in
struct f_uac1_legacy_opts?

drivers/usb/gadget/function/u_uac1_legacy.h:struct f_uac1_legacy_opts {
    ...
	unsigned			fn_play_alloc:1;
	unsigned			fn_cap_alloc:1;
	unsigned			fn_cntl_alloc:1;
    ...
}

>  static struct configfs_attribute *f_uac1_attrs[] = {
>  	&f_uac1_opts_attr_req_buf_size,

[ ... ]

[Severity: High]
This is a pre-existing issue, but does f_audio_free() lead to a use-after-free
if the playback workqueue executes on the freed memory?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_free() {
    ...
	kfree(audio);
	mutex_lock(&opts->lock);
    ...
}

It appears the structure is freed without cancelling the playback_work.

[Severity: High]
This is a pre-existing issue, but are the allocated usb_request objects and
their buffers never freed?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
				req = usb_ep_alloc_request(out_ep, GFP_ATOMIC);
				if (req) {
					req->buf = kzalloc(req_buf_size,
							GFP_ATOMIC);
    ...
}

Every set_alt(1) from the host allocates a batch of requests and buffers, but
usb_ep_free_request() doesn't seem to be called anywhere in the driver.

[Severity: Critical]
This is a pre-existing issue, but does a failure to allocate a new audio buffer
leave a dangling pointer in audio->copy_buf?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_out_ep_complete() {
    ...
		copy_buf = f_audio_buffer_alloc(audio_buf_size);
		if (IS_ERR(copy_buf))
			return -ENOMEM;
    ...
}

Returning early here leaves audio->copy_buf pointing to the buffer that was
just queued for playback or freed, potentially leading to list corruption and
a use-after-free.

[Severity: Critical]
This is a pre-existing issue, but does the handling of alt == 0 fail to stop
the endpoint or clear audio->copy_buf?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
		} else {
			struct f_audio_buf *copy_buf = audio->copy_buf;
			if (copy_buf) {
				list_add_tail(&copy_buf->list,
						&audio->play_queue);
				schedule_work(&audio->playback_work);
			}
		}
    ...
}

Because usb_ep_disable() is not called, can the endpoint continue receiving
data into copy_buf which was just queued to play_queue?

[Severity: Critical]
This is a pre-existing issue, but can a failed buffer allocation assign an
error pointer to audio->copy_buf, which is later dereferenced?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
			audio->copy_buf = f_audio_buffer_alloc(audio_buf_size);
			if (IS_ERR(audio->copy_buf))
				return -ENOMEM;
    ...
}

If a subsequent set_alt(0) is received:

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
			if (copy_buf) {
				list_add_tail(&copy_buf->list,
						&audio->play_queue);
    ...
}

ERR_PTR(-ENOMEM) evaluates as true, which would cause list_add_tail() to
write to an invalid pointer offset.

[Severity: High]
This is a pre-existing issue, but is this list modification unprotected?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
				list_add_tail(&copy_buf->list,
						&audio->play_queue);
    ...
}

It accesses and modifies audio->play_queue without holding audio->lock, which
could race concurrently with f_audio_playback_work() and completion handlers.

[Severity: High]
This is a pre-existing issue, but does this prematurely enable hardirqs
inside a USB completion callback?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_out_ep_complete() {
    ...
		spin_unlock_irq(&audio->lock);
    ...
}

Calling spin_unlock_irq() unconditionally unmasks hardware interrupts,
which violates IRQ masking state if the callback natively runs in interrupt
context.

[Severity: High]
This is a pre-existing issue, but does the lack of a queue drain loop in
f_audio_playback_work() cause unbounded list growth?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_playback_work() {
    ...
	play_buf = list_first_entry(&audio->play_queue,
			struct f_audio_buf, list);
	list_del(&play_buf->list);
	spin_unlock_irq(&audio->lock);

	u_audio_playback(&audio->card, play_buf->buf, play_buf->actual);
	f_audio_buffer_free(play_buf);
    ...
}

Since schedule_work() coalesces, if multiple audio packets complete before
the scheduled work executes, this will pop only one item and permanently
delay the processing of accumulated concurrent items.

[Severity: High]
This is a pre-existing issue, but can unvalidated audio_buf_size and
req_buf_size parameters allow a heap buffer overflow?

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_out_ep_complete() {
    ...
	memcpy(copy_buf->buf + copy_buf->actual, req->buf, req->actual);
    ...
}

If a user configures the gadget with req_buf_size larger than audio_buf_size,
the length of data copied (req->actual) could overflow the newly allocated
copy_buf->buf.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713050246.3739137-1-xu.yang_2@oss.nxp.com?part=1

Re: [PATCH] usb: gadget: f_uac1_legacy: remove broken string configfs attributes

[Frank Li]

On Mon, Jul 13, 2026 at 01:02:46PM +0800, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> The UAC1_STR_ATTRIBUTE macro defines configfs show/store handlers for
> the fn_play, fn_cap, and fn_cntl string options. The store function
> contains an inverted null check on the kstrndup() return value.
>
> This means every write attempt returns -ENOMEM on success and
> dereferences a NULL pointer on allocation failure. The attributes
> have been broken and unused for many years.
>
> Remove the UAC1_STR_ATTRIBUTE macro and the three attributes it
> generated. The internal defaults (FILE_PCM_PLAYBACK, FILE_PCM_CAPTURE,
> FILE_CONTROL) set in f_audio_alloc_inst() are unaffected.
>
> Fixes: 0854611a19ae ("usb: gadget: f_uac1: add configfs support")
> Link: https://lore.kernel.org/linux-usb/20260625113154.1954813-1-xu.yang_2@oss.nxp.com/
> Assisted-by: Claude:claude-sonnet-4.6
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  .../testing/configfs-usb-gadget-uac1_legacy   |  3 --
>  drivers/usb/gadget/function/f_uac1_legacy.c   | 50 -------------------
>  2 files changed, 53 deletions(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy b/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
> index b2eaefd9bc49..6a681d219f43 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
> @@ -5,8 +5,5 @@ Description:
>  		The attributes:
>
>  		audio_buf_size - audio buffer size
> -		fn_cap - capture pcm device file name
> -		fn_cntl - control device file name
> -		fn_play - playback pcm device file name
>  		req_buf_size - ISO OUT endpoint request buffer size
>  		req_count - ISO OUT endpoint request count
> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 5d201a2e30e7..b15f5e074f85 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -888,60 +888,10 @@ UAC1_INT_ATTRIBUTE(req_buf_size);
>  UAC1_INT_ATTRIBUTE(req_count);
>  UAC1_INT_ATTRIBUTE(audio_buf_size);
>
> -#define UAC1_STR_ATTRIBUTE(name)					\
> -static ssize_t f_uac1_opts_##name##_show(struct config_item *item,	\
> -					 char *page)			\
> -{									\
> -	struct f_uac1_legacy_opts *opts = to_f_uac1_opts(item);		\
> -	int result;							\
> -									\
> -	mutex_lock(&opts->lock);					\
> -	result = sprintf(page, "%s\n", opts->name);			\
> -	mutex_unlock(&opts->lock);					\
> -									\
> -	return result;							\
> -}									\
> -									\
> -static ssize_t f_uac1_opts_##name##_store(struct config_item *item,	\
> -					  const char *page, size_t len)	\
> -{									\
> -	struct f_uac1_legacy_opts *opts = to_f_uac1_opts(item);		\
> -	int ret = -EBUSY;						\
> -	char *tmp;							\
> -									\
> -	mutex_lock(&opts->lock);					\
> -	if (opts->refcnt)						\
> -		goto end;						\
> -									\
> -	tmp = kstrndup(page, len, GFP_KERNEL);				\
> -	if (tmp) {							\
> -		ret = -ENOMEM;						\
> -		goto end;						\
> -	}								\
> -	if (opts->name##_alloc)						\
> -		kfree(opts->name);					\
> -	opts->name##_alloc = true;					\
> -	opts->name = tmp;						\
> -	ret = len;							\
> -									\
> -end:									\
> -	mutex_unlock(&opts->lock);					\
> -	return ret;							\
> -}									\
> -									\
> -CONFIGFS_ATTR(f_uac1_opts_, name)
> -
> -UAC1_STR_ATTRIBUTE(fn_play);
> -UAC1_STR_ATTRIBUTE(fn_cap);
> -UAC1_STR_ATTRIBUTE(fn_cntl);
> -
>  static struct configfs_attribute *f_uac1_attrs[] = {
>  	&f_uac1_opts_attr_req_buf_size,
>  	&f_uac1_opts_attr_req_count,
>  	&f_uac1_opts_attr_audio_buf_size,
> -	&f_uac1_opts_attr_fn_play,
> -	&f_uac1_opts_attr_fn_cap,
> -	&f_uac1_opts_attr_fn_cntl,
>  	NULL,
>  };
>
> --
> 2.34.1
>
>