[PATCH net 0/2] NULL pointer dereference in {ipvlan|macvlan}_port_destroy

[PATCH net 0/2] NULL pointer dereference in {ipvlan|macvlan}_port_destroy

[Girish Moodalbail]

>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 

# echo -22 > /sys/kernel/debug/notifier-error-inject/\
  netdev/actions/NETDEV_POST_INIT/error

# 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(). The following two
patches fixes ipvlan and macvlan module.

-----
Girish Moodalbail (2):
  ipvlan: NULL pointer dereference panic in ipvlan_port_destroy
  macvlan: NULL pointer dereference panic in macvlan_port_destroy

 drivers/net/ipvlan/ipvlan_main.c | 6 ++++++
 drivers/net/macvlan.c            | 5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH net 1/2] ipvlan: NULL pointer dereference panic in ipvlan_port_destroy

[Girish Moodalbail]

When call to register_netdevice() (called from ipvlan_link_new()) fails,
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. Fix it.

Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
 drivers/net/ipvlan/ipvlan_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index c74893c..00a62a1 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -602,6 +602,12 @@ int ipvlan_link_new(struct net *src_net, struct net_device *dev,
 unregister_netdev:
 	unregister_netdevice(dev);
 remove_ida:
+	/* Through the call to ipvlan_uninit (ndo_uninit callback) IPvlan port
+	 * might be already destroyed in failure path in register_netdevice()
+	 * or the above call in unregister_netdevice().
+	 */
+	if (!ipvlan_port_get_rtnl(phy_dev))
+		return err;
 	ida_simple_remove(&port->ida, dev->dev_id);
 destroy_ipvlan_port:
 	if (create)
-- 
1.8.3.1

[PATCH net 2/2] macvlan: NULL pointer dereference panic in macvlan_port_destroy

[Girish Moodalbail]

When call to register_netdevice() (called from macvlan_common_newlink())
fails, we call macvlan_uninit() (through ndo_uninit()) to destroy the
macvlan port. Upon returning unsuccessfully from register_netdevice() we
go ahead and call macvlan_port_destroy() again which causes NULL pointer
dereference panic. Fix it.

Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
 drivers/net/macvlan.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d2aea96..2520de8 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1189,6 +1189,9 @@ static void macvlan_port_destroy(struct net_device *dev)
 	struct macvlan_port *port = macvlan_port_get_rtnl(dev);
 	struct sk_buff *skb;
 
+	if (!port)
+		return;
+
 	dev->priv_flags &= ~IFF_MACVLAN_PORT;
 	netdev_rx_handler_unregister(dev);
 
@@ -1444,7 +1447,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	unregister_netdevice(dev);
 destroy_macvlan_port:
 	if (create)
-		macvlan_port_destroy(port->dev);
+		macvlan_port_destroy(lowerdev);
 	return err;
 }
 EXPORT_SYMBOL_GPL(macvlan_common_newlink);
-- 
1.8.3.1

Re: [PATCH net 0/2] NULL pointer dereference in {ipvlan|macvlan}_port_destroy

[David Miller]

From: Girish Moodalbail <girish.moodalbail@oracle.com>
Date: Tue, 31 Oct 2017 09:39:45 -0700

> 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.

The problem is that ipvlan doesn't follow the proper convention that
->ndo_uninit() must only release resources allocated by ->ndo_init().

What needs to happen is that the port allocation occur in
->ndo_init().

Your fix, while solving some cases, does not fully cover all of the
posibiities due to this bug.

Please fix this correctly by moving the port allocation and related
setup from link creation to ->ndo_init().

Thank you.

Re: [PATCH net 0/2] NULL pointer dereference in {ipvlan|macvlan}_port_destroy

[Girish Moodalbail]

On 11/2/17 10:05 PM, David Miller wrote:
> From: Girish Moodalbail <girish.moodalbail@oracle.com>
> Date: Tue, 31 Oct 2017 09:39:45 -0700
> 
>> 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.
> 
> The problem is that ipvlan doesn't follow the proper convention that
> ->ndo_uninit() must only release resources allocated by ->ndo_init().
> 
> What needs to happen is that the port allocation occur in
> ->ndo_init().

I agree, will send out V2. I initially started off making them (ndo_init and 
ndo_uninit) symmetric by moving the port destruction out of ndo_uninit(), but I 
hit some WARN() errors. Will figure it out.

thanks,
~Girish

> 
> Your fix, while solving some cases, does not fully cover all of the
> posibiities due to this bug.
> 
> Please fix this correctly by moving the port allocation and related
> setup from link creation to ->ndo_init().
> 
> Thank you.
>