Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Andrew Morton]

On Thu, 1 Mar 2007 11:33:05 -0800
bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=8107
> 
>            Summary: dev->header_cache_update has a random value
>     Kernel Version: 2.6.20
>             Status: NEW
>           Severity: high
>              Owner: jgarzik@pobox.com
>          Submitter: loveminix@yahoo.com.cn
> 
> 
> Distribution: Kernel 2.6.20
> 
> Problem Description:
> 
> In struct net_device, there are two fields: hard_header_cache and 
> header_cache_update, both of which are function pointers. The third field, 
> hard_header, is also a function pointer. Whenever hard_header points to a valid 
> function, both hard_header_cache and header_cache_update should have a known 
> value, either NULL or a valid function pointer. However, in 
> drivers/net/wan/hdlc_cisco.c, in function static int cisco_ioctl(struct 
> net_device *dev, struct ifreq *ifr), where dev->hard_header is assigned a valid 
> function, and dev->hard_header_cache is assigned a known value (NULL), dev-
> >header_cache_update is not set to a known value:
> 
> dev->hard_start_xmit = hdlc->xmit;
>         dev->hard_header = cisco_hard_header;
>         dev->hard_header_cache = NULL;
>         dev->type = ARPHRD_CISCO;
>         dev->flags = IFF_POINTOPOINT | IFF_NOARP;
>         dev->addr_len = 0;
> 
> This may cause serious problems when dev->header_cache_update is invoked, 
> because it has an uninitialized value.
> 
> Steps to reproduce:
> 
> I found this suspicious spot with the help of a code-checking tool.
> 

Like this?



From: Andrew Morton <akpm@linux-foundation.org>

Fix http://bugzilla.kernel.org/show_bug.cgi?id=8107: we weren't initialising
the header_cache_update field.

Cc: Krzysztof Halasa <khc@pm.waw.pl>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/wan/hdlc_cisco.c |    1 +
 1 files changed, 1 insertion(+)

diff -puN drivers/net/wan/hdlc_cisco.c~cisco_ioctl-initialise-header_cache_update drivers/net/wan/hdlc_cisco.c
--- a/drivers/net/wan/hdlc_cisco.c~cisco_ioctl-initialise-header_cache_update
+++ a/drivers/net/wan/hdlc_cisco.c
@@ -366,6 +366,7 @@ static int cisco_ioctl(struct net_device
 		dev->hard_start_xmit = hdlc->xmit;
 		dev->hard_header = cisco_hard_header;
 		dev->hard_header_cache = NULL;
+		dev->header_cache_update = NULL;
 		dev->type = ARPHRD_CISCO;
 		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
 		dev->addr_len = 0;
_

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Stephen Hemminger]

On Thu, 1 Mar 2007 14:34:17 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 1 Mar 2007 11:33:05 -0800
> bugme-daemon@bugzilla.kernel.org wrote:
> 
> > http://bugzilla.kernel.org/show_bug.cgi?id=8107
> > 
> >            Summary: dev->header_cache_update has a random value
> >     Kernel Version: 2.6.20
> >             Status: NEW
> >           Severity: high
> >              Owner: jgarzik@pobox.com
> >          Submitter: loveminix@yahoo.com.cn
> > 
> > 
> > Distribution: Kernel 2.6.20
> > 
> > Problem Description:
> > 
> > In struct net_device, there are two fields: hard_header_cache and 
> > header_cache_update, both of which are function pointers. The third field, 
> > hard_header, is also a function pointer. Whenever hard_header points to a valid 
> > function, both hard_header_cache and header_cache_update should have a known 
> > value, either NULL or a valid function pointer. However, in 
> > drivers/net/wan/hdlc_cisco.c, in function static int cisco_ioctl(struct 
> > net_device *dev, struct ifreq *ifr), where dev->hard_header is assigned a valid 
> > function, and dev->hard_header_cache is assigned a known value (NULL), dev-
> > >header_cache_update is not set to a known value:
> > 
> > dev->hard_start_xmit = hdlc->xmit;
> >         dev->hard_header = cisco_hard_header;
> >         dev->hard_header_cache = NULL;
> >         dev->type = ARPHRD_CISCO;
> >         dev->flags = IFF_POINTOPOINT | IFF_NOARP;
> >         dev->addr_len = 0;
> > 
> > This may cause serious problems when dev->header_cache_update is invoked, 
> > because it has an uninitialized value.
> > 
> > Steps to reproduce:
> > 
> > I found this suspicious spot with the help of a code-checking tool.
> > 
> 
> Like this?

