sending VLAN packets via packet_mmap

sending VLAN packets via packet_mmap

[Phil Sutter]

Hi,

support for VLAN tags in af_packet.c seems to be incomplete. While it's
possible to receive a full packet using SOCK_RAW, sending one will fail
due to size constraints. tpacket_snd() does not account for the
additional four bytes.

There are a few possible solutions to this problem. When searching for
the most appropriate one, I've been looking at tpacket_rcv() which
simply writes the whole frame out, setting tpacket2_hdr.tp_vlan_tci on
the go. So from a user's point of view, information is redundantly
available.

The actual problem in tpacket_snd() is this:

| reserve = dev->hard_header_len;
| [...]
| if (size_max > dev->mtu + reserve)
| 	size_max = dev->mtu + reserve;

I guess the check is there to avoid skb overflows on malicious data
input. Is this correct? Are there other reasons for it's existence?

As af_packet.c has no knowledge about VLANs (other than a call to
vlan_tx_tag_get()), I guess avoiding expensive parsing of the inserted
data for the VLAN tag should be appropriate. Nevertheless the check from
above needs to account for the additional VLAN_HLEN when the tag exists.

So a rather trivial solution would be to drop the check completely
(given no other constraints, of course), thereby giving the user a
little more ability to break things. Alternatively, one could require
that tpacket2_hdr.tp_vlan_tci be set (at least non-zero) to identify
packets containing a VLAN tag and allow the additional size (probably
mostly consistent to the logic inside tpacket_rcv()).

A third solution could be like the second one, but not accepting
prebuilt packets including VLAN header at all and using
tpacket2_hdr.tp_vlan_tci together with vlan_put_tag() to instead insert
it from inside the kernel.

Hopefully I didn't overlook something crucial. Feel free to flame me if
that's the case! :)

Greetings, Phil

Re: sending VLAN packets via packet_mmap

[David Miller]

From: Phil Sutter <phil@nwl.cc>
Date: Thu, 30 Sep 2010 21:24:14 +0200

> The actual problem in tpacket_snd() is this:
> 
> | reserve = dev->hard_header_len;
> | [...]
> | if (size_max > dev->mtu + reserve)
> | 	size_max = dev->mtu + reserve;
> 
> I guess the check is there to avoid skb overflows on malicious data
> input. Is this correct? Are there other reasons for it's existence?

We can add a special allowance of 4 extra bytes in this size check
_iff_ the device is ethernet and the NETIF_F_VLAN_CHALLENGED netdev
feature bit is not set.

Re: sending VLAN packets via packet_mmap

[Phil Sutter]

David,

On Wed, Oct 06, 2010 at 11:53:43PM -0700, David Miller wrote:
> From: Phil Sutter <phil@nwl.cc>
> Date: Thu, 30 Sep 2010 21:24:14 +0200
> 
> > The actual problem in tpacket_snd() is this:
> > 
> > | reserve = dev->hard_header_len;
> > | [...]
> > | if (size_max > dev->mtu + reserve)
> > | 	size_max = dev->mtu + reserve;
> > 
> > I guess the check is there to avoid skb overflows on malicious data
> > input. Is this correct? Are there other reasons for it's existence?
> 
> We can add a special allowance of 4 extra bytes in this size check
> _iff_ the device is ethernet and the NETIF_F_VLAN_CHALLENGED netdev
> feature bit is not set.

Making sure the device can actually handle the additional data, good
point!

Perhaps a bit philosophical, but what about NETIF_F_HW_VLAN_TX? AFAICT,
NICs providing that feature insert the VLAN header themselfs, correct?
Therefore __vlan_hwaccel_put_tag() just sets skb->vlan_tci. In order to
make use of that feature, one could always insert the VLAN header in the
kernel using vlan_put_tag(), depending on tpacket2_hdr.tp_vlan_tci
(probably in combination with VLAN_TAG_PRESENT to allow for VLAN ID 0).

Resumptively speaking, I'm probably talking about another feature
instead of solution for the bug in first place so I'll reply to this
mail with a patch implementing the suggested solution.

Greetings, Phil

[PATCH] af_packet: account for VLAN when checking packet size

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/packet/af_packet.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 9a17f28..ad37754 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -56,6 +56,7 @@
 #include <linux/in.h>
 #include <linux/inet.h>
 #include <linux/netdevice.h>
+#include <linux/if_arp.h>
 #include <linux/if_packet.h>
 #include <linux/wireless.h>
 #include <linux/kernel.h>
@@ -983,6 +984,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 
 	reserve = dev->hard_header_len;
 
+	/* allow VLAN packets when device supports them */
+	if (likely(dev->type == ARPHRD_ETHER) &&
+	    likely(!(dev->features & NETIF_F_VLAN_CHALLENGED)))
+		reserve += VLAN_HLEN;
+
 	err = -ENETDOWN;
 	if (unlikely(!(dev->flags & IFF_UP)))
 		goto out_put;
-- 
1.7.2.2

Re: [PATCH] af_packet: account for VLAN when checking packet size

[Eric Dumazet]

Le lundi 11 octobre 2010 à 15:25 +0200, Phil Sutter a écrit :
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/packet/af_packet.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 9a17f28..ad37754 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -56,6 +56,7 @@
>  #include <linux/in.h>
>  #include <linux/inet.h>
>  #include <linux/netdevice.h>
> +#include <linux/if_arp.h>
>  #include <linux/if_packet.h>
>  #include <linux/wireless.h>
>  #include <linux/kernel.h>
> @@ -983,6 +984,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  
>  	reserve = dev->hard_header_len;
>  
> +	/* allow VLAN packets when device supports them */
> +	if (likely(dev->type == ARPHRD_ETHER) &&
> +	    likely(!(dev->features & NETIF_F_VLAN_CHALLENGED)))
> +		reserve += VLAN_HLEN;
> +
>  	err = -ENETDOWN;
>  	if (unlikely(!(dev->flags & IFF_UP)))
>  		goto out_put;

