Weird problem with PPPoE on tap interface

Weird problem with PPPoE on tap interface

[Florian Zumbiehl]

Hi,

I'm experiencing a pretty strange problem with kernel PPPoE on tap
interfaces with a vanilla 2.6.20 kernel that prevents the PPP connection
from being established:

Every PPPoE session packet (that is, LCP, since it never gets to a stage
where any other session data is being exchanged) is delivered to pppd
twice. This can be seen from pppd's logging messages when debugging
is enabled, and strace confirmed that it indeed does read() the exact
same data twice in a row from the same file descriptor - even though
a tcpdump on the corresponding tap interface does show each packet only
once.

For confirming this, I used a program with a select() loop that simply
moves packets unchanged and without reordering back and forth between
a "real" ethernet interface in promiscuous mode and the tap interface
used by pppd.

What makes this even stranger, is, that the setup works perfectly (and
only a single copy of packets is delivered to pppd) if I simply replace
the tap interface in pppd's config with the "real" ethernet interface
that the tap interface was previously bridged to (it's an ISA NE2K
clone, BTW).

Finally, it also works perfectly when I use userspace rp-pppoe through
the tap interface.

So far, I also confirmed that in kernel space, the call to ppp_input()
in drivers/net/pppoe.c is executed as many times as pppd receives a
packet, so the problem must be somewhere before that.

Well, I'm gonna try to find out more - but if someone with some
more knowledge of the involved kernel code would be willing to help
with this in some way or another, that would certainly be
appreciated ;-) - if you do need any additional information, let me
know ...

Florian

Re: Weird problem with PPPoE on tap interface

[Florian Zumbiehl]

Hi,

> I'm experiencing a pretty strange problem with kernel PPPoE on tap
> interfaces with a vanilla 2.6.20 kernel that prevents the PPP connection
> from being established:
[...]

Well, I guess I have found the source of the problem: The PPPoE code seems
to match inbound packets to sockets based only on source address and
session ID - so in the given scenario, packets are delivered to pppd
once when they arrive at the "real" ethernet interface and a second time
when they arrive at the tap interface. Now, this is kindof two problems,
actually:

1. This is not in compliance with RFC 2516, which states in section 4:

   | The SESSION_ID field is sixteen bits.  It is an unsigned value in
   | network byte order.  It's value is defined below for Discovery
   | packets.  The value is fixed for a given PPP session and, in fact,
   | defines a PPP session along with the Ethernet SOURCE_ADDR and
   | DESTINATION_ADDR.

   So, it would be legal for there to be multiple sessions with the same
   peer using the same session ID, as long as they are using different
   local MAC addresses - but the current PPPoE code would not be able to
   distinguish those.

   This probably is not a problem for me, but should be fixed anyway,
   IMO. (I say "probably" since I am actually using different local MAC
   addresses for multiple PPPoE sessions to the same DSL-AC, as T-Com
   doesn't allow for multiple sessions from the same MAC address -
   so they would be allowed by the RFC to use the same session ID for
   more than one of those sessions, even though I doubt that they're
   doing that.)

2. In addition to session ID, source and destination address, IMO, the
   incoming interface should be added to the key that's used for
   matching packets to sockets. This is probably somewhat of a design
   decision much like the weak ES vs. strong ES issue with IP, but
   AFAICS, the userspace-API works by binding to an interface index, not
   by binding to a MAC address (unlike IP socket binding), which is why
   I'd expect from pppd's perspective that only packets from that
   interface are delivered.

   Plus, from the end user's perspective, this might even be somewhat of
   a security problem. I for one would (well, I obviuosly _did_ :-)
   expect that if I do specify an interface name in pppd's config,
   pppd won't get into contact with anything that's going on on
   interfaces other than the one specified. As the involved MAC addresses
   probably usually aren't considered a secret, all you'd have to guess
   in order to interfere with/inject packets into PPPoE sessions "on a
   different interface" is the 16-bit session identifier, which might not
   even be random.

Well, your opinions are welcome. Plus any hints as to how to fix this.
I'd tend to simply(?) add some more fields to the
{hash,get,set,delete}_item() functions in drivers/net/pppoe.c.
But maybe there is some better way?