Not necessary, since any network device must already allocated by
alloc_netdev() and it initializes the whole struct to 0 (NULL).

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Andrew Morton]

On Thu, 1 Mar 2007 14:37:27 -0800
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Thu, 1 Mar 2007 14:34:17 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Thu, 1 Mar 2007 11:33:05 -0800
> > bugme-daemon@bugzilla.kernel.org wrote:
> > 
> > > http://bugzilla.kernel.org/show_bug.cgi?id=8107
> > > 
> > >            Summary: dev->header_cache_update has a random value
> > >     Kernel Version: 2.6.20
> > >             Status: NEW
> > >           Severity: high
> > >              Owner: jgarzik@pobox.com
> > >          Submitter: loveminix@yahoo.com.cn
> > > 
> > > 
> > > Distribution: Kernel 2.6.20
> > > 
> > > Problem Description:
> > > 
> > > In struct net_device, there are two fields: hard_header_cache and 
> > > header_cache_update, both of which are function pointers. The third field, 
> > > hard_header, is also a function pointer. Whenever hard_header points to a valid 
> > > function, both hard_header_cache and header_cache_update should have a known 
> > > value, either NULL or a valid function pointer. However, in 
> > > drivers/net/wan/hdlc_cisco.c, in function static int cisco_ioctl(struct 
> > > net_device *dev, struct ifreq *ifr), where dev->hard_header is assigned a valid 
> > > function, and dev->hard_header_cache is assigned a known value (NULL), dev-
> > > >header_cache_update is not set to a known value:
> > > 
> > > dev->hard_start_xmit = hdlc->xmit;
> > >         dev->hard_header = cisco_hard_header;
> > >         dev->hard_header_cache = NULL;
> > >         dev->type = ARPHRD_CISCO;
> > >         dev->flags = IFF_POINTOPOINT | IFF_NOARP;
> > >         dev->addr_len = 0;
> > > 
> > > This may cause serious problems when dev->header_cache_update is invoked, 
> > > because it has an uninitialized value.
> > > 
> > > Steps to reproduce:
> > > 
> > > I found this suspicious spot with the help of a code-checking tool.
> > > 
> > 
> > Like this?
> 
> Not necessary, since any network device must already allocated by
> alloc_netdev() and it initializes the whole struct to 0 (NULL).

But ioctl(IF_PROTO_CISCO) can be run multiple times across the lifetime of
a net_device?

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Stephen Hemminger]

