Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

net/hsr: Fix NULL pointer dereference and refcnt bugs when deleting a HSR interface.

To repeat:

$ sudo ip link del hsr0
BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: [<ffffffff8187f495>] hsr_del_port+0x15/0xa0
etc...

Bug description:

As part of the hsr master device destruction, hsr_del_port() is called for each of
the hsr ports. At each such call, the master device is updated regarding features
and mtu. When the master device is freed before the slave interfaces, master will
be NULL in hsr_del_port(), which led to a NULL pointer dereference.

Additionally, dev_put() was called on the master device itself in hsr_del_port(),
causing a refcnt error.

A third bug in the same code path was that the rtnl lock was not taken before
hsr_del_port() was called as part of hsr_dev_destroy().

The reporter (Nicolas Dichtel) also said: "hsr_netdev_notify() supposes that the
port will always be available when the notification is for an hsr interface. It's
wrong. For example, netdev_wait_allrefs() may resend NETDEV_UNREGISTER.". As a
precaution against this, a check for port == NULL was added in hsr_dev_notify().

Reported-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Fixes: 51f3c605318b056a ("net/hsr: Move slave init to hsr_slave.c.")
Signed-off-by: Arvid Brodin <arvid.brodin@alten.se>
Signed-off-by: David S. Miller <davem@davemloft.net>

authored by

Arvid Brodin and committed by
David S. Miller
56b08fdc 187d6785

+14 -3
+3
net/hsr/hsr_device.c
··· 359 359 struct hsr_port *port; 360 360 361 361 hsr = netdev_priv(hsr_dev); 362 + 363 + rtnl_lock(); 362 364 hsr_for_each_port(hsr, port) 363 365 hsr_del_port(port); 366 + rtnl_unlock(); 364 367 365 368 del_timer_sync(&hsr->prune_timer); 366 369 del_timer_sync(&hsr->announce_timer);
+4
net/hsr/hsr_main.c
··· 36 36 return NOTIFY_DONE; /* Not an HSR device */ 37 37 hsr = netdev_priv(dev); 38 38 port = hsr_port_get_hsr(hsr, HSR_PT_MASTER); 39 + if (port == NULL) { 40 + /* Resend of notification concerning removed device? */ 41 + return NOTIFY_DONE; 42 + } 39 43 } else { 40 44 hsr = port->hsr; 41 45 }
+7 -3
net/hsr/hsr_slave.c
··· 181 181 list_del_rcu(&port->port_list); 182 182 183 183 if (port != master) { 184 - netdev_update_features(master->dev); 185 - dev_set_mtu(master->dev, hsr_get_max_mtu(hsr)); 184 + if (master != NULL) { 185 + netdev_update_features(master->dev); 186 + dev_set_mtu(master->dev, hsr_get_max_mtu(hsr)); 187 + } 186 188 netdev_rx_handler_unregister(port->dev); 187 189 dev_set_promiscuity(port->dev, -1); 188 190 } ··· 194 192 */ 195 193 196 194 synchronize_rcu(); 197 - dev_put(port->dev); 195 + 196 + if (port != master) 197 + dev_put(port->dev); 198 198 }