| Message ID | 20240708142553.3995022-1-jkeeping@inmusicbrands.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: f_uac2: fix non-newline-terminated function name | expand |
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
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
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
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: \
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(-)