Florian

[PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[Florian Zumbiehl]

Hi,

> Well, your opinions are welcome. Plus any hints as to how to fix this.
> I'd tend to simply(?) add some more fields to the
> {hash,get,set,delete}_item() functions in drivers/net/pppoe.c.
> But maybe there is some better way?

As noone seems to have an opinion on this: Here is a patch that does
work for me and that should solve the problem as far as that is easily
possible. It is based on the assumption that an interface's ifindex is
basically an alias for a local MAC address, so incoming packets now are
matched to sockets based on remote MAC, session id, and ifindex of the
interface the packet came in on/the socket was bound to by connect().

For relayed packets, the socket that's used for relaying is selected
based on destination MAC, session ID and the interface index of the
interface whose name currently matches the name requested by userspace
as the relaying source interface. The relaying part of the patch is
untested.

Please note that I'd consider this a security fix for reasons outlined
in previous mails.

Florian

--- linux-2.6.20/drivers/net/pppoe.c.orig	2007-02-25 19:23:51.000000000 +0100
+++ linux-2.6.20/drivers/net/pppoe.c	2007-02-28 12:56:05.000000000 +0100
@@ -7,6 +7,12 @@
  *
  * Version:	0.7.0
  *
+ * 070228 :	Fix to allow multiple sessions with same remote MAC and same
+ *		session id by including the local device ifindex in the
+ *		tuple identifying a session. This also ensures packets can't
+ *		be injected into a session from interfaces other than the one
+ *		specified by userspace. Florian Zumbiehl <florz@florz.de>
+ *		(Oh, BTW, this one is YYMMDD, in case you were wondering ...)
  * 220102 :	Fix module use count on failure in pppoe_create, pppox_sk -acme
  * 030700 :	Fixed connect logic to allow for disconnect.
  * 270700 :	Fixed potential SMP problems; we must protect against
@@ -127,14 +133,14 @@
  *  Set/get/delete/rehash items  (internal versions)
  *
  **********************************************************************/
-static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr)
+static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr, int ifindex)
 {
 	int hash = hash_item(sid, addr);
 	struct pppox_sock *ret;
 
 	ret = item_hash_table[hash];
 
-	while (ret && !cmp_addr(&ret->pppoe_pa, sid, addr))
+	while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_dev->ifindex == ifindex))
 		ret = ret->next;
 
 	return ret;
@@ -147,21 +153,19 @@
 
 	ret = item_hash_table[hash];
 	while (ret) {
-		if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa))
+		if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) && ret->pppoe_dev->ifindex == po->pppoe_dev->ifindex)
 			return -EALREADY;
 
 		ret = ret->next;
 	}
 
-	if (!ret) {
-		po->next = item_hash_table[hash];
-		item_hash_table[hash] = po;
-	}
+	po->next = item_hash_table[hash];
+	item_hash_table[hash] = po;
 
 	return 0;
 }
 
-static struct pppox_sock *__delete_item(unsigned long sid, char *addr)
+static struct pppox_sock *__delete_item(unsigned long sid, char *addr, int ifindex)
 {
 	int hash = hash_item(sid, addr);
 	struct pppox_sock *ret, **src;
@@ -170,7 +174,7 @@
 	src = &item_hash_table[hash];
 
 	while (ret) {
-		if (cmp_addr(&ret->pppoe_pa, sid, addr)) {
+		if (cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_dev->ifindex == ifindex) {
 			*src = ret->next;
 			break;
 		}
@@ -188,12 +192,12 @@
  *
  **********************************************************************/
 static inline struct pppox_sock *get_item(unsigned long sid,
-					 unsigned char *addr)
+					 unsigned char *addr, int ifindex)
 {
 	struct pppox_sock *po;
 
 	read_lock_bh(&pppoe_hash_lock);
-	po = __get_item(sid, addr);
+	po = __get_item(sid, addr, ifindex);
 	if (po)
 		sock_hold(sk_pppox(po));
 	read_unlock_bh(&pppoe_hash_lock);
@@ -203,7 +207,15 @@
 
 static inline struct pppox_sock *get_item_by_addr(struct sockaddr_pppox *sp)
 {
-	return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote);
+	struct net_device *dev = NULL;
+	int ifindex;
+
+	dev = dev_get_by_name(sp->sa_addr.pppoe.dev);
+	if(!dev)
+		return NULL;
+	ifindex = dev->ifindex;
+	dev_put(dev);
+	return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote, ifindex);
 }
 
 static inline int set_item(struct pppox_sock *po)
@@ -220,12 +232,12 @@
 	return i;
 }
 
