| Message ID | 20200806015040.98379-1-xie.he.0141@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | drivers/net/wan/lapbether: Added needed_headroom and a skb->len check | expand |
I'm sorry I forgot to include the "net" prefix again. I remembered "PATCH" but not "net" this time. I'll try to remember both next time. If requested I can resend the patch with the correct prefix. Sorry.
On Thu, Aug 6, 2020 at 3:51 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. Changed to use needed_headroom instead of hard_header_len to request > necessary headroom to be allocated > > In net/packet/af_packet.c, the function packet_snd first reserves a > headroom of length (dev->hard_header_len + dev->needed_headroom). > Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header, > which calls dev->header_ops->create, to create the link layer header. > If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of > length (dev->hard_header_len), and assumes the user to provide the > appropriate link layer header. > > So according to the logic of af_packet.c, dev->hard_header_len should > be the length of the header that would be created by > dev->header_ops->create. > > However, this driver doesn't provide dev->header_ops, so logically > dev->hard_header_len should be 0. > > So we should use dev->needed_headroom instead of dev->hard_header_len > to request necessary headroom to be allocated. > > This change fixes kernel panic when this driver is used with AF_PACKET > SOCK_RAW sockets. > > Call stack when panic: > > [ 168.399197] skbuff: skb_under_panic: text:ffffffff819d95fb len:20 > put:14 head:ffff8882704c0a00 data:ffff8882704c09fd tail:0x11 end:0xc0 > dev:veth0 > ... > [ 168.399255] Call Trace: > [ 168.399259] skb_push.cold+0x14/0x24 > [ 168.399262] eth_header+0x2b/0xc0 > [ 168.399267] lapbeth_data_transmit+0x9a/0xb0 [lapbether] > [ 168.399275] lapb_data_transmit+0x22/0x2c [lapb] > [ 168.399277] lapb_transmit_buffer+0x71/0xb0 [lapb] > [ 168.399279] lapb_kick+0xe3/0x1c0 [lapb] > [ 168.399281] lapb_data_request+0x76/0xc0 [lapb] > [ 168.399283] lapbeth_xmit+0x56/0x90 [lapbether] > [ 168.399286] dev_hard_start_xmit+0x91/0x1f0 > [ 168.399289] ? irq_init_percpu_irqstack+0xc0/0x100 > [ 168.399291] __dev_queue_xmit+0x721/0x8e0 > [ 168.399295] ? packet_parse_headers.isra.0+0xd2/0x110 > [ 168.399297] dev_queue_xmit+0x10/0x20 > [ 168.399298] packet_sendmsg+0xbf0/0x19b0 > ...... > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Cc: Martin Schiller <ms@dev.tdt.de> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Xie He <xie.he.0141@gmail.com> Acked-by: Willem de Bruijn <willemb@google.com> The in-band signal byte is required, but stripped by lapbeth_xmit. Subsequent code will prefix additional headers, including an Ethernet link layer. The extra space needs to be reserved, but not pulled in packet_snd with skb_reserve, so has to use needed_headroom instead of hard_header_len.
On Thu, Aug 6, 2020 at 12:47 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Acked-by: Willem de Bruijn <willemb@google.com> > > The in-band signal byte is required, but stripped by lapbeth_xmit. > Subsequent code will prefix additional headers, including an Ethernet > link layer. The extra space needs to be reserved, but not pulled in > packet_snd with skb_reserve, so has to use needed_headroom instead of > hard_header_len. Thank you, Willem!
From: Xie He <xie.he.0141@gmail.com> Date: Wed, 5 Aug 2020 18:50:40 -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. Changed to use needed_headroom instead of hard_header_len to request > necessary headroom to be allocated > > In net/packet/af_packet.c, the function packet_snd first reserves a > headroom of length (dev->hard_header_len + dev->needed_headroom). > Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header, > which calls dev->header_ops->create, to create the link layer header. > If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of > length (dev->hard_header_len), and assumes the user to provide the > appropriate link layer header. > > So according to the logic of af_packet.c, dev->hard_header_len should > be the length of the header that would be created by > dev->header_ops->create. > > However, this driver doesn't provide dev->header_ops, so logically > dev->hard_header_len should be 0. > > So we should use dev->needed_headroom instead of dev->hard_header_len > to request necessary headroom to be allocated. > > This change fixes kernel panic when this driver is used with AF_PACKET > SOCK_RAW sockets. > > Call stack when panic: ... > Signed-off-by: Xie He <xie.he.0141@gmail.com> Applied and queued up for -stable, thanks.
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c index b2868433718f..1ea15f2123ed 100644 --- a/drivers/net/wan/lapbether.c +++ b/drivers/net/wan/lapbether.c @@ -157,6 +157,12 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff *skb, if (!netif_running(dev)) goto drop; + /* 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) + goto drop; + switch (skb->data[0]) { case X25_IFACE_DATA: break; @@ -305,6 +311,7 @@ static void lapbeth_setup(struct net_device *dev) dev->netdev_ops = &lapbeth_netdev_ops; dev->needs_free_netdev = true; dev->type = ARPHRD_X25; + dev->hard_header_len = 0; dev->mtu = 1000; dev->addr_len = 0; } @@ -331,7 +338,8 @@ static int lapbeth_new_device(struct net_device *dev) * then this driver prepends a length field of 2 bytes, * then the underlying Ethernet device prepends its own header. */ - ndev->hard_header_len = -1 + 3 + 2 + dev->hard_header_len; + ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len + + dev->needed_headroom; lapbeth = netdev_priv(ndev); lapbeth->axdev = ndev;
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. Changed to use needed_headroom instead of hard_header_len to request necessary headroom to be allocated In net/packet/af_packet.c, the function packet_snd first reserves a headroom of length (dev->hard_header_len + dev->needed_headroom). Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header, which calls dev->header_ops->create, to create the link layer header. If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of length (dev->hard_header_len), and assumes the user to provide the appropriate link layer header. So according to the logic of af_packet.c, dev->hard_header_len should be the length of the header that would be created by dev->header_ops->create. However, this driver doesn't provide dev->header_ops, so logically dev->hard_header_len should be 0. So we should use dev->needed_headroom instead of dev->hard_header_len to request necessary headroom to be allocated. This change fixes kernel panic when this driver is used with AF_PACKET SOCK_RAW sockets. Call stack when panic: [ 168.399197] skbuff: skb_under_panic: text:ffffffff819d95fb len:20 put:14 head:ffff8882704c0a00 data:ffff8882704c09fd tail:0x11 end:0xc0 dev:veth0 ... [ 168.399255] Call Trace: [ 168.399259] skb_push.cold+0x14/0x24 [ 168.399262] eth_header+0x2b/0xc0 [ 168.399267] lapbeth_data_transmit+0x9a/0xb0 [lapbether] [ 168.399275] lapb_data_transmit+0x22/0x2c [lapb] [ 168.399277] lapb_transmit_buffer+0x71/0xb0 [lapb] [ 168.399279] lapb_kick+0xe3/0x1c0 [lapb] [ 168.399281] lapb_data_request+0x76/0xc0 [lapb] [ 168.399283] lapbeth_xmit+0x56/0x90 [lapbether] [ 168.399286] dev_hard_start_xmit+0x91/0x1f0 [ 168.399289] ? irq_init_percpu_irqstack+0xc0/0x100 [ 168.399291] __dev_queue_xmit+0x721/0x8e0 [ 168.399295] ? packet_parse_headers.isra.0+0xd2/0x110 [ 168.399297] dev_queue_xmit+0x10/0x20 [ 168.399298] packet_sendmsg+0xbf0/0x19b0 ...... Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Cc: Martin Schiller <ms@dev.tdt.de> Cc: Brian Norris <briannorris@chromium.org> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- drivers/net/wan/lapbether.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)