netdev - Re: [PATCH] macvlan: fix oops with vlan-on-top and HW_VLAN_CTAG

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]

Message-Id: <20131229.170121.154913581019152817.davem@davemloft.net>
Date:	Sun, 29 Dec 2013 17:01:21 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	fw@...len.de
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] macvlan: fix oops with vlan-on-top and HW_VLAN_CTAG_TX
 lowerdev

From: Florian Westphal <fw@...len.de>
Date: Sat, 28 Dec 2013 14:38:40 +0100

>  Or is the real bug the use of netdev_priv in macvlan_hard_header()?

That's a really good question.

On the surface it really looks like header ops cannot assume anything
about the netdev_priv().  These operations are really meant to be
stateless and apply to an entire class of devices.  macvlan is trying
to make it's operations specific to exactly one device.

For example, eth_header() only assumes that the device the packet is
going over has an ETH_ALEN length MAC address.

Or at least, based upon what the vlan is trying to do it appears this
way.

But, ironically, vlan_dev_hard_header() makes this same assumption.  It
uses netdev_priv() the same way macvlan is.

So the more I think about this, the more I think that the operations
assignment VLAN is doing is illegal.  If it wants to "pass through"
it must do so with explicit handlers, ones that pass the expected
device to the hard header ops.

So vlan_dev.c would have things like:

static int vlan_passthru_hard_header(struct sk_buff *skb, struct net_device *dev,
				     unsigned short type,
				     const void *daddr, const void *saddr,
				     unsigned int len)
{
	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
	struct net_device *real_dev = vlan->real_dev;

	return real_dev->header_ops->create(skb, real_dev, type, daddr, saddr, len);
}

static int vlan_passthru_rebuild_header(struct sk_buff *skb)
{
	struct net_device *dev = skb->dev;
	struct net_device *real_dev;
	struct vlan_dev_priv *vlan;

	vlan = vlan_dev_priv(dev);
	real_dev = vlan->real_dev;

	return real_dev->header_ops->rebuild(skb);
}

and then:

static const struct header_ops vlan_passthru_header_ops = {
	.create	 = vlan_passthru_hard_header,
	.rebuild = vlan_passthru_rebuild_header,
	.parse	 = eth_header_parse,
};

BTW, ARCNET is another case which cares about the netdev_priv() in it's
header_ops.

Can someone test this?

Signed-off-by: David S. Miller <davem@...emloft.net>

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 762896e..91eb467 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -530,6 +530,35 @@ static const struct header_ops vlan_header_ops = {
 	.parse	 = eth_header_parse,
 };
 
+static int vlan_passthru_hard_header(struct sk_buff *skb, struct net_device *dev,
+				     unsigned short type,
+				     const void *daddr, const void *saddr,
+				     unsigned int len)
+{
+	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
+	struct net_device *real_dev = vlan->real_dev;
+
+	return real_dev->header_ops->create(skb, real_dev, type, daddr, saddr, len);
+}
+
+static int vlan_passthru_rebuild_header(struct sk_buff *skb)
+{
+	struct net_device *dev = skb->dev;
+	struct net_device *real_dev;
+	struct vlan_dev_priv *vlan;
+
+	vlan = vlan_dev_priv(dev);
+	real_dev = vlan->real_dev;
+
+	return real_dev->header_ops->rebuild(skb);
+}
+
+static const struct header_ops vlan_passthru_header_ops = {
+	.create	 = vlan_passthru_hard_header,
+	.rebuild = vlan_passthru_rebuild_header,
+	.parse	 = eth_header_parse,
+};
+
 static struct device_type vlan_type = {
 	.name	= "vlan",
 };
@@ -573,7 +602,7 @@ static int vlan_dev_init(struct net_device *dev)
 
 	dev->needed_headroom = real_dev->needed_headroom;
 	if (real_dev->features & NETIF_F_HW_VLAN_CTAG_TX) {
-		dev->header_ops      = real_dev->header_ops;
+		dev->header_ops      = &vlan_passthru_header_ops;
 		dev->hard_header_len = real_dev->hard_header_len;
 	} else {
 		dev->header_ops      = &vlan_header_ops;
--
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