-static inline struct pppox_sock *delete_item(unsigned long sid, char *addr)
+static inline struct pppox_sock *delete_item(unsigned long sid, char *addr, int ifindex)
 {
 	struct pppox_sock *ret;
 
 	write_lock_bh(&pppoe_hash_lock);
-	ret = __delete_item(sid, addr);
+	ret = __delete_item(sid, addr, ifindex);
 	write_unlock_bh(&pppoe_hash_lock);
 
 	return ret;
@@ -391,7 +403,7 @@
 
 	ph = (struct pppoe_hdr *) skb->nh.raw;
 
-	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source);
+	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source, dev->ifindex);
 	if (po != NULL)
 		return sk_receive_skb(sk_pppox(po), skb, 0);
 drop:
@@ -425,7 +437,7 @@
 	if (ph->code != PADT_CODE)
 		goto abort;
 
-	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source);
+	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source, dev->ifindex);
 	if (po) {
 		struct sock *sk = sk_pppox(po);
 
@@ -517,7 +529,7 @@
 
 	po = pppox_sk(sk);
 	if (po->pppoe_pa.sid) {
-		delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote);
+		delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote, po->pppoe_dev->ifindex);
 	}
 
 	if (po->pppoe_dev)
@@ -539,7 +551,7 @@
 		  int sockaddr_len, int flags)
 {
 	struct sock *sk = sock->sk;
-	struct net_device *dev = NULL;
+	struct net_device *dev;
 	struct sockaddr_pppox *sp = (struct sockaddr_pppox *) uservaddr;
 	struct pppox_sock *po = pppox_sk(sk);
 	int error;
@@ -565,7 +577,7 @@
 		pppox_unbind_sock(sk);
 
 		/* Delete the old binding */
-		delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote);
+		delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote,po->pppoe_dev->ifindex);
 
 		if(po->pppoe_dev)
 			dev_put(po->pppoe_dev);
