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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 23 Dec 2014 12:37:17 +0200
From:	Jouni Malinen <jkmalinen@...il.com>
To:	Willem de Bruijn <willemb@...gle.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	eric.dumazet@...il.com, dborkman@...hat.com
Subject: Re: [PATCH net-next v2] packet: make packet_snd fail on len smaller
 than l2 header

On Wed, Nov 19, 2014 at 8:10 PM, Willem de Bruijn <willemb@...gle.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html