[PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Xie He]

1. Added a skb->len check

This driver expects upper layers to include a pseudo header of 1 byte
when passing down a skb for transmission. This driver will read this
1-byte header. This patch added a skb->len check before reading the
header to make sure the header exists.

2. Added needed_headroom

When this driver transmits data,
  first this driver will remove a pseudo header of 1 byte,
  then the lapb module will prepend the LAPB header of 2 or 3 bytes.
So the value of needed_headroom in this driver should be 3 - 1.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Martin Schiller <ms@dev.tdt.de>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/x25_asy.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
index 84640a0c13f3..de7984463595 100644
--- a/drivers/net/wan/x25_asy.c
+++ b/drivers/net/wan/x25_asy.c
@@ -307,6 +307,14 @@ static netdev_tx_t x25_asy_xmit(struct sk_buff *skb,
 		return NETDEV_TX_OK;
 	}
 
+	/* There should be a pseudo header of 1 byte added by upper layers.
+	 * Check to make sure it is there before reading it.
+	 */
+	if (skb->len < 1) {
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
 	switch (skb->data[0]) {
 	case X25_IFACE_DATA:
 		break;
@@ -752,6 +760,12 @@ static void x25_asy_setup(struct net_device *dev)
 	dev->type		= ARPHRD_X25;
 	dev->tx_queue_len	= 10;
 
+	/* When transmitting data:
+	 * first this driver removes a pseudo header of 1 byte,
+	 * then the lapb module prepends an LAPB header of at most 3 bytes.
+	 */
+	dev->needed_headroom	= 3 - 1;
+
 	/* New-style flags. */
 	dev->flags		= IFF_NOARP;
 }
-- 
2.25.1

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Willem de Bruijn]

On Sun, Aug 9, 2020 at 4:36 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1. Added a skb->len check
>
> This driver expects upper layers to include a pseudo header of 1 byte
> when passing down a skb for transmission. This driver will read this
> 1-byte header. This patch added a skb->len check before reading the
> header to make sure the header exists.
>
> 2. Added needed_headroom
>
> When this driver transmits data,
>   first this driver will remove a pseudo header of 1 byte,
>   then the lapb module will prepend the LAPB header of 2 or 3 bytes.
> So the value of needed_headroom in this driver should be 3 - 1.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

The patch is analogous to commit c7ca03c216ac
("drivers/net/wan/lapbether: Added needed_headroom and a skb->len
check").

Seems to make sense based on call stack

  x25_asy_xmit               // skb_pull(skb, 1)
  lapb_data_request
  lapb_kick
  lapb_send_iframe        // skb_push(skb, 2)
  lapb_transmit_buffer    // skb_push(skb, 1)
  lapb_data_transmit
  x25_asy_data_transmit
  x25_asy_encaps

But I frankly don't know this code and would not modify logic that no
one has complained about for many years without evidence of a real
bug.

Were you able to actually exercise this path, similar to lapb_ether:
configure the device, send data from a packet socket? If so, can you
share the configuration steps?

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Xie He]

On Sun, Aug 9, 2020 at 2:13 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> The patch is analogous to commit c7ca03c216ac
> ("drivers/net/wan/lapbether: Added needed_headroom and a skb->len
> check").
>
> Seems to make sense based on call stack
>
>   x25_asy_xmit               // skb_pull(skb, 1)
>   lapb_data_request
>   lapb_kick
>   lapb_send_iframe        // skb_push(skb, 2)
>   lapb_transmit_buffer    // skb_push(skb, 1)
>   lapb_data_transmit
>   x25_asy_data_transmit
>   x25_asy_encaps

Thank you!

> But I frankly don't know this code and would not modify logic that no
> one has complained about for many years without evidence of a real
> bug.

Maybe it's better to submit this patch to "net-next"? I want to do
this change because:

1) I hope to set needed_headroom properly for all three X.25 drivers
(lapbether, x25_asy, hdlc_x25) in the kernel. So that the upper layer
(net/x25) can be changed to use needed_headroom to allocate skb,
instead of the current way of using a constant to estimate the needed
headroom.

