[PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[Willem de Bruijn]

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>
---
 net/packet/af_packet.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

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);
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[Eric Dumazet]

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

Re: [PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[Daniel Borkmann]

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>

Re: [PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[David Miller]

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.

Re: [PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[Jouni Malinen]

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

Re: [PATCH net-next v2] packet: make packet_snd fail on len smaller than l2 header

[Willem de Bruijn]

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