[PATCH] usb: gadget: f_uac2: fix non-newline-terminated function name

[PATCH] usb: gadget: f_uac2: fix non-newline-terminated function name

[John Keeping]

Most writes to configfs handle an optional newline, but do not require
it.  By using the number of bytes written as the limit for scnprintf()
it is guaranteed that the final character in the buffer will be
overwritten.

This is expected if it is a newline but is undesirable when a string is
written "as-is" (as libusbgx does, for example).

Update the store function to strip an optional newline, matching the
behaviour of usb_string_copy().

Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
---
 drivers/usb/gadget/function/f_uac2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index f85ffa24a5cd5..2d6d3286ffde2 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -2063,7 +2063,10 @@ static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
 		goto end;						\
 	}								\
 									\
-	ret = scnprintf(opts->name, min(sizeof(opts->name), len),	\
+	if (len && page[len - 1] == '\n')				\
+		len--;							\
+									\
+	ret = scnprintf(opts->name, min(sizeof(opts->name), len + 1),	\
 			"%s", page);					\
 									\
 end:									\
-- 
2.45.2

Re: [PATCH] usb: gadget: f_uac2: fix non-newline-terminated function name

[Greg Kroah-Hartman]

On Mon, Jul 08, 2024 at 03:25:53PM +0100, John Keeping wrote:
> Most writes to configfs handle an optional newline, but do not require
> it.  By using the number of bytes written as the limit for scnprintf()
> it is guaranteed that the final character in the buffer will be
> overwritten.
> 
> This is expected if it is a newline but is undesirable when a string is
> written "as-is" (as libusbgx does, for example).

So we are changing kernel functionality because a userspace program does
not work?  Why not fix the userspace program?

> Update the store function to strip an optional newline, matching the
> behaviour of usb_string_copy().

This changes the behaviour of a lot of configfs files right?  What will
break if this happens?

thanks,

greg k-h

Re: [PATCH] usb: gadget: f_uac2: fix non-newline-terminated function name

[John Keeping]

On Wed, Jul 10, 2024 at 01:53:22PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 08, 2024 at 03:25:53PM +0100, John Keeping wrote:
> > Most writes to configfs handle an optional newline, but do not require
> > it.  By using the number of bytes written as the limit for scnprintf()
> > it is guaranteed that the final character in the buffer will be
> > overwritten.
> > 
> > This is expected if it is a newline but is undesirable when a string is
> > written "as-is" (as libusbgx does, for example).
> 
> So we are changing kernel functionality because a userspace program does
> not work?  Why not fix the userspace program?

This file behaves differently from every other sysfs/debugfs/configfs
file AFAICT.  In most places the behaviour of the following two commands
is equivalent:

	$ echo foo >file

	$ printf foo >file

But for this function_name the result is that the final character is
dropped unconditionally, so the name reported in the USB descriptors
will be "fo" in the second case.

> > Update the store function to strip an optional newline, matching the
> > behaviour of usb_string_copy().
> 
> This changes the behaviour of a lot of configfs files right?  What will
> break if this happens?

No, this is just one file in f_uac2.

I can't see any scenario where a newline in a USB string descriptor
makes sense so it's unlikely to break any existing use cases.

This brings the audio function name more in line with other string
descriptors for the device manufacturer/product or configuration name
which use usb_string_copy() and strip a trailing newline if it's
present.


Regards,
John

Re: [PATCH] usb: gadget: f_uac2: fix non-newline-terminated function name

[Greg Kroah-Hartman]

On Wed, Jul 10, 2024 at 02:30:01PM +0100, John Keeping wrote:
> On Wed, Jul 10, 2024 at 01:53:22PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 08, 2024 at 03:25:53PM +0100, John Keeping wrote:
> > > Most writes to configfs handle an optional newline, but do not require
> > > it.  By using the number of bytes written as the limit for scnprintf()
> > > it is guaranteed that the final character in the buffer will be
> > > overwritten.
> > > 
> > > This is expected if it is a newline but is undesirable when a string is
> > > written "as-is" (as libusbgx does, for example).
> > 
> > So we are changing kernel functionality because a userspace program does
> > not work?  Why not fix the userspace program?
> 
> This file behaves differently from every other sysfs/debugfs/configfs
> file AFAICT.  In most places the behaviour of the following two commands
> is equivalent:
> 
> 	$ echo foo >file
> 
> 	$ printf foo >file
> 
> But for this function_name the result is that the final character is
> dropped unconditionally, so the name reported in the USB descriptors
> will be "fo" in the second case.
> 
> > > Update the store function to strip an optional newline, matching the
> > > behaviour of usb_string_copy().
> > 
> > This changes the behaviour of a lot of configfs files right?  What will
> > break if this happens?
> 
> No, this is just one file in f_uac2.
> 
> I can't see any scenario where a newline in a USB string descriptor
> makes sense so it's unlikely to break any existing use cases.
> 
> This brings the audio function name more in line with other string
> descriptors for the device manufacturer/product or configuration name
> which use usb_string_copy() and strip a trailing newline if it's
> present.

Ok, thanks for the added info, now applied.

greg k-h