[PATCH v2 0/3] rndis_host: handle bogus MAC addresses in ZTE RNDIS devices

[PATCH v2 0/3] rndis_host: handle bogus MAC addresses in ZTE RNDIS devices

[Lech Perczak]

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When porting support of ZTE MF286R to OpenWrt [1], it was discovered,
that its built-in LTE modem fails to adjust its target MAC address,
when a random MAC address is assigned to the interface, due to detection of
"locally-administered address" bit. This leads to dropping of ingress
trafficat the host. The modem uses RNDIS as its primary interface,
with some variants exposing both of them simultaneously.

Then it was discovered, that cdc_ether driver contains a fixup for that
exact issue, also appearing on CDC ECM interfaces.
I discussed how to proceed with that with Bjørn Mork at OpenWrt forum [3],
with the first approach would be to trust the locally-administered MAC
again, and add a quirk for the problematic ZTE devices, as suggested by
Kristian Evensen. before [4], but reusing the fixup from cdc_ether looks
like a safer and more generic solution.

Finally, according to Bjørn's suggestion. limit the scope of bogus MAC
addressdetection to ZTE devices, the same way as it is done in cdc_ether,
as this trait wasn't really observed outside of ZTE devices.
Do that for both flavours of RNDIS devices, with interface classes
02/02/ff and e0/01/03, as both types are reported by different modems.

[1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=7ac8da00609f42b8aba74b7efc6b0d055b7cef3e
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfe9b9d2df669a57a95d641ed46eb018e204c6ce
[3] https://forum.openwrt.org/t/problem-with-modem-in-zte-mf286r/120988
[4] https://lore.kernel.org/all/CAKfDRXhDp3heiD75Lat7cr1JmY-kaJ-MS0tt7QXX=s8RFjbpUQ@mail.gmail.com/T/

Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Bjørn Mork <bjorn@mork.no>
Cc: Oliver Neukum <oliver@neukum.org>

v2: ensure that MAC fixup is applied to all Ethernet frames in RNDIS
batch, by introducing a driver flag, and integrating the fixup inside
rndis_rx_fixup().

Lech Perczak (3):
  cdc_ether: export usbnet_cdc_zte_rx_fixup
  rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether
  rndis_host: limit scope of bogus MAC address detection to ZTE devices

 drivers/net/usb/cdc_ether.c    |  3 ++-
 drivers/net/usb/rndis_host.c   | 48 ++++++++++++++++++++++++++++++----
 include/linux/usb/rndis_host.h |  1 +
 include/linux/usb/usbnet.h     |  1 +
 4 files changed, 47 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] cdc_ether: export usbnet_cdc_zte_rx_fixup

[Lech Perczak]

Commit bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
introduces a workaround for certain ZTE modems reporting invalid MAC
addresses over CDC-ECM.
The same issue was present on their RNDIS interface,which was fixed in
commit a5a18bdf7453 ("rndis_host: Set valid random MAC on buggy devices").

However, internal modem of ZTE MF286R router, on its RNDIS interface, also
exhibits a second issue fixed already in CDC-ECM, of the device not
respecting configured random MAC address. In order to share the fixup for
this with rndis_host driver, export the workaround function, which will
be re-used in the following commit in rndis_host.

Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Bjørn Mork <bjorn@mork.no>
Cc: Oliver Neukum <oliver@neukum.org>
Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
---

v2:
- Updated line wrapping in commit description.
  No changes to patch contents.

 drivers/net/usb/cdc_ether.c | 3 ++-
 include/linux/usb/usbnet.h  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 9b4dfa3001d6..2de09ad5bac0 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -479,7 +479,7 @@ static int usbnet_cdc_zte_bind(struct usbnet *dev, struct usb_interface *intf)
  * device MAC address has been updated). Always set MAC address to that of the
  * device.
  */
