Message ID | 1416420616-5029-1-git-send-email-willemb@google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2014-11-19 at 13:10 -0500, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > When sending packets out with PF_PACKET, SOCK_RAW, ensure that the > packet is at least as long as the device's expected link layer header. > This check already exists in tpacket_snd, but not in packet_snd. > Also rate limit the warning in tpacket_snd. > > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- Acked-by: Eric Dumazet <edumazet@google.com> Thanks Willem -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/19/2014 07:10 PM, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > When sending packets out with PF_PACKET, SOCK_RAW, ensure that the > packet is at least as long as the device's expected link layer header. > This check already exists in tpacket_snd, but not in packet_snd. > Also rate limit the warning in tpacket_snd. > > Signed-off-by: Willem de Bruijn <willemb@google.com> Looks good to me, thanks! Acked-by: Daniel Borkmann <dborkman@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Willem de Bruijn <willemb@google.com> Date: Wed, 19 Nov 2014 13:10:16 -0500 > From: Willem de Bruijn <willemb@google.com> > > When sending packets out with PF_PACKET, SOCK_RAW, ensure that the > packet is at least as long as the device's expected link layer header. > This check already exists in tpacket_snd, but not in packet_snd. > Also rate limit the warning in tpacket_snd. > > Signed-off-by: Willem de Bruijn <willemb@google.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 19, 2014 at 8:10 PM, Willem de Bruijn <willemb@google.com> wrote: > When sending packets out with PF_PACKET, SOCK_RAW, ensure that the > packet is at least as long as the device's expected link layer header. > This check already exists in tpacket_snd, but not in packet_snd. Was this supposed to be refusing zero-length payload following the header like the implementation does or accept zero-length payload like this commit message seems to imply? Based on the commit message, I'd assume 14 byte buffer on Ethernet netdev should have been accepted. I just noticed that once pulling this commit in into my automated test setup, one of the test cases started failing because of the change here. That test case was trying to transmit a minimum length Ethernet header using raw packet socket. Not that I care too much since I can easily change the test case to use one octet longer data to send and this was not really a real protocol case, but I wanted to confirm what was the expected behavior here since this commit seems to have number of inconsistent statements between the commit message, actual validation step, debug print, and code comment.. > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > @@ -2095,6 +2095,18 @@ static void tpacket_destruct_skb(struct sk_buff *skb) > +static bool ll_header_truncated(const struct net_device *dev, int len) > +{ > + /* net device doesn't like empty head */ I'm not sure how to interpret "empty head". Is that saying that the data following the header should not be empty? Or that header should be at least hard_header_len? > + if (unlikely(len <= dev->hard_header_len)) { That would be rejecting exactly hard_header_len, i.e,., I would have expected < instead of <= here based on the commit message. > + net_warn_ratelimited("%s: packet size is too short (%d < %d)\n", > + current->comm, len, dev->hard_header_len); But that debug print uses < instead of <= which is not consistent with the actual condition (and yes, I realize this was there even before this commit). - Jouni -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 23, 2014 at 11:37 AM, Jouni Malinen <jkmalinen@gmail.com> wrote: > On Wed, Nov 19, 2014 at 8:10 PM, Willem de Bruijn <willemb@google.com> wrote: >> When sending packets out with PF_PACKET, SOCK_RAW, ensure that the >> packet is at least as long as the device's expected link layer header. >> This check already exists in tpacket_snd, but not in packet_snd. > > Was this supposed to be refusing zero-length payload following the > header like the implementation does or accept zero-length payload like > this commit message seems to imply? Based on the commit message, I'd > assume 14 byte buffer on Ethernet netdev should have been accepted. Fair point. This patch just extended the existing check in tpacket_rcv to cover packet_rcv, so the code is the authoritative answer. But my commit message is inconsistent with the patch. > I just noticed that once pulling this commit in into my automated test > setup, one of the test cases started failing because of the change > here. That test case was trying to transmit a minimum length Ethernet > header using raw packet socket. Not that I care too much since I can > easily change the test case to use one octet longer data to send and > this was not really a real protocol case, but I wanted to confirm what > was the expected behavior here since this commit seems to have number > of inconsistent statements between the commit message, actual > validation step, debug print, and code comment.. > >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> @@ -2095,6 +2095,18 @@ static void tpacket_destruct_skb(struct sk_buff *skb) >> +static bool ll_header_truncated(const struct net_device *dev, int len) >> +{ >> + /* net device doesn't like empty head */ > > I'm not sure how to interpret "empty head". Is that saying that the > data following the header should not be empty? Or that header should > be at least hard_header_len? I read it as the first interpretation. I do not have an immediate example of stack or driver code that cannot handle Ethernet frames without payload, but also do not know of any legitimate use for sending such frames. Minimum frame length on the wire is even longer. The current test errs on the side of caution based on that existing use, then. > >> + if (unlikely(len <= dev->hard_header_len)) { > > That would be rejecting exactly hard_header_len, i.e,., I would have > expected < instead of <= here based on the commit message. > >> + net_warn_ratelimited("%s: packet size is too short (%d < %d)\n", >> + current->comm, len, dev->hard_header_len); > > But that debug print uses < instead of <= which is not consistent with > the actual condition (and yes, I realize this was there even before > this commit). Thanks, good point. When moving that code, I did not read it closely enough to notice the inconsistency. This looks sloppy; I can send a one line patch, unless you want to do it yourself. > - Jouni -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 4cd13d8..58af5802 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2095,6 +2095,18 @@ static void tpacket_destruct_skb(struct sk_buff *skb) sock_wfree(skb); } +static bool ll_header_truncated(const struct net_device *dev, int len) +{ + /* net device doesn't like empty head */ + if (unlikely(len <= dev->hard_header_len)) { + net_warn_ratelimited("%s: packet size is too short (%d < %d)\n", + current->comm, len, dev->hard_header_len); + return true; + } + + return false; +} + static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb, void *frame, struct net_device *dev, int size_max, __be16 proto, unsigned char *addr, int hlen) @@ -2170,12 +2182,8 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb, if (unlikely(err < 0)) return -EINVAL; } else if (dev->hard_header_len) { - /* net device doesn't like empty head */ - if (unlikely(tp_len <= dev->hard_header_len)) { - pr_err("packet size is too short (%d < %d)\n", - tp_len, dev->hard_header_len); + if (ll_header_truncated(dev, tp_len)) return -EINVAL; - } skb_push(skb, dev->hard_header_len); err = skb_store_bits(skb, 0, data, @@ -2500,9 +2508,14 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) skb_set_network_header(skb, reserve); err = -EINVAL; - if (sock->type == SOCK_DGRAM && - (offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len)) < 0) - goto out_free; + if (sock->type == SOCK_DGRAM) { + offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len); + if (unlikely(offset) < 0) + goto out_free; + } else { + if (ll_header_truncated(dev, len)) + goto out_free; + } /* Returns -EFAULT on error */ err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov, 0, len);