On Thu, 1 Mar 2007 14:54:23 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 1 Mar 2007 14:37:27 -0800
> Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> 
> > On Thu, 1 Mar 2007 14:34:17 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Thu, 1 Mar 2007 11:33:05 -0800
> > > bugme-daemon@bugzilla.kernel.org wrote:
> > > 
> > > > http://bugzilla.kernel.org/show_bug.cgi?id=8107
> > > > 
> > > >            Summary: dev->header_cache_update has a random value
> > > >     Kernel Version: 2.6.20
> > > >             Status: NEW
> > > >           Severity: high
> > > >              Owner: jgarzik@pobox.com
> > > >          Submitter: loveminix@yahoo.com.cn
> > > > 
> > > > 
> > > > Distribution: Kernel 2.6.20
> > > > 
> > > > Problem Description:
> > > > 
> > > > In struct net_device, there are two fields: hard_header_cache and 
> > > > header_cache_update, both of which are function pointers. The third field, 
> > > > hard_header, is also a function pointer. Whenever hard_header points to a valid 
> > > > function, both hard_header_cache and header_cache_update should have a known 
> > > > value, either NULL or a valid function pointer. However, in 
> > > > drivers/net/wan/hdlc_cisco.c, in function static int cisco_ioctl(struct 
> > > > net_device *dev, struct ifreq *ifr), where dev->hard_header is assigned a valid 
> > > > function, and dev->hard_header_cache is assigned a known value (NULL), dev-
> > > > >header_cache_update is not set to a known value:
> > > > 
> > > > dev->hard_start_xmit = hdlc->xmit;
> > > >         dev->hard_header = cisco_hard_header;
> > > >         dev->hard_header_cache = NULL;
> > > >         dev->type = ARPHRD_CISCO;
> > > >         dev->flags = IFF_POINTOPOINT | IFF_NOARP;
> > > >         dev->addr_len = 0;
> > > > 
> > > > This may cause serious problems when dev->header_cache_update is invoked, 
> > > > because it has an uninitialized value.
> > > > 
> > > > Steps to reproduce:
> > > > 
> > > > I found this suspicious spot with the help of a code-checking tool.
> > > > 
> > > 
> > > Like this?
> > 
> > Not necessary, since any network device must already allocated by
> > alloc_netdev() and it initializes the whole struct to 0 (NULL).
> 
> But ioctl(IF_PROTO_CISCO) can be run multiple times across the lifetime of
> a net_device?

Your right, but so far there is no ioctl to take it out of this mode.
So it is a one way door.

This device never calls hdlc->detach??

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Thu, 1 Mar 2007 15:30:25 -0800

> Your right, but so far there is no ioctl to take it out of this mode.
> So it is a one way door.

You can keep attaching a different new protocol to an HDLC
device until it is brought up.

> This device never calls hdlc->detach??

The HDLC layer calls the protocol ->detach, but the Cisco
HDLC code does not have a detach method.

I'm starting to feel that this is the bug, because as it
stands the Cisco HDLC code leaves stale pointers around
and then if you unload the Cisco HDLC module things can
explode.

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Thu, 1 Mar 2007 14:37:27 -0800

> Not necessary, since any network device must already allocated by
> alloc_netdev() and it initializes the whole struct to 0 (NULL).

It is in this case, unfortunately, HDLC protocols can be registers
several times before the device is brought up and protocol changes
become disallowed.

So you can attach one, then a second one, and the second one has
to explicitly initialize the pointers potentially set by the
first one.

On the other hand, you could say that it's the protocol
->detach() method's responsibility to NULL out these things
instead of leaving references to function pointers of
a module that's about the be unloaded.

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Krzysztof Halasa]

Andrew Morton <akpm@linux-foundation.org> writes:

>> However, in 
>> drivers/net/wan/hdlc_cisco.c, in function static int cisco_ioctl(struct 
>> net_device *dev, struct ifreq *ifr), where dev->hard_header is assigned a valid 
>> function, and dev->hard_header_cache is assigned a known value (NULL), dev-
>> >header_cache_update is not set to a known value:

Right, it seems I was never aware of dev->header_cache_update existence.
I wonder where does the non-NULL value come from? Nevermind.