2) The code quality of this driver is actually very low, and I also
hope to improve it gradually. Actually this driver had been completely
broken for many years and no one had noticed this until I fixed it in
commit 8fdcabeac398 (drivers/net/wan/x25_asy: Fix to make it work)
last month. This driver has a lot of other issues and I wish I can
gradually fix them, too.

> Were you able to actually exercise this path, similar to lapb_ether:
> configure the device, send data from a packet socket? If so, can you
> share the configuration steps?

Yes, I can run this driver. The driver is a software driver that runs
over TTY links. We can set up a x25_asy link over a virtual TTY link
using this method:

First:
  sudo modprobe lapb
  sudo modprobe x25_asy

Then set up a virtual TTY link:
  socat -d -d pty,cfmakeraw pty,cfmakeraw &
This will open a pair of PTY ports.
(The "socat" program can be installed from package managers.)

Then use a C program to set the line discipline for the two PTY ports:
Simplified version for reading:
  int ldisc = N_X25;
  int fd = open("path/to/pty", O_RDWR);
  ioctl(fd, TIOCSETD, &ldisc);
  close(fd);
Complete version for running:
  https://github.com/hyanggi/testing_linux/blob/master/network_x25/lapb/set_ldisc.c
Then we'll get two network interfaces named x25asy0 and x25asy1.

Then we do:
  sudo ip link set x25asyN up
to bring them up.

After we set up this x25_asy link, we can test it using AF_PACKET sockets:

In the connected-side C program:
Complete version for running:
  https://github.com/hyanggi/testing_linux/blob/master/network_x25/lapb/receiver.c
Simplified version for reading:
  int sockfd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_ALL));

  /* Get interface index */
  struct ifreq ifr;
  strcpy(ifr.ifr_name, "interface_name");
  ioctl(sockfd, SIOCGIFINDEX, &ifr);
  int ifindex = ifr.ifr_ifindex;

  struct sockaddr_ll sender_addr;
  socklen_t sender_addr_len = sizeof sender_addr;
  char buffer[1500];

  while (1) {
      ssize_t length = recvfrom(sockfd, buffer, sizeof buffer, 0,
                                (struct sockaddr *)&sender_addr,
                                &sender_addr_len);
      if (sender_addr.sll_ifindex != ifindex)
          continue;
      else if (buffer[0] == 0)
          printf("Data received.\n");
      else if (buffer[0] == 1)
          printf("Connected by the other side.\n");
      else if (buffer[0] == 2) {
          printf("Disconnected by the other side.\n");
          break;
      }
  }

  close(sockfd);

In the connecting-side C program:
Complete version for running:
  https://github.com/hyanggi/testing_linux/blob/master/network_x25/lapb/sender.c
Simplified version for reading:
  int sockfd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_ALL));

  /* Get interface index */
  struct ifreq ifr;
  strcpy(ifr.ifr_name, "interface_name");
  ioctl(sockfd, SIOCGIFINDEX, &ifr);
  int ifindex = ifr.ifr_ifindex;

  struct sockaddr_ll addr = {
      .sll_family = AF_PACKET,
      .sll_ifindex = ifindex,
  };

  /* Connect */
  sendto(sockfd, "\x01", 1, 0, (struct sockaddr *)&addr, sizeof addr);

  /* Send data */
  sendto(sockfd, "\x00" "data", 5, 0, (struct sockaddr *)&addr,
         sizeof addr);

  sleep(1); /* Wait a while before disconnecting */

  /* Disconnect */
  sendto(sockfd, "\x02", 1, 0, (struct sockaddr *)&addr, sizeof addr);

  close(sockfd);

I'm happy to answer any questions. Thank you so much!

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Willem de Bruijn]

