usb_update_sn_string_descriptor generates only even-length strings

[datacompboy]

Describe the bug
When CONFIG_USB_DEVICE_SN is set to odd-length string (e.g. "CHANGEMEPLZ" :), which is 11 characters):

• the usb_update_sn_string_descriptor generates 1 byte too long (12 chars)
• failing usb_fix_ascii_sn_string_descriptor check for string length equality
• the generated USB serial remains compilation-time set fixed.

To Reproduce
Steps to reproduce the behavior:
0. Take any USB sameple (e.g. samples/subsys/usb/console)

1. Add to prj.conf line CONFIG_USB_DEVICE_SN="CHANGEMEPLZ"
2. build & run
3. See the connected device usb serial -- it would be "CHANGEMEPLZ"

Expected behavior
It should be some random string by default, e.g "A1B2C3D4E5F".

Impact
Annoyance

Additional context
The easiest fix is to add to the default usb_update_sn_string_descriptor implementation line:

sn[sizeof(CONFIG_USB_DEVICE_SN)-1] = 0;

right before the final return, which will make string length right -- at expense of not using the lowest entropy bits.

Alternate solution is to change the return to:

return &sn[1-sizeof(CONFIG_USB_DEVICE_SN)%2];

which will use string of the right length and use the lowest entropy bits, but looks little bit cryptic.

[github-actions]

Hi @datacompboy! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[datacompboy]

I understand it's low priority, the only question I have: do anyone care (i can send the fix), or it's legacy USB stack that gets phased away anyway (and it'll be removed)?

[jfischer-no]

@datacompboy What are your thoughts on this one?

diff --git a/subsys/usb/device/usb_descriptor.c b/subsys/usb/device/usb_descriptor.c
index 325989dc7fd..a55f621d1b4 100644
--- a/subsys/usb/device/usb_descriptor.c
+++ b/subsys/usb/device/usb_descriptor.c
@@ -347,12 +347,11 @@ static void usb_fix_ascii_sn_string_descriptor(struct usb_sn_descriptor *sn)
        default_sn_len = strlen(CONFIG_USB_DEVICE_SN);
 
        if (runtime_sn_len != default_sn_len) {
-               LOG_ERR("the new SN descriptor doesn't have the same "
+               LOG_WRN("the new SN descriptor doesn't have the same "
                        "length as CONFIG_USB_DEVICE_SN");
-               return;
        }
 
-       memcpy(sn->bString, runtime_sn, runtime_sn_len);
+       memcpy(sn->bString, runtime_sn, MIN(runtime_sn_len, default_sn_len));
 }

[datacompboy]

Thanks for the update! Yes, that should fix the issue (well, potentially losing the last byte of the random generated).

But I gave it a little of thoughts, and actually feel like there little more there than just odd/even case.

The Zephyr now have two ways to set the serial:

• CONFIG_USB_DEVICE_SN configuration constant
• Auto-generation of whole SN; using usb_update_sn_string_descriptor

Right now it looks like as long as the board has uniq entropy source, CONFIG_USB_DEVICE_SN is just sets the length of the serial, but not the serial itself -- the usb_update_sn_string_descriptor.

Which means, what should be intended behaviour?
Doing search thru github doesn't reveal use cases for the CONFIG_USB_DEVICE_SN, but obviously most of the use would be in private repositories...

I think the right behaviour should be explicit:

• CONFIG_USB_DEVICE_SN_TEMPLATE => something like "SN######" where only ##### part has to be replaced with the board-specific values
• CONFIG_USB_DEVICE_SN_DEFAULT => default value to use when no board entropy available
• CONFIG_USB_DEVICE_SN_REWRITE => Boolean, enables template processing
• CONFIG_USB_DEVICE_SN => if defined, generates build error with migration path to definition of TEMPLATE/DEFAULT/REWRITE depending on what developer expects here (as there is no way to know the original intention).

or do you think this is overkill?

[jfischer-no]

Right now it looks like as long as the board has uniq entropy source, CONFIG_USB_DEVICE_SN is just sets the length of the serial, but not the serial itself -- the usb_update_sn_string_descriptor.

This has historically been the case and is a placeholder.

zephyr/subsys/usb/device/Kconfig

Lines 43 to 49 in c211cb3

	config USB_DEVICE_SN
	string "USB device Serial Number String"
	default "0123456789ABCDEF"
	help
	Placeholder for USB device Serial Number String.
	Serial Number String will be derived from
	Hardware Information Driver (HWINFO).

Which means, what should be intended behaviour? Doing search thru github doesn't reveal use cases for the CONFIG_USB_DEVICE_SN, but obviously most of the use would be in private repositories...

I think the right behaviour should be explicit:

* `CONFIG_USB_DEVICE_SN_TEMPLATE` => something like "SN######" where only ##### part has to be replaced with the board-specific values

* `CONFIG_USB_DEVICE_SN_DEFAULT` => default value to use when no board entropy available

* `CONFIG_USB_DEVICE_SN_REWRITE` => Boolean, enables template processing

* `CONFIG_USB_DEVICE_SN` => if defined, generates build error with migration path to definition of TEMPLATE/DEFAULT/REWRITE depending on what developer expects here (as there is no way to know the original intention).

or do you think this is overkill?

Yes. And I do not want to introduce new Kconfig options or new features for this part.
Application can also have its own implementation where it obtains SN.

zephyr/subsys/usb/device/usb_descriptor.c

Line 304 in c211cb3

__weak uint8_t *usb_update_sn_string_descriptor(void)

[datacompboy]

This has historically been the case and is a placeholder.

Thanks, that makes sense now.