-static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 {
 	if (skb->len < ETH_HLEN || !(skb->data[0] & 0x02))
 		return 1;
@@ -489,6 +489,7 @@ static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(usbnet_cdc_zte_rx_fixup);
 
 /* Ensure correct link state
  *
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8336e86ce606..1b4d72d5e891 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -214,6 +214,7 @@ extern int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 extern int usbnet_cdc_bind(struct usbnet *, struct usb_interface *);
 extern void usbnet_cdc_unbind(struct usbnet *, struct usb_interface *);
 extern void usbnet_cdc_status(struct usbnet *, struct urb *);
+extern int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb);
 
 /* CDC and RNDIS support the same host-chosen packet filters for IN transfers */
 #define	DEFAULT_FILTER	(USB_CDC_PACKET_TYPE_BROADCAST \
-- 
2.30.2

[PATCH v2 2/3] rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether

[Lech Perczak]

Certain ZTE modems, namely: MF823. MF831, MF910, built-in modem from
MF286R, expose both CDC-ECM and RNDIS network interfaces.
They have a trait of ignoring the locally-administered MAC address
configured on the interface both in CDC-ECM and RNDIS part,
and this leads to dropping of incoming traffic by the host.
However, the workaround was only present in CDC-ECM, and MF286R
explicitly requires it in RNDIS mode.

Re-use the workaround in rndis_host as well, to fix operation of MF286R
module, some versions of which expose only the RNDIS interface. Do so by
introducing new flag, RNDIS_DRIVER_DATA_BOGUS_MAC, and testing for it in
rndis_rx_fixup. This is required, as RNDIS uses frame batching, and all
of the packets inside the batch need the fixup. This might introduce a
performance penalty, because test is done for every returned Ethernet
frame.

Apply the workaround to both "flavors" of RNDIS interfaces, as older ZTE
modems, like MF823 found in the wild, report the USB_CLASS_COMM class
interfaces, while MF286R reports USB_CLASS_WIRELESS_CONTROLLER.

Suggested-by: Bjørn Mork <bjorn@mork.no>
Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Oliver Neukum <oliver@neukum.org>
Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
---
v2:
- Ensured that MAC fixup is applied to all Ethernet frames inside an
  RNDIS batch. Thanks to Bjørn for finding the issue. 
- Introduced new driver flag to facilitate the above.

 drivers/net/usb/rndis_host.c   | 33 ++++++++++++++++++++++++++++++++-
 include/linux/usb/rndis_host.h |  1 +
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
index 247f58cb0f84..18b27a4ed8bd 100644
--- a/drivers/net/usb/rndis_host.c
+++ b/drivers/net/usb/rndis_host.c
@@ -485,10 +485,14 @@ EXPORT_SYMBOL_GPL(rndis_unbind);
  */
 int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 {
+	bool dst_mac_fixup;
+
 	/* This check is no longer done by usbnet */
 	if (skb->len < dev->net->hard_header_len)
 		return 0;
 
+	dst_mac_fixup = !!(dev->driver_info->data & RNDIS_DRIVER_DATA_DST_MAC_FIXUP);
+
 	/* peripheral may have batched packets to us... */
 	while (likely(skb->len)) {
 		struct rndis_data_hdr	*hdr = (void *)skb->data;
@@ -523,10 +527,17 @@ int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 			break;
 		skb_pull(skb, msg_len - sizeof *hdr);
 		skb_trim(skb2, data_len);
+
+		if (unlikely(dst_mac_fixup))
+			usbnet_cdc_zte_rx_fixup(dev, skb2);
+
 		usbnet_skb_return(dev, skb2);
 	}
 
 	/* caller will usbnet_skb_return the remaining packet */
+	if (unlikely(dst_mac_fixup))
+		usbnet_cdc_zte_rx_fixup(dev, skb);
+
 	return 1;
 }
 EXPORT_SYMBOL_GPL(rndis_rx_fixup);
@@ -578,7 +589,6 @@ rndis_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 }
 EXPORT_SYMBOL_GPL(rndis_tx_fixup);
 
