diff mbox series

[net-next,v4,5/5] net: hdlc_fr: Add support for any Ethertype

Message ID 20201030022839.438135-6-xie.he.0141@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand

Commit Message

Xie He Oct. 30, 2020, 2:28 a.m. UTC
Change the fr_rx function to make this driver support any Ethertype
when receiving skbs on normal (non-Ethernet-emulating) PVC devices.
(This driver is already able to handle any Ethertype when sending.)

Originally in the fr_rx function, the code that parses the long (10-byte)
header only recognizes a few Ethertype values and drops frames with other
Ethertype values. This patch replaces this code to make fr_rx support
any Ethertype. This patch also creates a new function fr_snap_parse as
part of the new code.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 75 +++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 26 deletions(-)

Comments

Willem de Bruijn Oct. 30, 2020, 4:32 p.m. UTC | #1
On Thu, Oct 29, 2020 at 10:32 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Change the fr_rx function to make this driver support any Ethertype
> when receiving skbs on normal (non-Ethernet-emulating) PVC devices.
> (This driver is already able to handle any Ethertype when sending.)
>
> Originally in the fr_rx function, the code that parses the long (10-byte)
> header only recognizes a few Ethertype values and drops frames with other
> Ethertype values. This patch replaces this code to make fr_rx support
> any Ethertype. This patch also creates a new function fr_snap_parse as
> part of the new code.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
>  drivers/net/wan/hdlc_fr.c | 75 +++++++++++++++++++++++++--------------
>  1 file changed, 49 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index 9a37575686b9..e95efc14bc97 100644
> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -871,6 +871,45 @@ static int fr_lmi_recv(struct net_device *dev, struct sk_buff *skb)
>         return 0;
>  }
>

>  static int fr_rx(struct sk_buff *skb)
>  {
> @@ -945,35 +984,19 @@ static int fr_rx(struct sk_buff *skb)
>                 skb->protocol = htons(ETH_P_IPV6);
>                 skb_reset_mac_header(skb);
>
> -       } else if (skb->len > 10 && data[3] == FR_PAD &&
> -                  data[4] == NLPID_SNAP && data[5] == FR_PAD) {
> -               u16 oui = ntohs(*(__be16*)(data + 6));
> -               u16 pid = ntohs(*(__be16*)(data + 8));
> -               skb_pull(skb, 10);
> -
> -               switch ((((u32)oui) << 16) | pid) {
> -               case ETH_P_ARP: /* routed frame with SNAP */
> -               case ETH_P_IPX:
> -               case ETH_P_IP:  /* a long variant */
> -               case ETH_P_IPV6:
> -                       if (!pvc->main)
> -                               goto rx_drop;
> -                       skb->dev = pvc->main;
> -                       skb->protocol = htons(pid);
> -                       skb_reset_mac_header(skb);
> -                       break;
> -
> -               case 0x80C20007: /* bridged Ethernet frame */
> -                       if (!pvc->ether)
> +       } else if (data[3] == FR_PAD) {
> +               if (skb->len < 5)
> +                       goto rx_error;
> +               if (data[4] == NLPID_SNAP) { /* A SNAP header follows */

Should this still check data[5] == FR_PAD?


> +                       skb_pull(skb, 5);
> +                       if (skb->len < 5) /* Incomplete SNAP header */
> +                               goto rx_error;
> +                       if (fr_snap_parse(skb, pvc))
>                                 goto rx_drop;
> -                       skb->protocol = eth_type_trans(skb, pvc->ether);
> -                       break;
> -
> -               default:
> -                       netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
> -                                   oui, pid);
> +               } else {
>                         goto rx_drop;
>                 }
> +
>         } else {
>                 netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
>                             data[3], skb->len);
> --
> 2.27.0
>
Xie He Oct. 30, 2020, 7:29 p.m. UTC | #2
On Fri, Oct 30, 2020 at 9:33 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Should this still check data[5] == FR_PAD?

No, the 6th byte (data[5]) is not a padding field. It is the first
byte of the SNAP header. The original code is misleading. That is part
of the reasons why I want to fix it with this patch.

