* [PATCH net v2 0/1] NULL pointer dereference in ipvlan_port_destroy
@ 2017-11-17 7:16 Girish Moodalbail
2017-11-17 7:16 ` [PATCH net v2 1/1] ipvlan: NULL pointer dereference panic " Girish Moodalbail
0 siblings, 1 reply; 3+ messages in thread
From: Girish Moodalbail @ 2017-11-17 7:16 UTC (permalink / raw)
To: netdev, davem, mahesh
>From code inspection it appeared that there is a possibility where in
ipvlan_port_destroy() might be dealing with a port (struct ipvl_port)
that has already been destroyed and is therefore already NULL. However,
we don't check for NULL and continue to access the fields which results
in a kernel panic.
When call to register_netdevice() (called from ipvlan_link_new()) fails,
inside that function we call ipvlan_uninit() (through ndo_uninit()) to
destroy the ipvlan port. Upon returning unsuccessfully from
register_netdevice() we go ahead and call ipvlan_port_destroy() again
which causes NULL pointer dereference panic.
To test this theory, I loaded up netdev-notifier-error-inject.ko and did
$ sudo echo -22 > /sys/kernel/debug/notifier-error-inject/\
netdev/actions/NETDEV_POST_INIT/error
$ sudo ip li add ipvl0 link enp7s0 type ipvlan
...system panics...
BUG: unable to handle kernel NULL pointer dereference at 0000000000000820
IP: ipvlan_port_destroy+0x2a/0xf0 [ipvlan]
Similar issue exists in macvlan_port_destroy() and it will be addressed
by a separate patch. The following patch fixes the ipvlan case. I tested
my changes for regression by running LTP's ipvlan test case.
Girish Moodalbail (1):
ipvlan: NULL pointer dereference panic in ipvlan_port_destroy
drivers/net/ipvlan/ipvlan_main.c | 104 +++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 49 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH net v2 1/1] ipvlan: NULL pointer dereference panic in ipvlan_port_destroy
2017-11-17 7:16 [PATCH net v2 0/1] NULL pointer dereference in ipvlan_port_destroy Girish Moodalbail
@ 2017-11-17 7:16 ` Girish Moodalbail
2017-11-18 1:37 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Girish Moodalbail @ 2017-11-17 7:16 UTC (permalink / raw)
To: netdev, davem, mahesh
When call to register_netdevice() (called from ipvlan_link_new()) fails,
we call ipvlan_uninit() (through ndo_uninit()) to destroy the ipvlan
port. After returning unsuccessfully from register_netdevice() we go
ahead and call ipvlan_port_destroy() again which causes NULL pointer
dereference panic. Fix the issue by making ipvlan_init() and
ipvlan_uninit() call symmetric.
The ipvlan port will now be created inside ipvlan_init() and will be
destroyed in ipvlan_uninit().
Fixes: 2ad7bf363841 (ipvlan: Initial check-in of the IPVLAN driver)
Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
v1 -> v2:
- took care of David Miller's comment on ipvlan_init() and
ipvlan_uninit() not being symmetric.
---
---
drivers/net/ipvlan/ipvlan_main.c | 104 +++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 49 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index a266aa4..30cb803 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -107,16 +107,6 @@ static int ipvlan_port_create(struct net_device *dev)
struct ipvl_port *port;
int err, idx;
- if (dev->type != ARPHRD_ETHER || dev->flags & IFF_LOOPBACK) {
- netdev_err(dev, "Master is either lo or non-ether device\n");
- return -EINVAL;
- }
-
- if (netdev_is_rx_handler_busy(dev)) {
- netdev_err(dev, "Device is already in use.\n");
- return -EBUSY;
- }
-
port = kzalloc(sizeof(struct ipvl_port), GFP_KERNEL);
if (!port)
return -ENOMEM;
@@ -179,8 +169,9 @@ static void ipvlan_port_destroy(struct net_device *dev)
static int ipvlan_init(struct net_device *dev)
{
struct ipvl_dev *ipvlan = netdev_priv(dev);
- const struct net_device *phy_dev = ipvlan->phy_dev;
- struct ipvl_port *port = ipvlan->port;
+ struct net_device *phy_dev = ipvlan->phy_dev;
+ struct ipvl_port *port;
+ int err;
dev->state = (dev->state & ~IPVLAN_STATE_MASK) |
(phy_dev->state & IPVLAN_STATE_MASK);
@@ -196,18 +187,27 @@ static int ipvlan_init(struct net_device *dev)
if (!ipvlan->pcpu_stats)
return -ENOMEM;
+ if (!netif_is_ipvlan_port(phy_dev)) {
+ err = ipvlan_port_create(phy_dev);
+ if (err < 0) {
+ free_percpu(ipvlan->pcpu_stats);
+ return err;
+ }
+ }
+ port = ipvlan_port_get_rtnl(phy_dev);
port->count += 1;
-
return 0;
}
static void ipvlan_uninit(struct net_device *dev)
{
struct ipvl_dev *ipvlan = netdev_priv(dev);
- struct ipvl_port *port = ipvlan->port;
+ struct net_device *phy_dev = ipvlan->phy_dev;
+ struct ipvl_port *port;
free_percpu(ipvlan->pcpu_stats);
+ port = ipvlan_port_get_rtnl(phy_dev);
port->count -= 1;
if (!port->count)
ipvlan_port_destroy(port->dev);
@@ -554,7 +554,6 @@ int ipvlan_link_new(struct net *src_net, struct net_device *dev,
struct net_device *phy_dev;
int err;
u16 mode = IPVLAN_MODE_L3;
- bool create = false;
if (!tb[IFLA_LINK])
return -EINVAL;
@@ -568,28 +567,41 @@ int ipvlan_link_new(struct net *src_net, struct net_device *dev,
phy_dev = tmp->phy_dev;
} else if (!netif_is_ipvlan_port(phy_dev)) {
- err = ipvlan_port_create(phy_dev);
- if (err < 0)
- return err;
- create = true;
- }
+ /* Exit early if the underlying link is invalid or busy */
+ if (phy_dev->type != ARPHRD_ETHER ||
+ phy_dev->flags & IFF_LOOPBACK) {
+ netdev_err(phy_dev,
+ "Master is either lo or non-ether device\n");
+ return -EINVAL;
+ }
- if (data && data[IFLA_IPVLAN_MODE])
- mode = nla_get_u16(data[IFLA_IPVLAN_MODE]);
+ if (netdev_is_rx_handler_busy(phy_dev)) {
+ netdev_err(phy_dev, "Device is already in use.\n");
+ return -EBUSY;
+ }
+ }
- port = ipvlan_port_get_rtnl(phy_dev);
ipvlan->phy_dev = phy_dev;
ipvlan->dev = dev;
- ipvlan->port = port;
ipvlan->sfeatures = IPVLAN_FEATURES;
ipvlan_adjust_mtu(ipvlan, phy_dev);
INIT_LIST_HEAD(&ipvlan->addrs);
- /* Flags are per port and latest update overrides. User has
- * to be consistent in setting it just like the mode attribute.
+ /* TODO Probably put random address here to be presented to the
+ * world but keep using the physical-dev address for the outgoing
+ * packets.
*/
- if (data && data[IFLA_IPVLAN_FLAGS])
- ipvlan->port->flags = nla_get_u16(data[IFLA_IPVLAN_FLAGS]);
+ memcpy(dev->dev_addr, phy_dev->dev_addr, ETH_ALEN);
+
+ dev->priv_flags |= IFF_IPVLAN_SLAVE;
+
+ err = register_netdevice(dev);
+ if (err < 0)
+ return err;
+
+ /* ipvlan_init() would have created the port, if required */
+ port = ipvlan_port_get_rtnl(phy_dev);
+ ipvlan->port = port;
/* If the port-id base is at the MAX value, then wrap it around and
* begin from 0x1 again. This may be due to a busy system where lots
@@ -609,31 +621,28 @@ int ipvlan_link_new(struct net *src_net, struct net_device *dev,
err = ida_simple_get(&port->ida, 0x1, port->dev_id_start,
GFP_KERNEL);
if (err < 0)
- goto destroy_ipvlan_port;
+ goto unregister_netdev;
dev->dev_id = err;
+
/* Increment id-base to the next slot for the future assignment */
port->dev_id_start = err + 1;
- /* TODO Probably put random address here to be presented to the
- * world but keep using the physical-dev address for the outgoing
- * packets.
- */
- memcpy(dev->dev_addr, phy_dev->dev_addr, ETH_ALEN);
+ err = netdev_upper_dev_link(phy_dev, dev, extack);
+ if (err)
+ goto remove_ida;
- dev->priv_flags |= IFF_IPVLAN_SLAVE;
+ /* Flags are per port and latest update overrides. User has
+ * to be consistent in setting it just like the mode attribute.
+ */
+ if (data && data[IFLA_IPVLAN_FLAGS])
+ port->flags = nla_get_u16(data[IFLA_IPVLAN_FLAGS]);
- err = register_netdevice(dev);
- if (err < 0)
- goto remove_ida;
+ if (data && data[IFLA_IPVLAN_MODE])
+ mode = nla_get_u16(data[IFLA_IPVLAN_MODE]);
- err = netdev_upper_dev_link(phy_dev, dev, extack);
- if (err) {
- goto unregister_netdev;
- }
err = ipvlan_set_port_mode(port, mode);
- if (err) {
+ if (err)
goto unlink_netdev;
- }
list_add_tail_rcu(&ipvlan->pnode, &port->ipvlans);
netif_stacked_transfer_operstate(phy_dev, dev);
@@ -641,13 +650,10 @@ int ipvlan_link_new(struct net *src_net, struct net_device *dev,
unlink_netdev:
netdev_upper_dev_unlink(phy_dev, dev);
-unregister_netdev:
- unregister_netdevice(dev);
remove_ida:
ida_simple_remove(&port->ida, dev->dev_id);
-destroy_ipvlan_port:
- if (create)
- ipvlan_port_destroy(phy_dev);
+unregister_netdev:
+ unregister_netdevice(dev);
return err;
}
EXPORT_SYMBOL_GPL(ipvlan_link_new);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net v2 1/1] ipvlan: NULL pointer dereference panic in ipvlan_port_destroy
2017-11-17 7:16 ` [PATCH net v2 1/1] ipvlan: NULL pointer dereference panic " Girish Moodalbail
@ 2017-11-18 1:37 ` David Miller
0 siblings, 0 replies; 3+ messages in thread
From: David Miller @ 2017-11-18 1:37 UTC (permalink / raw)
To: girish.moodalbail; +Cc: netdev, mahesh
From: Girish Moodalbail <girish.moodalbail@oracle.com>
Date: Thu, 16 Nov 2017 23:16:17 -0800
> When call to register_netdevice() (called from ipvlan_link_new()) fails,
> we call ipvlan_uninit() (through ndo_uninit()) to destroy the ipvlan
> port. After returning unsuccessfully from register_netdevice() we go
> ahead and call ipvlan_port_destroy() again which causes NULL pointer
> dereference panic. Fix the issue by making ipvlan_init() and
> ipvlan_uninit() call symmetric.
>
> The ipvlan port will now be created inside ipvlan_init() and will be
> destroyed in ipvlan_uninit().
>
> Fixes: 2ad7bf363841 (ipvlan: Initial check-in of the IPVLAN driver)
> Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
Applied.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-18 1:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-17 7:16 [PATCH net v2 0/1] NULL pointer dereference in ipvlan_port_destroy Girish Moodalbail
2017-11-17 7:16 ` [PATCH net v2 1/1] ipvlan: NULL pointer dereference panic " Girish Moodalbail
2017-11-18 1:37 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).