[PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[Jussi Peltola]

This brokenness appears reliably after running "rdisc6 wwan0" but I have
not debugged if this is related to timing or the format of the router
solicitation. Before receiving a router solicitation, v4 is received
correctly and v6 does not work. After sending the MF821D a router
solicitation with rdisc6, v4 is broken but v6 works. With this patch
(which I am using to write this message) a dual-stack context is usable.

commit 2c25237d19c0c9741c6ebec854def99b88618eac
Author: Jussi Peltola <plz@plz.fi>
Date:   Sun Nov 13 15:41:50 2016 +0200

    Fixup packets with incorrect ethertype sent by ZTE MF821D
    
    Signed-off-by: Jussi Peltola <plz@plz.fi>

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 3ff76c6..edd8172 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -172,6 +172,12 @@ static const u8 buggy_fw_addr[ETH_ALEN] = {0x00, 0xa0, 0xc6, 0x00, 0x00, 0x00};
  * Another common firmware bug results in all packets being addressed
  * to 00:a0:c6:00:00:00 despite the host address being different.
  * This function will also fixup such packets.
+ *
+ * At least the ZTE MF821D sends IPv4 packets with a bogus ethertype
+ * of 0x63bc, 0xe3bc, 0x63bd or 0xe3bd, and bogus source and
+ * destination MACs after it has received an IPv6 router solicitation
+ * (IPv6 is transmitted correctly). This function will also fix up
+ * such packets.
  */
 static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 {
@@ -195,12 +201,21 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
                        return 0;
                if (is_multicast_ether_addr(skb->data))
                        return 1;
+
                /* possibly bogus destination - rewrite just in case */
                skb_reset_mac_header(skb);
                goto fix_dest;
        default:
                if (rawip)
                        return 0;
+
+               /* Bogus ethertype and src/dst mac for v4 on ZTE MF821D */
+               if ((skb->data[12] & 0x7f) == 0x63
+                && (skb->data[13] & 0xfe) == 0xbc) {
+                       proto = htons(ETH_P_IP);
+                       goto reset_mac;
+               }
+
                /* pass along other packets without modifications */
                return 1;
        }
@@ -213,6 +228,7 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
        if (skb_headroom(skb) < ETH_HLEN)
                return 0;
        skb_push(skb, ETH_HLEN);
+reset_mac:
        skb_reset_mac_header(skb);
        eth_hdr(skb)->h_proto = proto;
        eth_zero_addr(eth_hdr(skb)->h_source);

Re: [PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[Bjørn Mork]

Jussi Peltola <plz@plz.fi> writes:

> This brokenness appears reliably after running "rdisc6 wwan0" but I have
> not debugged if this is related to timing or the format of the router
> solicitation. Before receiving a router solicitation, v4 is received
> correctly and v6 does not work. After sending the MF821D a router
> solicitation with rdisc6, v4 is broken but v6 works. With this patch
> (which I am using to write this message) a dual-stack context is usable.
>
> commit 2c25237d19c0c9741c6ebec854def99b88618eac
> Author: Jussi Peltola <plz@plz.fi>
> Date:   Sun Nov 13 15:41:50 2016 +0200
>
>     Fixup packets with incorrect ethertype sent by ZTE MF821D
>     
>     Signed-off-by: Jussi Peltola <plz@plz.fi>
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 3ff76c6..edd8172 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -172,6 +172,12 @@ static const u8 buggy_fw_addr[ETH_ALEN] = {0x00, 0xa0, 0xc6, 0x00, 0x00, 0x00};
>   * Another common firmware bug results in all packets being addressed
>   * to 00:a0:c6:00:00:00 despite the host address being different.
>   * This function will also fixup such packets.
> + *
> + * At least the ZTE MF821D sends IPv4 packets with a bogus ethertype
> + * of 0x63bc, 0xe3bc, 0x63bd or 0xe3bd, and bogus source and
> + * destination MACs after it has received an IPv6 router solicitation
> + * (IPv6 is transmitted correctly). This function will also fix up
> + * such packets.
>   */
>  static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  {
> @@ -195,12 +201,21 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>                         return 0;
>                 if (is_multicast_ether_addr(skb->data))
>                         return 1;
> +
>                 /* possibly bogus destination - rewrite just in case */
>                 skb_reset_mac_header(skb);
>                 goto fix_dest;
>         default:
>                 if (rawip)
>                         return 0;
> +
> +               /* Bogus ethertype and src/dst mac for v4 on ZTE MF821D */
> +               if ((skb->data[12] & 0x7f) == 0x63
> +                && (skb->data[13] & 0xfe) == 0xbc) {
> +                       proto = htons(ETH_P_IP);
> +                       goto reset_mac;
> +               }
> +
>                 /* pass along other packets without modifications */
>                 return 1;
>         }
> @@ -213,6 +228,7 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>         if (skb_headroom(skb) < ETH_HLEN)
>                 return 0;
>         skb_push(skb, ETH_HLEN);
> +reset_mac:
>         skb_reset_mac_header(skb);
>         eth_hdr(skb)->h_proto = proto;
>         eth_zero_addr(eth_hdr(skb)->h_source);


I must admit that I don't like the additional magic stuff at all.  I
don't doubt a second that it fixes the problem for you.  But there is
also no doubt that the firmware is so broken that there is no way we can
know this fixes anything for anyone else.  Where did the firmware get
those magic numbers?  It's definitely garbage.  But we have no reason to
think the garbage is invariable. It could be part of the firmware code,
or a serial number, or your assigned IPv6 prefix, or something else that
changes between different modems.  The fact that it is stable for you is
unfortunately not proof that it is the same for everyone else.

We have rejected similar fixes when this issue has come up earlier (it
looks like a generic Qualcomm firmware issue shared with other vendors
using the same baseband firmware generation). Using raw-ip is the
recommended workaround for these modems.

In any case, if we're going to add a fix like this, then I want it way
more generic.  The only valid ethertypes expected from the modem is IP,
IPV6 or ARP. Testing against those three, resetting anything else to IP,
will at least catch *any* bogus value.

But I'm not convinced I want this additional processing of every packet
just to let Qualcomm go on hiring monkeys-on-crack to write their
firmware. At least not when we do have raw-ip as a workaround for the
issue.

Feel free to try to convince me, though.


Bjørn

Re: [PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[David Miller]

From: Bjørn Mork <bjorn@mork.no>
Date: Sun, 13 Nov 2016 17:12:57 +0100

> In any case, if we're going to add a fix like this, then I want it way
> more generic.  The only valid ethertypes expected from the modem is IP,
> IPV6 or ARP. Testing against those three, resetting anything else to IP,
> will at least catch *any* bogus value.
> 
> But I'm not convinced I want this additional processing of every packet
> just to let Qualcomm go on hiring monkeys-on-crack to write their
> firmware. At least not when we do have raw-ip as a workaround for the
> issue.
> 
> Feel free to try to convince me, though.

There are also several coding style problems with this patch.

Re: [PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[Jussi Peltola]

On Sun, Nov 13, 2016 at 05:12:57PM +0100, Bjørn Mork wrote:
> In any case, if we're going to add a fix like this, then I want it way
> more generic.  The only valid ethertypes expected from the modem is IP,
> IPV6 or ARP. Testing against those three, resetting anything else to IP,
> will at least catch *any* bogus value.
 
Yes, this is pretty obvious, and it's pretty easy to look for an initial
4 or 6 and assume anything else is ARP (and just pass it through since
no reports of wrong ARP ethertype have been seen as far as I know and
fixing ARP up is probably futile if the L2 addresses in the body or
header don't make sense.)

I didn't bother writing this before sending this one first to provoke
discussion. It should actually be pretty simple; just pop the L2 header
if it looks like it's too broken and later add a new one if (!rawip).

> But I'm not convinced I want this additional processing of every packet
> just to let Qualcomm go on hiring monkeys-on-crack to write their
> firmware. At least not when we do have raw-ip as a workaround for the
> issue.
> 
> Feel free to try to convince me, though.

This is a modem widely sold in Finland by one of the telcos that has
enabled IPv6 to all subscribers. So the population affected is
definitely not just one person - but I will have to see if rawip works
and then see if ModemManager can be made to use that by default. I
didn't initially even think of trying, because v6 only works on this
modem after a router solicit. But who knows...

I find bugs like this, where the general answer for users (or userspace
parts like ModemManager) is "just disable this ipv6 crap" pretty nasty.
Even if the required fix is not exactly elegant, brokenness like this
can greatly set back getting v6 enabled when available.

As more v6 is deployed around the world I would expect more broken
Qualcomm modems to show themselves. I'll have to try to test this modem
to see if this bug disappears when the telco does not have IPv6; if so,
people are in for annoying surprises as the modem suddenly stops working
when the telco deploys v6.

I can agree with the lack of elegance. But I don't see processing per
every packet as a significant issue when the devices connect over USB
and their transfer rates are limited by the real world performance of
mobile networks, and the other option is that the device just doesn't
work at all. A knob would definitely feel wrong, as there is no
indication this logic will ever break any functionality for anyone, it
just wastes a few CPU cycles if the modem is not broken.

Re: [PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[Jussi Peltola]

So here's another stab. The comments and the current implementation are
not in sync: any non-multicast address starting with a null octet gets
rewritten, while the comment specifically mentions 00:a0:c6:00:00:00. It
is certainly not elegant but re-writing all unicast destinations with
our address does come to mind instead of special cases.

This patch fails to handle the invalid destinations in either way so I
will send another one if you think it's worthwhile to go on. And it
seems I forgot htons but I need this device for work now so a better
patch must wait :)

commit 35d3a46b7f1ece70e24386acbdd16af4507cb5f3
Author: Jussi Peltola <plz@plz.fi>
Date:   Mon Nov 14 01:45:32 2016 +0200

    Attempt to fix up packets with a broken ethernet header
    
    Signed-off-by: Jussi Peltola <plz@plz.fi>

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 3ff76c6..7308d6b 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -153,25 +153,57 @@ static const u8 default_modem_addr[ETH_ALEN] = {0x02, 0x50, 0xf3};
 
 static const u8 buggy_fw_addr[ETH_ALEN] = {0x00, 0xa0, 0xc6, 0x00, 0x00, 0x00};
 
-/* Make up an ethernet header if the packet doesn't have one.
+/* Check if the ethernet header has an unknown ethertype, and return a
+ * guess of the correct one based on the L3 header, or zero if the type was
+ * known or detection failed.
+ */
+static __be16 detect_bogus_header(struct sk_buff *skb) {
+       struct ethhdr *eth_hdr = (struct ethhdr*) skb->data;
+
+       switch (eth_hdr->h_proto) {
+       case ETH_P_IP:
+       case ETH_P_IPV6:
+       case ETH_P_ARP:
+               return 0;
+       default:
+               switch (skb->data[14] & 0xf0) {
+               case 0x40:
+                       return htons(ETH_P_IP);
+               case 0x60:
+                       return htons(ETH_P_IPV6);
+               default:
+                       /* pass on undetectable packets */
+                       return 0;
+               }
+       }
+       /*NOTREACHED*/
+       return 0;
+}
+
+/* Make up an ethernet header if the packet doesn't have a correct one.
  *
  * A firmware bug common among several devices cause them to send raw
  * IP packets under some circumstances.  There is no way for the
  * driver/host to know when this will happen.  And even when the bug
  * hits, some packets will still arrive with an intact header.
  *
- * The supported devices are only capably of sending IPv4, IPv6 and
+ * The supported devices are only capable of sending IPv4, IPv6 and
  * ARP packets on a point-to-point link. Any packet with an ethernet
  * header will have either our address or a broadcast/multicast
- * address as destination.  ARP packets will always have a header.
+ * address as destination. ARP packets will always have a header.
  *
  * This means that this function will reliably add the appropriate
- * header iff necessary, provided our hardware address does not start
+ * header if necessary, provided our hardware address does not start
  * with 4 or 6.
  *
  * Another common firmware bug results in all packets being addressed
  * to 00:a0:c6:00:00:00 despite the host address being different.
- * This function will also fixup such packets.
+ *
+ * Some devices will send packets with garbage source/destination MACs and
+ * ethertypes.
+ *
+ * This function will try to fix up all such packets.
+ *
  */
 static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 {
@@ -179,8 +211,8 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
        bool rawip = info->flags & QMI_WWAN_FLAG_RAWIP;
        __be16 proto;
 
-       /* This check is no longer done by usbnet */
-       if (skb->len < dev->net->hard_header_len)
+       /* Shorter is definitely invalid and breaks subsequent tests */
+       if (skb->len < 15)
                return 0;
 
        switch (skb->data[0] & 0xf0) {
@@ -190,17 +222,17 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
        case 0x60:
                proto = htons(ETH_P_IPV6);
                break;
-       case 0x00:
+       default:
                if (rawip)
                        return 0;
                if (is_multicast_ether_addr(skb->data))
                        return 1;
-               /* possibly bogus destination - rewrite just in case */
-               skb_reset_mac_header(skb);
-               goto fix_dest;
-       default:
-               if (rawip)
-                       return 0;
+               proto = detect_bogus_header(skb);
+               if (proto) {
+                       /* remove terminally broken header */
+                       skb_pull(skb, ETH_HLEN);
+                       break;
+               }
                /* pass along other packets without modifications */
                return 1;
        }
@@ -208,17 +240,17 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
                skb->dev = dev->net; /* normally set by eth_type_trans */
                skb->protocol = proto;
                return 1;
+       } else {
+               /* push a made-up header */
+               if (skb_headroom(skb) < ETH_HLEN)
+                       return 0;
+               skb_push(skb, ETH_HLEN);
+               skb_reset_mac_header(skb);
+               eth_hdr(skb)->h_proto = proto;
+               eth_zero_addr(eth_hdr(skb)->h_source);
+               memcpy(eth_hdr(skb)->h_dest, dev->net->dev_addr, ETH_ALEN);
+               return 1;
        }
-
-       if (skb_headroom(skb) < ETH_HLEN)
-               return 0;
-       skb_push(skb, ETH_HLEN);
-       skb_reset_mac_header(skb);
-       eth_hdr(skb)->h_proto = proto;
-       eth_zero_addr(eth_hdr(skb)->h_source);
-fix_dest:
-       memcpy(eth_hdr(skb)->h_dest, dev->net->dev_addr, ETH_ALEN);
-       return 1;
 }
 
 /* very simplistic detection of IPv4 or IPv6 headers */

Re: [PATCH] Fixup packets with incorrect ethertype sent by ZTE MF821D

[Bjørn Mork]

Jussi Peltola <plz@plz.fi> writes:

> So here's another stab. The comments and the current implementation are
> not in sync: any non-multicast address starting with a null octet gets
> rewritten, while the comment specifically mentions 00:a0:c6:00:00:00. It
> is certainly not elegant but re-writing all unicast destinations with
> our address does come to mind instead of special cases.

The known bug is related to 00:a0:c6:00:00:00 only.  But the workaround
catches anything starting with 00 for simplicity.  It's a deliberate
trade-off.  Could probably be clearer from the comments, yes.

> This patch fails to handle the invalid destinations in either way so I
> will send another one if you think it's worthwhile to go on. And it
> seems I forgot htons but I need this device for work now so a better
> patch must wait :)
>
> commit 35d3a46b7f1ece70e24386acbdd16af4507cb5f3
> Author: Jussi Peltola <plz@plz.fi>
> Date:   Mon Nov 14 01:45:32 2016 +0200
>
>     Attempt to fix up packets with a broken ethernet header
>     
>     Signed-off-by: Jussi Peltola <plz@plz.fi>
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 3ff76c6..7308d6b 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -153,25 +153,57 @@ static const u8 default_modem_addr[ETH_ALEN] = {0x02, 0x50, 0xf3};
>  
>  static const u8 buggy_fw_addr[ETH_ALEN] = {0x00, 0xa0, 0xc6, 0x00, 0x00, 0x00};
>  
> -/* Make up an ethernet header if the packet doesn't have one.
> +/* Check if the ethernet header has an unknown ethertype, and return a
> + * guess of the correct one based on the L3 header, or zero if the type was
> + * known or detection failed.
> + */
> +static __be16 detect_bogus_header(struct sk_buff *skb) {
> +       struct ethhdr *eth_hdr = (struct ethhdr*) skb->data;
> +
> +       switch (eth_hdr->h_proto) {
> +       case ETH_P_IP:
> +       case ETH_P_IPV6:
> +       case ETH_P_ARP:
> +               return 0;
> +       default:
> +               switch (skb->data[14] & 0xf0) {
> +               case 0x40:
> +                       return htons(ETH_P_IP);
> +               case 0x60:
> +                       return htons(ETH_P_IPV6);
> +               default:
> +                       /* pass on undetectable packets */
> +                       return 0;
> +               }
> +       }
> +       /*NOTREACHED*/
> +       return 0;
> +}
> +
> +/* Make up an ethernet header if the packet doesn't have a correct one.
>   *
>   * A firmware bug common among several devices cause them to send raw
>   * IP packets under some circumstances.  There is no way for the
>   * driver/host to know when this will happen.  And even when the bug
>   * hits, some packets will still arrive with an intact header.
>   *
> - * The supported devices are only capably of sending IPv4, IPv6 and
> + * The supported devices are only capable of sending IPv4, IPv6 and
>   * ARP packets on a point-to-point link. Any packet with an ethernet
>   * header will have either our address or a broadcast/multicast
> - * address as destination.  ARP packets will always have a header.
> + * address as destination. ARP packets will always have a header.
>   *
>   * This means that this function will reliably add the appropriate
> - * header iff necessary, provided our hardware address does not start
> + * header if necessary, provided our hardware address does not start
>   * with 4 or 6.
>   *
>   * Another common firmware bug results in all packets being addressed
>   * to 00:a0:c6:00:00:00 despite the host address being different.
> - * This function will also fixup such packets.
> + *
> + * Some devices will send packets with garbage source/destination MACs and
> + * ethertypes.
> + *
> + * This function will try to fix up all such packets.
> + *
>   */
>  static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  {
> @@ -179,8 +211,8 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>         bool rawip = info->flags & QMI_WWAN_FLAG_RAWIP;
>         __be16 proto;
>  
> -       /* This check is no longer done by usbnet */
> -       if (skb->len < dev->net->hard_header_len)
> +       /* Shorter is definitely invalid and breaks subsequent tests */
> +       if (skb->len < 15)
>                 return 0;
>  
>         switch (skb->data[0] & 0xf0) {


Makes sense, but could we please use some reasonable macro or something
instead of an arbitrary magic number?  Anything shorter than an IP
header will be bogus, for example.



> @@ -190,17 +222,17 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>         case 0x60:
>                 proto = htons(ETH_P_IPV6);
>                 break;
> -       case 0x00:
> +       default:
>                 if (rawip)
>                         return 0;
>                 if (is_multicast_ether_addr(skb->data))
>                         return 1;
> -               /* possibly bogus destination - rewrite just in case */
> -               skb_reset_mac_header(skb);
> -               goto fix_dest;
> -       default:
> -               if (rawip)
> -                       return 0;
> +               proto = detect_bogus_header(skb);
> +               if (proto) {
> +                       /* remove terminally broken header */
> +                       skb_pull(skb, ETH_HLEN);
> +                       break;
> +               }
>                 /* pass along other packets without modifications */
>                 return 1;
>         }


Am I missing somehting, or is this removing the bogus destination
address fixup?  In any case, I don't have any device with that bug
available or the time to go and test it.  So I want you to either leave
that part of the code alone, or verify the workaround on a device with
the "00:a0:c6:00:00:00" bug.


> @@ -208,17 +240,17 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>                 skb->dev = dev->net; /* normally set by eth_type_trans */
>                 skb->protocol = proto;
>                 return 1;
> +       } else {


Completely unnecessary "else". We return from the other branch.

In any case, this change is not related to the rest of the patch and is
just making review more confusing.  And why would you add more indenting
levels than strictly necessary?


But please: Try to make the device work with raw-ip first. Qualcomm
never managed to fix their fake ethernet mode, and have given up on it.
Newer devices don't have that mode at all.  The device you have does
still have the mode.  But as is pretty obvious from the observed
behaviour: It's completely untested and not really working.  If you look
at the Windows driver, I'm pretty sure you'll find that the only reason
it works is because they use raw-ip mode.

FWIW, I regret having chosen the fake ethernet mode as the default for
qmi_wwan.  I did not anticipate the number of firmware issues Qualcomm
could manage to create simply adding a made-up ethernet header.  And I
did not anticipate them finally dropping it on the floor, forcing us to
support raw-ip mode anyway.

But with raw-ip support, there is absolutely no reason to play around
with fixing up the ethernet header bugs anymore.  If you have an older
device where it sort of works, then by all means use it.  But otherwise:
Use raw-ip mode.  I don't think I would have wanted any of the existing
header fixups either if the driver had supported raw-ip when they were
proposed.


Bjørn