If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes
frames for MTU=1500

Should we really care ?

If not, just do

reserve = dev->hard_header_len + VLAN_HLEN;

Re: [PATCH] af_packet: account for VLAN when checking packet size

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 16:03:02 +0200

> If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes
> frames for MTU=1500
> 
> Should we really care ?
> 
> If not, just do
> 
> reserve = dev->hard_header_len + VLAN_HLEN;

Thats a good point, I think we need to validate the SKB protocol
field.

Re: [PATCH] af_packet: account for VLAN when checking packet size

[Phil Sutter]

On Mon, Oct 11, 2010 at 09:01:53AM -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 11 Oct 2010 16:03:02 +0200
> 
> > If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes
> > frames for MTU=1500
> > 
> > Should we really care ?
> > 
> > If not, just do
> > 
> > reserve = dev->hard_header_len + VLAN_HLEN;
> 
> Thats a good point, I think we need to validate the SKB protocol
> field.

Which is set to the value of the passed struct sockaddr_ll field
sll_protocol. At least in the two userspace code samples I have here,
the later field is set to htons(ETH_P_ALL). So unless one changes the
API, the only way to find out the packet type is to actually parse the
given ethernet header.

Since tpacket_rcv() just interprets the vlan_tci skb field, such
detailed packet inspection is otherwise not done in af_packet.c.

Greetings, Phil

Re: [PATCH] af_packet: account for VLAN when checking packet size

[Michael S. Tsirkin]

On Mon, Oct 11, 2010 at 07:29:32PM +0200, Phil Sutter wrote:
> On Mon, Oct 11, 2010 at 09:01:53AM -0700, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 11 Oct 2010 16:03:02 +0200
> > 
> > > If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes
> > > frames for MTU=1500
> > > 
> > > Should we really care ?
> > > 
> > > If not, just do
> > > 
> > > reserve = dev->hard_header_len + VLAN_HLEN;
> > 
> > Thats a good point, I think we need to validate the SKB protocol
> > field.
> 
> Which is set to the value of the passed struct sockaddr_ll field
> sll_protocol. At least in the two userspace code samples I have here,
> the later field is set to htons(ETH_P_ALL). So unless one changes the
> API, the only way to find out the packet type is to actually parse the
> given ethernet header.

Yes, like eth_type_trans does I guess.  I think we had a similar
discussion already:

http://lists.openwall.net/netdev/2010/01/06/38

Summary: if we want to make the protocol field have the correct
value for this case we need to make it work for other
transports not just for ethernet.

> Since tpacket_rcv() just interprets the vlan_tci skb field, such
> detailed packet inspection is otherwise not done in af_packet.c.
> 
> Greetings, Phil
> --
> 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

Re: [PATCH] af_packet: account for VLAN when checking packet size

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 12 Oct 2010 19:19:41 +0200

> Yes, like eth_type_trans does I guess.  I think we had a similar
> discussion already:
> 
> http://lists.openwall.net/netdev/2010/01/06/38
> 
> Summary: if we want to make the protocol field have the correct
> value for this case we need to make it work for other
> transports not just for ethernet.

Right, so for now we should just allow 4-byte larger
than MTU TX packets, as long as the device is ethernet
and can handle VLANs properly.

Re: [PATCH] af_packet: account for VLAN when checking packet size

[Simon Horman]

On Tue, Oct 12, 2010 at 10:40:32AM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Tue, 12 Oct 2010 19:19:41 +0200
> 
> > Yes, like eth_type_trans does I guess.  I think we had a similar
> > discussion already:
> > 
> > http://lists.openwall.net/netdev/2010/01/06/38
> > 
> > Summary: if we want to make the protocol field have the correct
> > value for this case we need to make it work for other
> > transports not just for ethernet.
> 
> Right, so for now we should just allow 4-byte larger
> than MTU TX packets, as long as the device is ethernet
> and can handle VLANs properly.

Incidently, I believe that this problem will only become more acute
and complex if support for 802.1ad (Provider Bridges, aka Q-in-Q),
802.1ah (Provider Backbone Bridges, aka MAC-in-MAC) or other standards
which further extend the maximum frame size.

Dave, you were mentioning to me the other day that the kernel
already supports some notion of Q-in-Q (though its not 802.1ad).
Does the current implementation allow for frames > 1504 bytes?
Is that a complication to the change proposed here?

Re: [PATCH] af_packet: account for VLAN when checking packet size

[David Miller]

From: Simon Horman <horms@verge.net.au>
Date: Fri, 22 Oct 2010 10:41:26 +0200

> Incidently, I believe that this problem will only become more acute
> and complex if support for 802.1ad (Provider Bridges, aka Q-in-Q),
> 802.1ah (Provider Backbone Bridges, aka MAC-in-MAC) or other standards
> which further extend the maximum frame size.

No doubt.

> Dave, you were mentioning to me the other day that the kernel
> already supports some notion of Q-in-Q (though its not 802.1ad).
> Does the current implementation allow for frames > 1504 bytes?

It's only going to hardware offload and allow the extra space
for the outer-most VLAN tag.  Everthing inside of the outer
tag will be handled in software as far as Linux is concerned.

> Is that a complication to the change proposed here?

For now, I don't think so.