@@ -705,7 +717,7 @@
 			break;
 
 		/* PPPoE address from the user specifies an outbound
-		   PPPoE address to which frames are forwarded to */
+		   PPPoE address which frames are forwarded to */
 		err = -EFAULT;
 		if (copy_from_user(&po->pppoe_relay,
 				   (void __user *)arg,

Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[David Miller]

From: Florian Zumbiehl <florz@gmx.de>
Date: Wed, 28 Feb 2007 13:38:44 +0100

> As noone seems to have an opinion on this: Here is a patch that does
> work for me and that should solve the problem as far as that is easily
> possible. It is based on the assumption that an interface's ifindex is
> basically an alias for a local MAC address, so incoming packets now are
> matched to sockets based on remote MAC, session id, and ifindex of the
> interface the packet came in on/the socket was bound to by connect().

I agree with your analysis and have applied your patch.

Another way to implement this would have been to store the
pre-computed ifindex on the kernel side sockaddr.

Thanks.

Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[Florian Zumbiehl]

Hi,

> From: Florian Zumbiehl <florz@gmx.de>
> Date: Wed, 28 Feb 2007 13:38:44 +0100
> 
> > As noone seems to have an opinion on this: Here is a patch that does
> > work for me and that should solve the problem as far as that is easily
> > possible. It is based on the assumption that an interface's ifindex is
> > basically an alias for a local MAC address, so incoming packets now are
> > matched to sockets based on remote MAC, session id, and ifindex of the
> > interface the packet came in on/the socket was bound to by connect().
> 
> I agree with your analysis and have applied your patch.

Below you find a slightly changed version of the patch that avoids
a possible NULL pointer dereference in case pppoe_device_event()/
pppoe_flush_dev() dev_put()s dev and sets it to NULL before pppoe_connect()
tries to unbind from the previous address, in which case it would
dereference the NULL pointer in dev.

It now saves the ifindex in the socket's data structure upon connect(),
so that it's still available for finding the entry to remove from the
hash table in case pppoe_device_event() should have dropped the socket's
reference to dev.

> Another way to implement this would have been to store the
> pre-computed ifindex on the kernel side sockaddr.

Well, that probably depends on the intended semantics. There isn't any
documentation somewhere that specifies what the intended behaviour is,
is there?!

Florian

--- linux-2.6.20/drivers/net/pppoe.c.orig	2007-02-25 19:23:51.000000000 +0100
+++ linux-2.6.20/drivers/net/pppoe.c	2007-03-04 02:11:51.000000000 +0100
@@ -7,6 +7,12 @@
  *
  * Version:	0.7.0
  *
+ * 070228 :	Fix to allow multiple sessions with same remote MAC and same
+ *		session id by including the local device ifindex in the
+ *		tuple identifying a session. This also ensures packets can't
+ *		be injected into a session from interfaces other than the one
+ *		specified by userspace. Florian Zumbiehl <florz@florz.de>
+ *		(Oh, BTW, this one is YYMMDD, in case you were wondering ...)
  * 220102 :	Fix module use count on failure in pppoe_create, pppox_sk -acme
  * 030700 :	Fixed connect logic to allow for disconnect.
  * 270700 :	Fixed potential SMP problems; we must protect against
@@ -127,14 +133,14 @@
  *  Set/get/delete/rehash items  (internal versions)
  *
  **********************************************************************/
-static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr)
+static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr, int ifindex)
 {
 	int hash = hash_item(sid, addr);
 	struct pppox_sock *ret;
 
 	ret = item_hash_table[hash];
 
-	while (ret && !cmp_addr(&ret->pppoe_pa, sid, addr))
+	while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_ifindex == ifindex))
 		ret = ret->next;
 
 	return ret;
@@ -147,21 +153,19 @@
 
 	ret = item_hash_table[hash];
 	while (ret) {
-		if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa))
+		if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) && ret->pppoe_ifindex == po->pppoe_ifindex)
 			return -EALREADY;
 
 		ret = ret->next;
 	}
 
-	if (!ret) {
-		po->next = item_hash_table[hash];
-		item_hash_table[hash] = po;
-	}
+	po->next = item_hash_table[hash];
+	item_hash_table[hash] = po;
 
 	return 0;
 }
 
-static struct pppox_sock *__delete_item(unsigned long sid, char *addr)
+static struct pppox_sock *__delete_item(unsigned long sid, char *addr, int ifindex)
 {
 	int hash = hash_item(sid, addr);
 	struct pppox_sock *ret, **src;
@@ -170,7 +174,7 @@
 	src = &item_hash_table[hash];
 
 	while (ret) {
-		if (cmp_addr(&ret->pppoe_pa, sid, addr)) {
+		if (cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_ifindex == ifindex) {
 			*src = ret->next;
 			break;
 		}
@@ -188,12 +192,12 @@
  *
  **********************************************************************/
 static inline struct pppox_sock *get_item(unsigned long sid,
-					 unsigned char *addr)
+					 unsigned char *addr, int ifindex)
 {
 	struct pppox_sock *po;
 
 	read_lock_bh(&pppoe_hash_lock);
-	po = __get_item(sid, addr);
+	po = __get_item(sid, addr, ifindex);
 	if (po)
 		sock_hold(sk_pppox(po));
 	read_unlock_bh(&pppoe_hash_lock);
@@ -203,7 +207,15 @@
 
 static inline struct pppox_sock *get_item_by_addr(struct sockaddr_pppox *sp)
 {
-	return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote);
+	struct net_device *dev = NULL;
+	int ifindex;
+
+	dev = dev_get_by_name(sp->sa_addr.pppoe.dev);
+	if(!dev)
+		return NULL;
+	ifindex = dev->ifindex;
+	dev_put(dev);
+	return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote, ifindex);
 }
 
 static inline int set_item(struct pppox_sock *po)
@@ -220,12 +232,12 @@
 	return i;
 }
 
