netdev - Re: [PATCH 1/2] net: dsa: introduce STPID switch tagging handling code

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:	Wed, 21 Jul 2010 17:02:51 +0200
From:	Lennert Buytenhek <buytenh@...tstofly.org>
To:	Mike Frysinger <vapier@...too.org>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	uclinux-dist-devel@...ckfin.uclinux.org,
	Karl Beldan <karl.beldan@...il.com>,
	Graf Yang <graf.yang@...log.com>,
	Bryan Wu <cooloney@...nel.org>
Subject: Re: [PATCH 1/2] net: dsa: introduce STPID switch tagging handling code

On Wed, Jul 21, 2010 at 09:37:21AM -0400, Mike Frysinger wrote:

> diff --git a/net/dsa/tag_stpid.c b/net/dsa/tag_stpid.c
> new file mode 100644
> index 0000000..b5d9829
> --- /dev/null
> +++ b/net/dsa/tag_stpid.c
> @@ -0,0 +1,147 @@
> +/*
> + * net/dsa/tag_stpid.c - special tag identifier,
> + * 0x810 + 4 bit "port mask" + 3 bit 8021p + 1 bit CFI + 12 bit VLAN ID
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/etherdevice.h>
> +#include <linux/list.h>
> +#include <linux/netdevice.h>
> +#include "dsa_priv.h"
> +
> +#define ETH_P_8021QH      (ETH_P_8021Q >> 8)
> +#define ETH_P_8021QL      (ETH_P_8021Q & 0xFF)
> +#define STPID_HLEN        4
> +
> +#define ZERO_VID          0
> +#define RESERVED_VID      0xFFF
> +#define STPID_VID         ZERO_VID
> +
> +int stpid_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	u8 *dsa_header;
> +
> +	dev->stats.tx_packets++;
> +	dev->stats.tx_bytes += skb->len;

Everything up to this point looks OK, but..


> +	/*
> +	 * For 802.1Q frames, convert to STPID tagged frames,
> +	 * do nothing for common frames.
> +	 */
> +	if (skb->protocol == htons(ETH_P_8021Q)) {
> +		if (skb_cow_head(skb, 0) < 0)
> +			goto out_free;
> +
> +		dsa_header = skb->data + 2 * ETH_ALEN;
> +		dsa_header[1] = p->port & 0x03;
> +	}

This is almost certainly wrong -- according to the KSZ8893ML datasheet,
this means that VLAN tagged frames will be switched explicitly (by
sending them to p->port), but non-VLAN tagged frames will be switched
according to the switch's MAC address database.  You want explicit (i.e.
host kernel-side MAC address database lookup) switching in both cases.


> +{
> +	struct dsa_switch_tree *dst = dev->dsa_ptr;
> +	struct dsa_switch *ds = dst->ds[0];
> +	u8 *dsa_header;
> +	int source_port;
> +	int vid;
> +
> +	if (unlikely(ds == NULL))
> +		goto out_drop;
> +
> +	skb = skb_unshare(skb, GFP_ATOMIC);
> +	if (skb == NULL)
> +		goto out;
> +
> +	/* The ether_head has been pulled by master driver */
> +	dsa_header = skb->data - 2;
> +
> +	vid = ((dsa_header[2] & 0x0f)<<8 | dsa_header[3]);

Coding style (need spaces around '<<').


> +
> +	source_port = dsa_header[1] & 0x03;
> +	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
> +		goto out_drop;
> +
> +	if (((dsa_header[0] & ETH_P_8021QH) == ETH_P_8021QH) &&

This is bogus -- what it does is:

	if ((dsa_header[0] & 0x81) == 0x81)

It doesn't look like you need to mask here at all.
--
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