> diff -puN drivers/net/wan/hdlc_cisco.c~cisco_ioctl-initialise-header_cache_update drivers/net/wan/hdlc_cisco.c
> --- a/drivers/net/wan/hdlc_cisco.c~cisco_ioctl-initialise-header_cache_update
> +++ a/drivers/net/wan/hdlc_cisco.c
> @@ -366,6 +366,7 @@ static int cisco_ioctl(struct net_device
>  		dev->hard_start_xmit = hdlc->xmit;
>  		dev->hard_header = cisco_hard_header;
>  		dev->hard_header_cache = NULL;
> +		dev->header_cache_update = NULL;
>  		dev->type = ARPHRD_CISCO;
>  		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
>  		dev->addr_len = 0;
> _

ACK, I think it's the best place.

Is it OK to leave this (and hard_header_cache) set to random value
if dev->hard_header = NULL (as with other protocols)?
-- 
Krzysztof Halasa

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Fri, 02 Mar 2007 16:29:06 +0100

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> >> However, in 
> >> drivers/net/wan/hdlc_cisco.c, in function static int cisco_ioctl(struct 
> >> net_device *dev, struct ifreq *ifr), where dev->hard_header is assigned a valid 
> >> function, and dev->hard_header_cache is assigned a known value (NULL), dev-
> >> >header_cache_update is not set to a known value:
> 
> Right, it seems I was never aware of dev->header_cache_update existence.
> I wonder where does the non-NULL value come from? Nevermind.
> 
> > diff -puN drivers/net/wan/hdlc_cisco.c~cisco_ioctl-initialise-header_cache_update drivers/net/wan/hdlc_cisco.c
> > --- a/drivers/net/wan/hdlc_cisco.c~cisco_ioctl-initialise-header_cache_update
> > +++ a/drivers/net/wan/hdlc_cisco.c
> > @@ -366,6 +366,7 @@ static int cisco_ioctl(struct net_device
> >  		dev->hard_start_xmit = hdlc->xmit;
> >  		dev->hard_header = cisco_hard_header;
> >  		dev->hard_header_cache = NULL;
> > +		dev->header_cache_update = NULL;
> >  		dev->type = ARPHRD_CISCO;
> >  		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
> >  		dev->addr_len = 0;
> > _
> 
> ACK, I think it's the best place.

I disagree, you can't leave dangling references to functions
which are potentially inside of unloaded modules, as this code
does.

Rather, HDLC Cisco should implement a proper protocol destructor
method to clean up these function pointers.

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Krzysztof Halasa]

Switching HDLC devices from Ethernet-framing mode caused stale ethernet
function assignments within net_device.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

diff --git a/drivers/net/wan/hdlc.c b/drivers/net/wan/hdlc.c
index db354e0..f6e6b63 100644
--- a/drivers/net/wan/hdlc.c
+++ b/drivers/net/wan/hdlc.c
@@ -38,7 +38,7 @@
 #include <linux/hdlc.h>
 
 
-static const char* version = "HDLC support module revision 1.20";
+static const char* version = "HDLC support module revision 1.21";
 
 #undef DEBUG_LINK
 
@@ -222,19 +222,30 @@ int hdlc_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return -EINVAL;
 }
 
+static void hdlc_setup_dev(struct net_device *dev)
+{
+/* Re-init all variables changed by HDLC protocol drivers,
+   including ether_setup() called from hdlc_raw_eth.c. */
+	dev->get_stats		 = hdlc_get_stats;
+	dev->flags		 = IFF_POINTOPOINT | IFF_NOARP;
+	dev->mtu		 = HDLC_MAX_MTU;
+	dev->type		 = ARPHRD_RAWHDLC;
+	dev->hard_header_len	 = 16;
+	dev->addr_len		 = 0;
+	dev->hard_header	 = NULL;
+	dev->rebuild_header	 = NULL;
+	dev->set_mac_address	 = NULL;
+	dev->hard_header_cache	 = NULL;
+	dev->header_cache_update = NULL;
+	dev->change_mtu		 = hdlc_change_mtu;
+	dev->hard_header_parse	 = NULL;
+}
+
 void hdlc_setup(struct net_device *dev)
 {
 	hdlc_device *hdlc = dev_to_hdlc(dev);
 
-	dev->get_stats = hdlc_get_stats;
-	dev->change_mtu = hdlc_change_mtu;
-	dev->mtu = HDLC_MAX_MTU;
-
-	dev->type = ARPHRD_RAWHDLC;
-	dev->hard_header_len = 16;
-
-	dev->flags = IFF_POINTOPOINT | IFF_NOARP;
-
+	hdlc_setup_dev(dev);
 	hdlc->carrier = 1;
 	hdlc->open = 0;
 	spin_lock_init(&hdlc->state_lock);
@@ -294,6 +305,7 @@ void detach_hdlc_protocol(struct net_device *dev)
 	}
 	kfree(hdlc->state);
 	hdlc->state = NULL;
+	hdlc_setup_dev(dev);
 }
 
 
