| Message ID | 20200802195046.402539-1-xie.he.0141@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v3] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len | expand |
On Sun, Aug 2, 2020 at 9:51 PM Xie He <xie.he.0141@gmail.com> wrote: > > 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 > ...... > > Additional change: > When sending, check skb->len to ensure the 1-byte pseudo header is > present before reading it. > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Xie He <xie.he.0141@gmail.com> Overall, looks great. A few nits. It's [PATCH net v3], not [net v3] > diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c > index b2868433718f..8a3f7ba36f7e 100644 > --- a/drivers/net/wan/lapbether.c > +++ b/drivers/net/wan/lapbether.c > @@ -157,6 +157,9 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff *skb, > if (!netif_running(dev)) > goto drop; > > + if (skb->len < 1) > + goto drop; > + Might be worth a comment along the lines of: /* upper layers pass a control byte. must validate pf_packet input */ > switch (skb->data[0]) { > case X25_IFACE_DATA: > break; > @@ -305,6 +308,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; Technically not needed. The struct is allocated with kvzalloc, z for __GFP_ZERO. Fine to leave if intended as self-describing comment, of course. > dev->mtu = 1000; > dev->addr_len = 0; > } > @@ -331,7 +335,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; > -- > 2.25.1 >
Thanks! On Mon, Aug 3, 2020 at 2:50 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > It's [PATCH net v3], not [net v3] Sorry. My mistake. I'll pay attention next time. I'm currently thinking about changing the subject to reflect that we added a "skb->len" check. Should I number the new patch as v1 or continue to number it as v4? > > + if (skb->len < 1) > > + goto drop; > > + > > Might be worth a comment along the lines of: /* upper layers pass a > control byte. must validate pf_packet input */ OK. I'll add the comment before it to make its meaning clearer. > > + dev->hard_header_len = 0; > > Technically not needed. The struct is allocated with kvzalloc, z for > __GFP_ZERO. Fine to leave if intended as self-describing comment, of > course. Thanks for pointing out! I think I can leave it as a self-describing comment. Thanks!
On 2020-08-02 21:50, Xie He wrote: > 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. I'm not an expert in the field, but after reading the commit message and the previous comments, I'd say that makes sense. > > 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 > ...... Shouldn't this kernel panic be intercepted by a skb_cow() before the skb_push() in lapbeth_data_transmit()? > > Additional change: > When sending, check skb->len to ensure the 1-byte pseudo header is > present before reading it. > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Xie He <xie.he.0141@gmail.com> > --- > > Change from v2: > Added skb->len check when sending. > > Change from v1: > None > > --- > drivers/net/wan/lapbether.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c > index b2868433718f..8a3f7ba36f7e 100644 > --- a/drivers/net/wan/lapbether.c > +++ b/drivers/net/wan/lapbether.c > @@ -157,6 +157,9 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff > *skb, > if (!netif_running(dev)) > goto drop; > > + if (skb->len < 1) > + goto drop; > + > switch (skb->data[0]) { > case X25_IFACE_DATA: > break; > @@ -305,6 +308,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 +335,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;
On Tue, Aug 4, 2020 at 5:43 AM Martin Schiller <ms@dev.tdt.de> wrote: > > I'm not an expert in the field, but after reading the commit message and > the previous comments, I'd say that makes sense. Thanks! > Shouldn't this kernel panic be intercepted by a skb_cow() before the > skb_push() in lapbeth_data_transmit()? When a skb is passing down a protocol stack for transmission, there might be several different skb_push calls to prepend different headers. It would be the best (in terms of performance) if we can allocate the needed header space in advance, so that we don't need to reallocate the skb every time a new header needs to be prepended. Adding skb_cow before these skb_push calls would indeed help preventing kernel panics, but that might not be the essential issue here, and it might also prevent us from discovering the real issue. (I guess this is also the reason skb_cow is not included in skb_push itself.)
On 2020-08-04 21:20, Xie He wrote: > On Tue, Aug 4, 2020 at 5:43 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> I'm not an expert in the field, but after reading the commit message >> and >> the previous comments, I'd say that makes sense. > > Thanks! > >> Shouldn't this kernel panic be intercepted by a skb_cow() before the >> skb_push() in lapbeth_data_transmit()? > > When a skb is passing down a protocol stack for transmission, there > might be several different skb_push calls to prepend different > headers. It would be the best (in terms of performance) if we can > allocate the needed header space in advance, so that we don't need to > reallocate the skb every time a new header needs to be prepended. Yes, I agree. > Adding skb_cow before these skb_push calls would indeed help > preventing kernel panics, but that might not be the essential issue > here, and it might also prevent us from discovering the real issue. (I > guess this is also the reason skb_cow is not included in skb_push > itself.) Well, you are right that the panic is "useful" to discover the real problem. But on the other hand, if it is possible to prevent a panic, I think we should do so. Maybe with adding a warning, when skb_cow() needs to reallocate memory. But this is getting a little bit off topic. For this patch I can say: LGTM. Reviewed-by: Martin Schiller <ms@dev.tdt.de>
On Tue, Aug 4, 2020 at 10:23 PM Martin Schiller <ms@dev.tdt.de> wrote: > > > Adding skb_cow before these skb_push calls would indeed help > > preventing kernel panics, but that might not be the essential issue > > here, and it might also prevent us from discovering the real issue. (I > > guess this is also the reason skb_cow is not included in skb_push > > itself.) > > Well, you are right that the panic is "useful" to discover the real > problem. But on the other hand, if it is possible to prevent a panic, I > think we should do so. Maybe with adding a warning, when skb_cow() needs > to reallocate memory. > > But this is getting a little bit off topic. For this patch I can say: > > LGTM. > > Reviewed-by: Martin Schiller <ms@dev.tdt.de> Thank you so much! Yes, it might be better to use skb_cow with a warning so that we can prevent kernel panic while still being able to discover the problem. If we want to do this, there are 2 more places in addition to lapbeth_data_transmit that need to be guarded with skb_cow: lapb_send_iframe and lapb_transmit_buffer in net/lapb/lapb_out.c. Maybe we can address this in a separate patch.
On Wed, Aug 5, 2020 at 10:57 AM Xie He <xie.he.0141@gmail.com> wrote: > > On Tue, Aug 4, 2020 at 10:23 PM Martin Schiller <ms@dev.tdt.de> wrote: > > > > > Adding skb_cow before these skb_push calls would indeed help > > > preventing kernel panics, but that might not be the essential issue > > > here, and it might also prevent us from discovering the real issue. (I > > > guess this is also the reason skb_cow is not included in skb_push > > > itself.) > > > > Well, you are right that the panic is "useful" to discover the real > > problem. But on the other hand, if it is possible to prevent a panic, I > > think we should do so. Maybe with adding a warning, when skb_cow() needs > > to reallocate memory. > > > > But this is getting a little bit off topic. For this patch I can say: > > > > LGTM. > > > > Reviewed-by: Martin Schiller <ms@dev.tdt.de> > > Thank you so much! > > Yes, it might be better to use skb_cow with a warning so that we can > prevent kernel panic while still being able to discover the problem. Let's not add defenses to work around possibly buggy code. In the long run that reduces code quality. Instead, fix bugs at the source.
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c index b2868433718f..8a3f7ba36f7e 100644 --- a/drivers/net/wan/lapbether.c +++ b/drivers/net/wan/lapbether.c @@ -157,6 +157,9 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff *skb, if (!netif_running(dev)) goto drop; + if (skb->len < 1) + goto drop; + switch (skb->data[0]) { case X25_IFACE_DATA: break; @@ -305,6 +308,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 +335,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;
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 ...... Additional change: When sending, check skb->len to ensure the 1-byte pseudo header is present before reading it. Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Cc: Brian Norris <briannorris@chromium.org> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- Change from v2: Added skb->len check when sending. Change from v1: None --- drivers/net/wan/lapbether.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)