On Sun, Aug 9, 2020 at 8:08 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sun, Aug 9, 2020 at 2:13 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > The patch is analogous to commit c7ca03c216ac
> > ("drivers/net/wan/lapbether: Added needed_headroom and a skb->len
> > check").

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

> >
> > Seems to make sense based on call stack
> >
> >   x25_asy_xmit               // skb_pull(skb, 1)
> >   lapb_data_request
> >   lapb_kick
> >   lapb_send_iframe        // skb_push(skb, 2)
> >   lapb_transmit_buffer    // skb_push(skb, 1)
> >   lapb_data_transmit
> >   x25_asy_data_transmit
> >   x25_asy_encaps
>
> Thank you!
>
> > But I frankly don't know this code and would not modify logic that no
> > one has complained about for many years without evidence of a real
> > bug.
>
> Maybe it's better to submit this patch to "net-next"?

That depends on whether this solves a bug. If it is possible to send a
0 byte packet and make ndo_start_xmit read garbage, then net is the
right target.

> I want to do this change because:
>
> 1) I hope to set needed_headroom properly for all three X.25 drivers
> (lapbether, x25_asy, hdlc_x25) in the kernel. So that the upper layer
> (net/x25) can be changed to use needed_headroom to allocate skb,
> instead of the current way of using a constant to estimate the needed
> headroom.

Which constant, X25_MAX_L2_LEN?

> 2) The code quality of this driver is actually very low, and I also
> hope to improve it gradually. Actually this driver had been completely
> broken for many years and no one had noticed this until I fixed it in
> commit 8fdcabeac398 (drivers/net/wan/x25_asy: Fix to make it work)
> last month.

Just curious: how come that netif_rx could be removed?

> This driver has a lot of other issues and I wish I can
> gradually fix them, too.
>
> > Were you able to actually exercise this path, similar to lapb_ether:
> > configure the device, send data from a packet socket? If so, can you
> > share the configuration steps?
>
> Yes, I can run this driver. The driver is a software driver that runs
> over TTY links. We can set up a x25_asy link over a virtual TTY link
> using this method:
>
> First:
>   sudo modprobe lapb
>   sudo modprobe x25_asy
>
> Then set up a virtual TTY link:
>   socat -d -d pty,cfmakeraw pty,cfmakeraw &
> This will open a pair of PTY ports.
> (The "socat" program can be installed from package managers.)
>
> Then use a C program to set the line discipline for the two PTY ports:
> Simplified version for reading:
>   int ldisc = N_X25;
>   int fd = open("path/to/pty", O_RDWR);
>   ioctl(fd, TIOCSETD, &ldisc);
>   close(fd);
> Complete version for running:
>   https://github.com/hyanggi/testing_linux/blob/master/network_x25/lapb/set_ldisc.c
> Then we'll get two network interfaces named x25asy0 and x25asy1.
>
> Then we do:
>   sudo ip link set x25asyN up
> to bring them up.
>
> After we set up this x25_asy link, we can test it using AF_PACKET sockets:
>
> In the connected-side C program:
> Complete version for running:
>   https://github.com/hyanggi/testing_linux/blob/master/network_x25/lapb/receiver.c
> Simplified version for reading:
>   int sockfd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_ALL));
>
>   /* Get interface index */
>   struct ifreq ifr;
>   strcpy(ifr.ifr_name, "interface_name");
>   ioctl(sockfd, SIOCGIFINDEX, &ifr);
>   int ifindex = ifr.ifr_ifindex;
>
>   struct sockaddr_ll sender_addr;
>   socklen_t sender_addr_len = sizeof sender_addr;
>   char buffer[1500];
>
>   while (1) {
>       ssize_t length = recvfrom(sockfd, buffer, sizeof buffer, 0,
>                                 (struct sockaddr *)&sender_addr,
>                                 &sender_addr_len);
>       if (sender_addr.sll_ifindex != ifindex)
>           continue;
>       else if (buffer[0] == 0)
>           printf("Data received.\n");
>       else if (buffer[0] == 1)
>           printf("Connected by the other side.\n");
>       else if (buffer[0] == 2) {
>           printf("Disconnected by the other side.\n");
>           break;
>       }
>   }
>
>   close(sockfd);
>
> In the connecting-side C program:
> Complete version for running:
>   https://github.com/hyanggi/testing_linux/blob/master/network_x25/lapb/sender.c
> Simplified version for reading:
>   int sockfd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_ALL));
>
>   /* Get interface index */
>   struct ifreq ifr;
>   strcpy(ifr.ifr_name, "interface_name");
>   ioctl(sockfd, SIOCGIFINDEX, &ifr);
>   int ifindex = ifr.ifr_ifindex;
>
>   struct sockaddr_ll addr = {
>       .sll_family = AF_PACKET,
>       .sll_ifindex = ifindex,
>   };
>
>   /* Connect */
>   sendto(sockfd, "\x01", 1, 0, (struct sockaddr *)&addr, sizeof addr);
>
>   /* Send data */
>   sendto(sockfd, "\x00" "data", 5, 0, (struct sockaddr *)&addr,
>          sizeof addr);
>
>   sleep(1); /* Wait a while before disconnecting */
>
>   /* Disconnect */
>   sendto(sockfd, "\x02", 1, 0, (struct sockaddr *)&addr, sizeof addr);
>
>   close(sockfd);
>
> I'm happy to answer any questions. Thank you so much!