-static inline struct pppox_sock *delete_item(unsigned long sid, char *addr)
+static inline struct pppox_sock *delete_item(unsigned long sid, char *addr, int ifindex)
 {
 	struct pppox_sock *ret;
 
 	write_lock_bh(&pppoe_hash_lock);
-	ret = __delete_item(sid, addr);
+	ret = __delete_item(sid, addr, ifindex);
 	write_unlock_bh(&pppoe_hash_lock);
 
 	return ret;
@@ -391,7 +403,7 @@
 
 	ph = (struct pppoe_hdr *) skb->nh.raw;
 
-	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source);
+	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source, dev->ifindex);
 	if (po != NULL)
 		return sk_receive_skb(sk_pppox(po), skb, 0);
 drop:
@@ -425,7 +437,7 @@
 	if (ph->code != PADT_CODE)
 		goto abort;
 
-	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source);
+	po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source, dev->ifindex);
 	if (po) {
 		struct sock *sk = sk_pppox(po);
 
@@ -517,7 +529,7 @@
 
 	po = pppox_sk(sk);
 	if (po->pppoe_pa.sid) {
-		delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote);
+		delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote, po->pppoe_ifindex);
 	}
 
 	if (po->pppoe_dev)
@@ -539,7 +551,7 @@
 		  int sockaddr_len, int flags)
 {
 	struct sock *sk = sock->sk;
-	struct net_device *dev = NULL;
+	struct net_device *dev;
 	struct sockaddr_pppox *sp = (struct sockaddr_pppox *) uservaddr;
 	struct pppox_sock *po = pppox_sk(sk);
 	int error;
@@ -565,7 +577,7 @@
 		pppox_unbind_sock(sk);
 
 		/* Delete the old binding */
-		delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote);
+		delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote,po->pppoe_ifindex);
 
 		if(po->pppoe_dev)
 			dev_put(po->pppoe_dev);
@@ -585,6 +597,7 @@
 			goto end;
 
 		po->pppoe_dev = dev;
+		po->pppoe_ifindex = dev->ifindex;
 
 		if (!(dev->flags & IFF_UP))
 			goto err_put;
@@ -705,7 +718,7 @@
 			break;
 
 		/* PPPoE address from the user specifies an outbound
-		   PPPoE address to which frames are forwarded to */
+		   PPPoE address which frames are forwarded to */
 		err = -EFAULT;
 		if (copy_from_user(&po->pppoe_relay,
 				   (void __user *)arg,
--- linux-2.6.20/include/linux/if_pppox.h.orig	2007-02-09 10:21:19.000000000 +0100
+++ linux-2.6.20/include/linux/if_pppox.h	2007-03-04 02:14:24.000000000 +0100
@@ -114,6 +114,7 @@
 #ifdef __KERNEL__
 struct pppoe_opt {
 	struct net_device      *dev;	  /* device associated with socket*/
+	int			ifindex;  /* ifindex of device associated with socket */
 	struct pppoe_addr	pa;	  /* what this socket is bound to*/
 	struct sockaddr_pppox	relay;	  /* what socket data will be
 					     relayed to (PPPoE relaying) */
@@ -132,6 +133,7 @@
 	unsigned short		num;
 };
 #define pppoe_dev	proto.pppoe.dev
+#define pppoe_ifindex	proto.pppoe.ifindex
 #define pppoe_pa	proto.pppoe.pa
 #define pppoe_relay	proto.pppoe.relay
 

Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[David Miller]

From: Florian Zumbiehl <florz@gmx.de>
Date: Sun, 4 Mar 2007 02:55:16 +0100

> Below you find a slightly changed version of the patch

I already applied your first patch, so if you have any
fixes to submit please provide them as relative patches
to your original change.

Thank you.

Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[Florian Zumbiehl]

> From: Florian Zumbiehl <florz@gmx.de>
> Date: Sun, 4 Mar 2007 02:55:16 +0100
> 
> > Below you find a slightly changed version of the patch
> 
> I already applied your first patch, so if you have any
> fixes to submit please provide them as relative patches
> to your original change.
> 
> Thank you.

Here you go ...

--- linux-2.6.20/drivers/net/pppoe.c.orig	2007-03-04 13:06:01.000000000 +0100
+++ linux-2.6.20/drivers/net/pppoe.c	2007-03-04 02:11:51.000000000 +0100
@@ -140,7 +140,7 @@
 
 	ret = item_hash_table[hash];
 
-	while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_dev->ifindex == ifindex))
+	while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_ifindex == ifindex))
 		ret = ret->next;
 
 	return ret;