diff --git a/drivers/net/wan/hdlc_cisco.c b/drivers/net/wan/hdlc_cisco.c
index b0bc5dd..c9664fd 100644
--- a/drivers/net/wan/hdlc_cisco.c
+++ b/drivers/net/wan/hdlc_cisco.c
@@ -365,10 +365,7 @@ static int cisco_ioctl(struct net_device *dev, struct ifreq *ifr)
 		memcpy(&state(hdlc)->settings, &new_settings, size);
 		dev->hard_start_xmit = hdlc->xmit;
 		dev->hard_header = cisco_hard_header;
-		dev->hard_header_cache = NULL;
 		dev->type = ARPHRD_CISCO;
-		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
-		dev->addr_len = 0;
 		netif_dormant_on(dev);
 		return 0;
 	}
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index b45ab68..c6c3c75 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -1289,10 +1289,7 @@ static int fr_ioctl(struct net_device *dev, struct ifreq *ifr)
 		memcpy(&state(hdlc)->settings, &new_settings, size);
 
 		dev->hard_start_xmit = hdlc->xmit;
-		dev->hard_header = NULL;
 		dev->type = ARPHRD_FRAD;
-		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
-		dev->addr_len = 0;
 		return 0;
 
 	case IF_PROTO_FR_ADD_PVC:
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index e9f7170..4591437 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -127,9 +127,7 @@ static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 		if (result)
 			return result;
 		dev->hard_start_xmit = hdlc->xmit;
-		dev->hard_header = NULL;
 		dev->type = ARPHRD_PPP;
-		dev->addr_len = 0;
 		netif_dormant_off(dev);
 		return 0;
 	}
diff --git a/drivers/net/wan/hdlc_raw.c b/drivers/net/wan/hdlc_raw.c
index fe3cae5..e23bc66 100644
--- a/drivers/net/wan/hdlc_raw.c
+++ b/drivers/net/wan/hdlc_raw.c
@@ -88,10 +88,7 @@ static int raw_ioctl(struct net_device *dev, struct ifreq *ifr)
 			return result;
 		memcpy(hdlc->state, &new_settings, size);
 		dev->hard_start_xmit = hdlc->xmit;
-		dev->hard_header = NULL;
 		dev->type = ARPHRD_RAWHDLC;
-		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
-		dev->addr_len = 0;
 		netif_dormant_off(dev);
 		return 0;
 	}
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index e4bb9f8..cd7b22f 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -215,9 +215,7 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
 						   x25_rx, 0)) != 0)
 			return result;
 		dev->hard_start_xmit = x25_xmit;
-		dev->hard_header = NULL;
 		dev->type = ARPHRD_X25;
-		dev->addr_len = 0;
 		netif_dormant_off(dev);
 		return 0;
 	}

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Sat, 03 Mar 2007 00:38:05 +0100

> Switching HDLC devices from Ethernet-framing mode caused stale ethernet
> function assignments within net_device.
> 
> Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

This looks good to me, I think I'll apply it :-)

Thanks!

Re: [Bugme-new] [Bug 8107] New: dev->header_cache_update has a random value

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> I disagree, you can't leave dangling references to functions
> which are potentially inside of unloaded modules, as this code
> does.

All such pointers were thought to be initialized by all HDLC protocol
handlers before device activation, but they were actually used by the
hdlc* code, and this one doesn't seem to...

> Rather, HDLC Cisco should implement a proper protocol destructor
> method to clean up these function pointers.

No, it wouldn't work - hdlc_cisco doesn't use it at all, it's just
a victim. But now I think there may be other victims.

It seems the only way to become non-NULL is through ether_setup()
from hdlc_raw_eth.c (Ethernet framing over HDLC).

I think it's best to NULLify it and the like in hdlc.c
unconditionally, it's slow path and we don't need another useless
EXPORT_SYMBOL(s). It would fix all such problems forever.

Compile-tested only but it seems pretty obvious and of course I check
if the packets still flow after regular kernel upgrades (and I run
automatic tests checking all protos except X.25 from time to time as
well).

(the patch is in the next message).

Not sure if 2.6.21 material.
-- 
Krzysztof Halasa