Thanks very much for the detailed reproducer.

One thing to keep in mind is that AF_PACKET sockets are not the normal
datapath. AF_X25 sockets are. But you mention that you also exercise
the upper layer? That gives confidence that these changes are not
accidentally introducing regressions for the default path while fixing
oddly crafted packets with (root only for a reason) packet sockets.

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Xie He]

On Mon, Aug 10, 2020 at 12:21 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Acked-by: Willem de Bruijn <willemb@google.com>

Thank you so much!

> > 1) I hope to set needed_headroom properly for all three X.25 drivers
> > (lapbether, x25_asy, hdlc_x25) in the kernel. So that the upper layer
> > (net/x25) can be changed to use needed_headroom to allocate skb,
> > instead of the current way of using a constant to estimate the needed
> > headroom.
>
> Which constant, X25_MAX_L2_LEN?

Yes, by grepping X25_MAX_L2_LEN in net/x25, I can see it is used in
various places to allocate and reserve the needed header space. For
example in net/x25/af_x25.c, the function x25_sendmsg allocates and
reserves a header space of X25_MAX_L2_LEN + X25_EXT_MIN_LEN.

> > 2) The code quality of this driver is actually very low, and I also
> > hope to improve it gradually. Actually this driver had been completely
> > broken for many years and no one had noticed this until I fixed it in
> > commit 8fdcabeac398 (drivers/net/wan/x25_asy: Fix to make it work)
> > last month.
>
> Just curious: how come that netif_rx could be removed?

When receiving data, the driver should only submit skb to upper layers
after it has been processed by the lapb module, i.e., it should only
call netif_rx in the function x25_asy_data_indication. The removed
netif_rx is in the function x25_asy_bump. This function is responsible
for passing the skb to the lapb module to process. It doesn't make
sense to call netif_rx here. If we call netif_rx here, we may pass
control frames that shouldn't be passed to upper layers (and have been
consumed and freed by the lapb module) to upper layers.

> One thing to keep in mind is that AF_PACKET sockets are not the normal
> datapath. AF_X25 sockets are. But you mention that you also exercise
> the upper layer? That gives confidence that these changes are not
> accidentally introducing regressions for the default path while fixing
> oddly crafted packets with (root only for a reason) packet sockets.

Yes, I test with AF_X25 sockets too to make sure the changes are OK.
I usually test AF_X25 sockets with:
https://github.com/hyanggi/testing_linux/blob/master/network_x25/x25/server.c
https://github.com/hyanggi/testing_linux/blob/master/network_x25/x25/client.c

I became interested in X.25 when I was trying different address
families that Linux supported. I tried AF_X25 sockets. And then I
tried to use the X.25 link layer directly through AF_PACKET. I believe
both AF_X25 sockets and AF_PACKET sockets need to work without
problems with X.25 drivers - lapbether and x25_asy. There is another
X.25 driver (hdlc_x25) in the kernel. I haven't been able to run that
driver. But that driver seems to be the real driver which is really
used, and I know Martin Schiller <ms@dev.tdt.de> is an active user and
developer of that driver.

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Willem de Bruijn]

