The CDC-ECM specification [1] requires to send the host MAC address as an uppercase hexadecimal string in chapter "5.4 Ethernet Networking Functional Descriptor": The Unicode character is chosen from the set of values 30h through 39h and 41h through 46h (0-9 and A-F).

However, snprintf(.., "%pm", ..) generates a lowercase MAC address string. While most host drivers are tolerant to this, UsbNcm.sys on Windows 10 is not. Instead it uses a different MAC address with all bytes set to zero including and after the first byte containing a lowercase letter. On Windows 11 Microsoft fixed it, but apparently they did not backport the fix.

This change fixes the issue by using "%pmU" to generate an uppercase hex string to comply with the specification.

[1]: https://www.usb.org/document-library/class-definitions-communication-devices..., file ECM120.pdf

Fixes: bcd4a1c40bee ("usb: gadget: u_ether: construct with default values and add setters/getters") Cc: stable@vger.kernel.org Signed-off-by: Konrad Gräfe k.graefe@gateware.de ---

Changes since v3: None

Changes since v2: * Add uppercase MAC address format string and use that instead of manually uppercasing the resulting MAC address string.

Changes since v1: * Fixed checkpatch.pl warnings

drivers/usb/gadget/function/u_ether.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c index 6956ad8ba8dd..70e6b825654c 100644 --- a/drivers/usb/gadget/function/u_ether.c +++ b/drivers/usb/gadget/function/u_ether.c @@ -963,7 +963,7 @@ int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len) return -EINVAL;

dev = netdev_priv(net); - snprintf(host_addr, len, "%pm", dev->host_mac); + snprintf(host_addr, len, "%pmU", dev->host_mac);

return strlen(host_addr); }

The CDC-ECM specification [1] requires to send the host MAC address as an uppercase hexadecimal string in chapter "5.4 Ethernet Networking Functional Descriptor": The Unicode character is chosen from the set of values 30h through 39h and 41h through 46h (0-9 and A-F).

However, snprintf(.., "%pm", ..) generates a lowercase MAC address string. While most host drivers are tolerant to this, UsbNcm.sys on Windows 10 is not. Instead it uses a different MAC address with all bytes set to zero including and after the first byte containing a lowercase letter. On Windows 11 Microsoft fixed it, but apparently they did not backport the fix.

This change fixes the issue by upper-casing the MAC to comply with the specification.

[1]: https://www.usb.org/document-library/class-definitions-communication-devices..., file ECM120.pdf

Fixes: bcd4a1c40bee ("usb: gadget: u_ether: construct with default values and add setters/getters") Cc: stable@vger.kernel.org Signed-off-by: Konrad Gräfe k.graefe@gateware.de --- Changes since v4: * Use string_upper() instead of a special format string

Changes since v3: None

Changes since v2: * Add uppercase MAC address format string and use that instead of manually uppercasing the resulting MAC address string.

Changes since v1: * Fixed checkpatch.pl warnings

drivers/usb/gadget/function/u_ether.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c index 6956ad8ba8dd..a366abb45623 100644 --- a/drivers/usb/gadget/function/u_ether.c +++ b/drivers/usb/gadget/function/u_ether.c @@ -17,6 +17,7 @@ #include <linux/etherdevice.h> #include <linux/ethtool.h> #include <linux/if_vlan.h> +#include <linux/string_helpers.h> #include <linux/usb/composite.h>

#include "u_ether.h" @@ -965,6 +966,8 @@ int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len) dev = netdev_priv(net); snprintf(host_addr, len, "%pm", dev->host_mac);

+ string_upper(host_addr, host_addr); + return strlen(host_addr); } EXPORT_SYMBOL_GPL(gether_get_host_addr_cdc);

Hello Rasmus, Konrad, *,

On Thu, 27 Apr 2023 14:35:19 +0200, Rasmus Villemoes linux@rasmusvillemoes.dk wrote:

On 27/04/2023 13.51, Konrad Gräfe wrote:

...

The CDC-ECM specification requires an USB gadget to send the host MAC address as uppercase hex string. This change adds the appropriate modifier.

Cc: stable@vger.kernel.org

Why cc stable?

...

Signed-off-by: Konrad Gräfe k.graefe@gateware.de

Added in v3

lib/vsprintf.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)

The diffstat here, or for some other patch in the same series, definitely ought to mention lib/test_printf.c.

...

diff --git a/lib/vsprintf.c b/lib/vsprintf.c index be71a03c936a..8aee1caabd9e 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, { char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")]; char *p = mac_addr;

• int i;

• int i, pos; char separator; bool reversed = false;
• bool uppercase = false;

if (check_pointer(&buf, end, addr, spec)) return buf; @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, separator = '-'; break;

• case 'U':

• uppercase = true;
  

• break;
  

• case 'R': reversed = true; fallthrough;

This seems broken, and I'm surprised the compiler doesn't warn about separator possibly being uninitialized further down. I'm also surprised your testing hasn't caught this. For reference, the full switch statement is currently

Compiler (gcc) does not warn because of Makefile:

1038 # Enabled with W=2, disabled by default as noisy 1039 ifdef CONFIG_CC_IS_GCC 1040 KBUILD_CFLAGS += -Wno-maybe-uninitialized 1041 endif

With this commented:

lib/vsprintf.c: In function ‘mac_address_string’: lib/vsprintf.c:1310:30: warning: ‘separator’ may be used uninitialized [-Wmaybe-uninitialized] 1310 | *p++ = separator; | ~~~~~^~~~~~~~~~~ lib/vsprintf.c:1273:14: note: ‘separator’ was declared here 1273 | char separator; | ^~~~~~~~~

Regards, Peter

    switch (fmt[1]) {
    case 'F':
            separator = '-';
            break;

    case 'R':
            reversed = true;
            fallthrough;

    default:
            separator = ':';
            break;
    }

...

@@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr, for (i = 0; i < 6; i++) { if (reversed)

• 	p = hex_byte_pack(p, addr[5 - i]);
  

• 	pos = 5 - i;
  

• else
  

• 	pos = i;
  

• if (uppercase)
  

• 	p = hex_byte_pack_upper(p, addr[pos]);
  

  else

• 	p = hex_byte_pack(p, addr[i]);
  

• 	p = hex_byte_pack(p, addr[pos]);  
  

I think this becomes quite hard to follow. We have string_upper() in linux/string_helpers.h, so I'd rather just leave this loop alone and do

if (uppercase) string_upper(mac_addr, mac_addr);

after the nul-termination.

Rasmus