-
 static const struct driver_info	rndis_info = {
 	.description =	"RNDIS device",
 	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
@@ -600,6 +610,17 @@ static const struct driver_info	rndis_poll_status_info = {
 	.tx_fixup =	rndis_tx_fixup,
 };
 
+static const struct driver_info	zte_rndis_info = {
+	.description =	"ZTE RNDIS device",
+	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
+	.data =		RNDIS_DRIVER_DATA_DST_MAC_FIXUP,
+	.bind =		rndis_bind,
+	.unbind =	rndis_unbind,
+	.status =	rndis_status,
+	.rx_fixup =	rndis_rx_fixup,
+	.tx_fixup =	rndis_tx_fixup,
+};
+
 /*-------------------------------------------------------------------------*/
 
 static const struct usb_device_id	products [] = {
@@ -613,6 +634,16 @@ static const struct usb_device_id	products [] = {
 	USB_VENDOR_AND_INTERFACE_INFO(0x238b,
 				      USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
 	.driver_info = (unsigned long)&rndis_info,
+}, {
+	/* ZTE WWAN modules */
+	USB_VENDOR_AND_INTERFACE_INFO(0x19d2,
+				      USB_CLASS_WIRELESS_CONTROLLER, 1, 3),
+	.driver_info = (unsigned long)&zte_rndis_info,
+}, {
+	/* ZTE WWAN modules, ACM flavour */
+	USB_VENDOR_AND_INTERFACE_INFO(0x19d2,
+				      USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
+	.driver_info = (unsigned long)&zte_rndis_info,
 }, {
 	/* RNDIS is MSFT's un-official variant of CDC ACM */
 	USB_INTERFACE_INFO(USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
diff --git a/include/linux/usb/rndis_host.h b/include/linux/usb/rndis_host.h
index 809bccd08455..cc42db51bbba 100644
--- a/include/linux/usb/rndis_host.h
+++ b/include/linux/usb/rndis_host.h
@@ -197,6 +197,7 @@ struct rndis_keepalive_c {	/* IN (optionally OUT) */
 
 /* Flags for driver_info::data */
 #define RNDIS_DRIVER_DATA_POLL_STATUS	1	/* poll status before control */
+#define RNDIS_DRIVER_DATA_DST_MAC_FIXUP	2	/* device ignores configured MAC address */
 
 extern void rndis_status(struct usbnet *dev, struct urb *urb);
 extern int
-- 
2.30.2

[PATCH v2 3/3] rndis_host: limit scope of bogus MAC address detection to ZTE devices

[Lech Perczak]

Reporting of bogus MAC addresses and ignoring configuration of new
destination address wasn't observed outside of a range of ZTE devices,
among which this seems to be the common bug. Align rndis_host driver
with implementation found in cdc_ether, which also limits this workaround
to ZTE devices.

Suggested-by: Bjørn Mork <bjorn@mork.no>
Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Oliver Neukum <oliver@neukum.org>
Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
---

v2:
- No logical changes, just rebased on top of previous patches.

 drivers/net/usb/rndis_host.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
index 18b27a4ed8bd..c5e0a5f659b1 100644
--- a/drivers/net/usb/rndis_host.c
+++ b/drivers/net/usb/rndis_host.c
@@ -418,10 +418,7 @@ generic_rndis_bind(struct usbnet *dev, struct usb_interface *intf, int flags)
 		goto halt_fail_and_release;
 	}
 
-	if (bp[0] & 0x02)
-		eth_hw_addr_random(net);
-	else
-		eth_hw_addr_set(net, bp);
+	eth_hw_addr_set(net, bp);
 
 	/* set a nonzero filter to enable data transfers */
 	memset(u.set, 0, sizeof *u.set);
@@ -463,6 +460,16 @@ static int rndis_bind(struct usbnet *dev, struct usb_interface *intf)
 	return generic_rndis_bind(dev, intf, FLAG_RNDIS_PHYM_NOT_WIRELESS);
 }
 
+static int zte_rndis_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int status = rndis_bind(dev, intf);
+
+	if (!status && (dev->net->dev_addr[0] & 0x02))
+		eth_hw_addr_random(dev->net);
+
+	return status;
+}
+
 void rndis_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct rndis_halt	*halt;
@@ -614,7 +621,7 @@ static const struct driver_info	zte_rndis_info = {
 	.description =	"ZTE RNDIS device",
 	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
 	.data =		RNDIS_DRIVER_DATA_DST_MAC_FIXUP,
-	.bind =		rndis_bind,
+	.bind =		zte_rndis_bind,
 	.unbind =	rndis_unbind,
 	.status =	rndis_status,
 	.rx_fixup =	rndis_rx_fixup,
-- 
2.30.2

Re: [PATCH v2 2/3] rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether

[Lech Perczak]

W dniu 2022-04-13 o 02:11, Lech Perczak pisze:
> Certain ZTE modems, namely: MF823. MF831, MF910, built-in modem from
> MF286R, expose both CDC-ECM and RNDIS network interfaces.
> They have a trait of ignoring the locally-administered MAC address
> configured on the interface both in CDC-ECM and RNDIS part,
> and this leads to dropping of incoming traffic by the host.
> However, the workaround was only present in CDC-ECM, and MF286R
> explicitly requires it in RNDIS mode.
>
> Re-use the workaround in rndis_host as well, to fix operation of MF286R
> module, some versions of which expose only the RNDIS interface. Do so by
> introducing new flag, RNDIS_DRIVER_DATA_BOGUS_MAC, and testing for it in
And I just noticed that I forgot to rename that flag here, as well as
one unneded whitespace change creeped in. Will resend V3 shortly.
> rndis_rx_fixup. This is required, as RNDIS uses frame batching, and all
> of the packets inside the batch need the fixup. This might introduce a
> performance penalty, because test is done for every returned Ethernet
> frame.
>
> Apply the workaround to both "flavors" of RNDIS interfaces, as older ZTE
> modems, like MF823 found in the wild, report the USB_CLASS_COMM class
> interfaces, while MF286R reports USB_CLASS_WIRELESS_CONTROLLER.
>
> Suggested-by: Bjørn Mork <bjorn@mork.no>
> Cc: Kristian Evensen <kristian.evensen@gmail.com>
> Cc: Oliver Neukum <oliver@neukum.org>
> Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
> ---
> v2:
> - Ensured that MAC fixup is applied to all Ethernet frames inside an
>    RNDIS batch. Thanks to Bjørn for finding the issue.
> - Introduced new driver flag to facilitate the above.
>
>   drivers/net/usb/rndis_host.c   | 33 ++++++++++++++++++++++++++++++++-
>   include/linux/usb/rndis_host.h |  1 +
>   2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> index 247f58cb0f84..18b27a4ed8bd 100644
> --- a/drivers/net/usb/rndis_host.c
> +++ b/drivers/net/usb/rndis_host.c
> @@ -485,10 +485,14 @@ EXPORT_SYMBOL_GPL(rndis_unbind);
>    */
>   int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>   {
> +	bool dst_mac_fixup;
> +
>   	/* This check is no longer done by usbnet */
>   	if (skb->len < dev->net->hard_header_len)
>   		return 0;
>   
> +	dst_mac_fixup = !!(dev->driver_info->data & RNDIS_DRIVER_DATA_DST_MAC_FIXUP);
> +
>   	/* peripheral may have batched packets to us... */
>   	while (likely(skb->len)) {
>   		struct rndis_data_hdr	*hdr = (void *)skb->data;
> @@ -523,10 +527,17 @@ int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>   			break;
>   		skb_pull(skb, msg_len - sizeof *hdr);
>   		skb_trim(skb2, data_len);
> +
> +		if (unlikely(dst_mac_fixup))
> +			usbnet_cdc_zte_rx_fixup(dev, skb2);
> +
>   		usbnet_skb_return(dev, skb2);
>   	}
>   
>   	/* caller will usbnet_skb_return the remaining packet */
> +	if (unlikely(dst_mac_fixup))
> +		usbnet_cdc_zte_rx_fixup(dev, skb);
> +
>   	return 1;
>   }
>   EXPORT_SYMBOL_GPL(rndis_rx_fixup);
> @@ -578,7 +589,6 @@ rndis_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>   }
>   EXPORT_SYMBOL_GPL(rndis_tx_fixup);
>   
> -
>   static const struct driver_info	rndis_info = {
>   	.description =	"RNDIS device",
>   	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
> @@ -600,6 +610,17 @@ static const struct driver_info	rndis_poll_status_info = {
>   	.tx_fixup =	rndis_tx_fixup,
>   };
>   
> +static const struct driver_info	zte_rndis_info = {
> +	.description =	"ZTE RNDIS device",
> +	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
> +	.data =		RNDIS_DRIVER_DATA_DST_MAC_FIXUP,
> +	.bind =		rndis_bind,
> +	.unbind =	rndis_unbind,
> +	.status =	rndis_status,
> +	.rx_fixup =	rndis_rx_fixup,
> +	.tx_fixup =	rndis_tx_fixup,
> +};
> +
>   /*-------------------------------------------------------------------------*/
>   
>   static const struct usb_device_id	products [] = {
> @@ -613,6 +634,16 @@ static const struct usb_device_id	products [] = {
>   	USB_VENDOR_AND_INTERFACE_INFO(0x238b,
>   				      USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
>   	.driver_info = (unsigned long)&rndis_info,
> +}, {
> +	/* ZTE WWAN modules */
> +	USB_VENDOR_AND_INTERFACE_INFO(0x19d2,
> +				      USB_CLASS_WIRELESS_CONTROLLER, 1, 3),
> +	.driver_info = (unsigned long)&zte_rndis_info,
> +}, {
> +	/* ZTE WWAN modules, ACM flavour */
> +	USB_VENDOR_AND_INTERFACE_INFO(0x19d2,
> +				      USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
> +	.driver_info = (unsigned long)&zte_rndis_info,
>   }, {
>   	/* RNDIS is MSFT's un-official variant of CDC ACM */
>   	USB_INTERFACE_INFO(USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
> diff --git a/include/linux/usb/rndis_host.h b/include/linux/usb/rndis_host.h
> index 809bccd08455..cc42db51bbba 100644
> --- a/include/linux/usb/rndis_host.h
> +++ b/include/linux/usb/rndis_host.h
> @@ -197,6 +197,7 @@ struct rndis_keepalive_c {	/* IN (optionally OUT) */
>   
>   /* Flags for driver_info::data */
>   #define RNDIS_DRIVER_DATA_POLL_STATUS	1	/* poll status before control */
> +#define RNDIS_DRIVER_DATA_DST_MAC_FIXUP	2	/* device ignores configured MAC address */
>   
>   extern void rndis_status(struct usbnet *dev, struct urb *urb);
>   extern int


-- 
Pozdrawiam/Kind regards,
Lech Perczak

Re: [PATCH v2 1/3] cdc_ether: export usbnet_cdc_zte_rx_fixup

[Oliver Neukum]

On 13.04.22 02:11, Lech Perczak wrote:
> Commit bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
> introduces a workaround for certain ZTE modems reporting invalid MAC
> addresses over CDC-ECM.
> The same issue was present on their RNDIS interface,which was fixed in
> commit a5a18bdf7453 ("rndis_host: Set valid random MAC on buggy devices").
>
> However, internal modem of ZTE MF286R router, on its RNDIS interface, also
> exhibits a second issue fixed already in CDC-ECM, of the device not
> respecting configured random MAC address. In order to share the fixup for
> this with rndis_host driver, export the workaround function, which will
> be re-used in the following commit in rndis_host.
>
> Cc: Kristian Evensen <kristian.evensen@gmail.com>
> Cc: Bjørn Mork <bjorn@mork.no>
> Cc: Oliver Neukum <oliver@neukum.org>
> Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
>
Acked-by: Oliver Neukum <oneukum@suse.com>