The frame format is specified in RFC 2427
(https://tools.ietf.org/html/rfc2427). We can see in Section 4.1 and
4.2 that the 6th byte is the first byte of the SNAP header.
Willem de Bruijn Oct. 30, 2020, 9:25 p.m. UTC | #3
On Fri, Oct 30, 2020 at 3:29 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 9:33 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Should this still check data[5] == FR_PAD?
>
> No, the 6th byte (data[5]) is not a padding field. It is the first
> byte of the SNAP header. The original code is misleading. That is part
> of the reasons why I want to fix it with this patch.

Oh, good point. In that case

Acked-by: Willem de Bruijn <willemb@google.com>

> The frame format is specified in RFC 2427
> (https://tools.ietf.org/html/rfc2427). We can see in Section 4.1 and
> 4.2 that the 6th byte is the first byte of the SNAP header.
diff mbox series

Patch

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 9a37575686b9..e95efc14bc97 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -871,6 +871,45 @@  static int fr_lmi_recv(struct net_device *dev, struct sk_buff *skb)
 	return 0;
 }
 
+static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
+{
+	/* OUI 00-00-00 indicates an Ethertype follows */
+	if (skb->data[0] == 0x00 &&
+	    skb->data[1] == 0x00 &&
+	    skb->data[2] == 0x00) {
+		if (!pvc->main)
+			return -1;
+		skb->dev = pvc->main;
+		skb->protocol = *(__be16 *)(skb->data + 3); /* Ethertype */
+		skb_pull(skb, 5);
+		skb_reset_mac_header(skb);
+		return 0;
+
+	/* OUI 00-80-C2 stands for the 802.1 organization */
+	} else if (skb->data[0] == 0x00 &&
+		   skb->data[1] == 0x80 &&
+		   skb->data[2] == 0xC2) {
+		/* PID 00-07 stands for Ethernet frames without FCS */
+		if (skb->data[3] == 0x00 &&
+		    skb->data[4] == 0x07) {
+			if (!pvc->ether)
+				return -1;
+			skb_pull(skb, 5);
+			if (skb->len < ETH_HLEN)
+				return -1;
+			skb->protocol = eth_type_trans(skb, pvc->ether);
+			return 0;
+
+		/* PID unsupported */
+		} else {
+			return -1;
+		}
+
+	/* OUI unsupported */
+	} else {
+		return -1;
+	}
+}
 
 static int fr_rx(struct sk_buff *skb)
 {
@@ -945,35 +984,19 @@  static int fr_rx(struct sk_buff *skb)
 		skb->protocol = htons(ETH_P_IPV6);
 		skb_reset_mac_header(skb);
 
-	} else if (skb->len > 10 && data[3] == FR_PAD &&
-		   data[4] == NLPID_SNAP && data[5] == FR_PAD) {
-		u16 oui = ntohs(*(__be16*)(data + 6));
-		u16 pid = ntohs(*(__be16*)(data + 8));
-		skb_pull(skb, 10);
-
-		switch ((((u32)oui) << 16) | pid) {
-		case ETH_P_ARP: /* routed frame with SNAP */
-		case ETH_P_IPX:
-		case ETH_P_IP:	/* a long variant */
-		case ETH_P_IPV6:
-			if (!pvc->main)
-				goto rx_drop;
-			skb->dev = pvc->main;
-			skb->protocol = htons(pid);
-			skb_reset_mac_header(skb);
-			break;
-
-		case 0x80C20007: /* bridged Ethernet frame */
-			if (!pvc->ether)
+	} else if (data[3] == FR_PAD) {
+		if (skb->len < 5)
+			goto rx_error;
+		if (data[4] == NLPID_SNAP) { /* A SNAP header follows */
+			skb_pull(skb, 5);
+			if (skb->len < 5) /* Incomplete SNAP header */
+				goto rx_error;
+			if (fr_snap_parse(skb, pvc))
 				goto rx_drop;
-			skb->protocol = eth_type_trans(skb, pvc->ether);
-			break;
-
-		default:
-			netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
-				    oui, pid);
+		} else {
 			goto rx_drop;
 		}
+
 	} else {
 		netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
 			    data[3], skb->len);