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.
Reply-To: akpm@linux-foundation.org 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; _
I queued a patch.
Reply-To: shemminger@linux-foundation.org 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).
Reply-To: akpm@linux-foundation.org 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?
Reply-To: shemminger@linux-foundation.org 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??
Reply-To: davem@davemloft.net 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.
Reply-To: davem@davemloft.net 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.
Reply-To: khc@pm.waw.pl 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)?
Reply-To: davem@davemloft.net 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.
Reply-To: khc@pm.waw.pl 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; }
Reply-To: khc@pm.waw.pl 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.
Reply-To: davem@davemloft.net 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!
Actually, I found another place where header_cache_update is dangling: in drivers/media/dvb/dvb-core/dvb_net.c, in function static void dvb_net_setup (struct net_device *dev), dev->hard_header_cache is initialized, but dev- >header_cache_update is not: dev->open = dvb_net_open; dev->stop = dvb_net_stop; dev->hard_start_xmit = dvb_net_tx; dev->get_stats = dvb_net_get_stats; dev->set_multicast_list = dvb_net_set_multicast_list; dev->set_mac_address = dvb_net_set_mac; dev->mtu = 4096; dev->mc_count = 0; dev->hard_header_cache = NULL; dev->flags |= IFF_NOARP; A third spot where hard_header_cache is dangling: in drivers/net/skfp/skfddi.c, in function static int skfp_init_one(struct pci_dev *pdev, const struct pci_device_id *ent), dev->header_cache_update is updated, but dev->hard_header_cache is not: dev->irq = pdev->irq; dev->get_stats = &skfp_ctl_get_stats; dev->open = &skfp_open; dev->stop = &skfp_close; dev->hard_start_xmit = &skfp_send_pkt; dev->set_multicast_list = &skfp_ctl_set_multicast_list; dev->set_mac_address = &skfp_ctl_set_mac_address; dev->do_ioctl = &skfp_ioctl; dev->header_cache_update = NULL; /* not supported */
dvb case is already zeroed memory from kcalloc skfddi likewise At least as of 2.6.27rc