@@ -153,7 +153,7 @@
 
 	ret = item_hash_table[hash];
 	while (ret) {
-		if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) && ret->pppoe_dev->ifindex == po->pppoe_dev->ifindex)
+		if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) && ret->pppoe_ifindex == po->pppoe_ifindex)
 			return -EALREADY;
 
 		ret = ret->next;
@@ -174,7 +174,7 @@
 	src = &item_hash_table[hash];
 
 	while (ret) {
-		if (cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_dev->ifindex == ifindex) {
+		if (cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_ifindex == ifindex) {
 			*src = ret->next;
 			break;
 		}
@@ -529,7 +529,7 @@
 
 	po = pppox_sk(sk);
 	if (po->pppoe_pa.sid) {
-		delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote, po->pppoe_dev->ifindex);
+		delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote, po->pppoe_ifindex);
 	}
 
 	if (po->pppoe_dev)
@@ -577,7 +577,7 @@
 		pppox_unbind_sock(sk);
 
 		/* Delete the old binding */
-		delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote,po->pppoe_dev->ifindex);
+		delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote,po->pppoe_ifindex);
 
 		if(po->pppoe_dev)
 			dev_put(po->pppoe_dev);
@@ -597,6 +597,7 @@
 			goto end;
 
 		po->pppoe_dev = dev;
+		po->pppoe_ifindex = dev->ifindex;
 
 		if (!(dev->flags & IFF_UP))
 			goto err_put;
--- linux-2.6.20/include/linux/if_pppox.h.orig	2007-02-09 10:21:19.000000000 +0100
+++ linux-2.6.20/include/linux/if_pppox.h	2007-03-04 02:14:24.000000000 +0100
@@ -114,6 +114,7 @@
 #ifdef __KERNEL__
 struct pppoe_opt {
 	struct net_device      *dev;	  /* device associated with socket*/
+	int			ifindex;  /* ifindex of device associated with socket */
 	struct pppoe_addr	pa;	  /* what this socket is bound to*/
 	struct sockaddr_pppox	relay;	  /* what socket data will be
 					     relayed to (PPPoE relaying) */
@@ -132,6 +133,7 @@
 	unsigned short		num;
 };
 #define pppoe_dev	proto.pppoe.dev
+#define pppoe_ifindex	proto.pppoe.ifindex
 #define pppoe_pa	proto.pppoe.pa
 #define pppoe_relay	proto.pppoe.relay
 

Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[Michal Ostrowski]

Sorry for the late reply.... I've been on the road the past few days.

I ACK the patch.

I'll need to think about it some more, but we could probably go a step
further and eliminate the MAC address from the hash as well.


-- 
Michal Ostrowski <mostrows@earthlink.net>

On Sat, 2007-03-03 at 21:08 -0800, David Miller wrote:
> From: Florian Zumbiehl <florz@gmx.de>
> Date: Sun, 4 Mar 2007 02:55:16 +0100
> 
> > Below you find a slightly changed version of the patch
> 
> I already applied your first patch, so if you have any
> fixes to submit please provide them as relative patches
> to your original change.
> 
> Thank you.
> 

Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface

[David Miller]

From: Florian Zumbiehl <florz@gmx.de>
Date: Sun, 4 Mar 2007 13:09:39 +0100

> > From: Florian Zumbiehl <florz@gmx.de>
> > Date: Sun, 4 Mar 2007 02:55:16 +0100
> > 
> > > Below you find a slightly changed version of the patch
> > 
> > I already applied your first patch, so if you have any
> > fixes to submit please provide them as relative patches
> > to your original change.
> > 
> > Thank you.
> 
> Here you go ...

Applied, thanks a lot Florian.