> > > 2) The code quality of this driver is actually very low, and I also
> > > hope to improve it gradually. Actually this driver had been completely
> > > broken for many years and no one had noticed this until I fixed it in
> > > commit 8fdcabeac398 (drivers/net/wan/x25_asy: Fix to make it work)
> > > last month.
> >
> > Just curious: how come that netif_rx could be removed?
>
> When receiving data, the driver should only submit skb to upper layers
> after it has been processed by the lapb module, i.e., it should only
> call netif_rx in the function x25_asy_data_indication. The removed
> netif_rx is in the function x25_asy_bump. This function is responsible
> for passing the skb to the lapb module to process. It doesn't make
> sense to call netif_rx here. If we call netif_rx here, we may pass
> control frames that shouldn't be passed to upper layers (and have been
> consumed and freed by the lapb module) to upper layers.

Ah of course. Thanks for explaining.

> > One thing to keep in mind is that AF_PACKET sockets are not the normal
> > datapath. AF_X25 sockets are. But you mention that you also exercise
> > the upper layer? That gives confidence that these changes are not
> > accidentally introducing regressions for the default path while fixing
> > oddly crafted packets with (root only for a reason) packet sockets.
>
> Yes, I test with AF_X25 sockets too to make sure the changes are OK.
> I usually test AF_X25 sockets with:
> https://github.com/hyanggi/testing_linux/blob/master/network_x25/x25/server.c
> https://github.com/hyanggi/testing_linux/blob/master/network_x25/x25/client.c

Excellent. Thanks for the link. Good to know that these changes are
getting real code coverage.

> I became interested in X.25 when I was trying different address
> families that Linux supported. I tried AF_X25 sockets. And then I
> tried to use the X.25 link layer directly through AF_PACKET. I believe
> both AF_X25 sockets and AF_PACKET sockets need to work without
> problems with X.25 drivers - lapbether and x25_asy. There is another
> X.25 driver (hdlc_x25) in the kernel. I haven't been able to run that
> driver. But that driver seems to be the real driver which is really
> used, and I know Martin Schiller <ms@dev.tdt.de> is an active user and
> developer of that driver.

Great, sounds like we might have additional LAPB and X25 maintainers soon? :)

MAINTAINERS lists Andrew Hendry as maintainer for X.25. Please do CC them.

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Xie He]

On Tue, Aug 11, 2020 at 3:50 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> > I became interested in X.25 when I was trying different address
> > families that Linux supported. I tried AF_X25 sockets. And then I
> > tried to use the X.25 link layer directly through AF_PACKET. I believe
> > both AF_X25 sockets and AF_PACKET sockets need to work without
> > problems with X.25 drivers - lapbether and x25_asy. There is another
> > X.25 driver (hdlc_x25) in the kernel. I haven't been able to run that
> > driver. But that driver seems to be the real driver which is really
> > used, and I know Martin Schiller <ms@dev.tdt.de> is an active user and
> > developer of that driver.
>
> Great, sounds like we might have additional LAPB and X25 maintainers soon? :)

:)  I just want to fix any problems I see. I'll do this whenever I have time.

> MAINTAINERS lists Andrew Hendry as maintainer for X.25. Please do CC them.

OK. I'll surely do that. Thanks!

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[David Miller]

From: Xie He <xie.he.0141@gmail.com>
Date: Sat,  8 Aug 2020 19:35:48 -0700

> 1. Added a skb->len check
> 
> This driver expects upper layers to include a pseudo header of 1 byte
> when passing down a skb for transmission. This driver will read this
> 1-byte header. This patch added a skb->len check before reading the
> header to make sure the header exists.
> 
> 2. Added needed_headroom
> 
> When this driver transmits data,
>   first this driver will remove a pseudo header of 1 byte,
>   then the lapb module will prepend the LAPB header of 2 or 3 bytes.
> So the value of needed_headroom in this driver should be 3 - 1.
> 
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Applied, thank you.

Re: [PATCH net] drivers/net/wan/x25_asy: Added needed_headroom and a skb->len check

[Xie He]

On Tue, Aug 11, 2020 at 10:32 AM David Miller <davem@davemloft.net> wrote:
>
> Applied, thank you.

Thank you!