[PATCH 0/3] xtensa: iss/network initialization error path fixes

[PATCH 0/3] xtensa: iss/network initialization error path fixes

[Max Filippov]

Hello,

this series cleans up xtensa ISS network driver and fixes memory leaks
in initialization error paths.
The series was prompted by the patch [1] from Yang Yingliang, but that
patch alone has issues:
- a newly created net_device was added to a list of devices and not
  removed from it in case of error leading to UAF. The way the device
  list was used in the driver doesn't make much sense, so patch 1
  removes it altogether.
- a call to platform_device_unregister would complain that iss-netdev
  does not have a release() function and must be fixed. Patch 2 adds the
  release function for the iss-netdev platform device.
- a proper release() function for the platform device must free the
  net_device object, so the error path that calls
  platform_device_unregister must not call free_netdev afterwards to
  avoid double free. I've modified the patch 3 so that it does that and
  updated the description.

[1] https://lore.kernel.org/lkml/20220707023229.2580893-1-yangyingliang@huawei.com/

Max Filippov (2):
  xtensa: iss/network: drop 'devices' list
  xtensa: iss/network: provide release() callback

Yang Yingliang (1):
  xtensa: iss: fix handling error cases in iss_net_configure()

 arch/xtensa/platforms/iss/network.c | 63 +++++++++++++----------------
 1 file changed, 28 insertions(+), 35 deletions(-)

-- 
2.30.2

[PATCH 1/3] xtensa: iss/network: drop 'devices' list

[Max Filippov]

There are two per-device lists in the ISS network driver: command line
parameters list and iss_net_private object list. The latter is only used
for duplicate checking in the function iss_net_setup where the former
should have been used.
Drop iss_net_private object list and associated code and use command
line parameters list in the iss_net_setup instead.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 arch/xtensa/platforms/iss/network.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index fd84d4891758..2d566231688f 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -37,10 +37,6 @@
 #define ETH_HEADER_OTHER 14
 #define ISS_NET_TIMER_VALUE (HZ / 10)
 
-
-static DEFINE_SPINLOCK(devices_lock);
-static LIST_HEAD(devices);
-
 /* ------------------------------------------------------------------------- */
 
 /* We currently only support the TUNTAP transport protocol. */
@@ -70,8 +66,6 @@ struct iss_net_ops {
 /* This structure contains out private information for the driver. */
 
 struct iss_net_private {
-	struct list_head device_list;
-
 	spinlock_t lock;
 	struct net_device *dev;
 	struct platform_device pdev;
@@ -488,7 +482,6 @@ static int iss_net_configure(int index, char *init)
 
 	lp = netdev_priv(dev);
 	*lp = (struct iss_net_private) {
-		.device_list		= LIST_HEAD_INIT(lp->device_list),
 		.dev			= dev,
 		.index			= index,
 	};
@@ -521,10 +514,6 @@ static int iss_net_configure(int index, char *init)
 		driver_registered = 1;
 	}
 
-	spin_lock(&devices_lock);
-	list_add(&lp->device_list, &devices);
-	spin_unlock(&devices_lock);
-
 	lp->pdev.id = index;
 	lp->pdev.name = DRIVER_NAME;
 	platform_device_register(&lp->pdev);
@@ -574,7 +563,7 @@ struct iss_net_init {
 
 static int __init iss_net_setup(char *str)
 {
-	struct iss_net_private *device = NULL;
+	struct iss_net_init *device = NULL;
 	struct iss_net_init *new;
 	struct list_head *ele;
 	char *end;
@@ -595,16 +584,12 @@ static int __init iss_net_setup(char *str)
 	}
 	str = end;
 
-	spin_lock(&devices_lock);
-
-	list_for_each(ele, &devices) {
-		device = list_entry(ele, struct iss_net_private, device_list);
+	list_for_each(ele, &eth_cmd_line) {
+		device = list_entry(ele, struct iss_net_init, list);
 		if (device->index == n)
 			break;
 	}
 
-	spin_unlock(&devices_lock);
-
 	if (device && device->index == n) {
 		pr_err("Device %u already configured\n", n);
 		return 1;
-- 
2.30.2

[PATCH 2/3] xtensa: iss/network: provide release() callback

[Max Filippov]

Provide release() callback for the platform device embedded into struct
iss_net_private and registered in the iss_net_configure so that
platform_device_unregister could be called for it.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 arch/xtensa/platforms/iss/network.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index 2d566231688f..2a22e80a488d 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -466,6 +466,15 @@ static const struct net_device_ops iss_netdev_ops = {
 	.ndo_set_rx_mode	= iss_net_set_multicast_list,
 };
 
+static void iss_net_pdev_release(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iss_net_private *lp =
+		container_of(pdev, struct iss_net_private, pdev);
+
+	free_netdev(lp->dev);
+}
+
 static int iss_net_configure(int index, char *init)
 {
 	struct net_device *dev;
@@ -516,6 +525,7 @@ static int iss_net_configure(int index, char *init)
 
 	lp->pdev.id = index;
 	lp->pdev.name = DRIVER_NAME;
+	lp->pdev.dev.release = iss_net_pdev_release;
 	platform_device_register(&lp->pdev);
 	SET_NETDEV_DEV(dev, &lp->pdev.dev);
 
-- 
2.30.2

[PATCH v3 3/3] xtensa: iss: fix handling error cases in iss_net_configure()

[Max Filippov]

From: Yang Yingliang <yangyingliang@huawei.com>

The 'pdev' and 'netdev' need to be released in error cases of
iss_net_configure().

Change the return type of iss_net_configure() to void, because it's
not used.

Fixes: 7282bee78798 ("[PATCH] xtensa: Architecture support for Tensilica Xtensa Part 8")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v2->v3:
- drop err_unregister_device label and return after calling
  platform_device_unregister in case of register_netdevice failure

Changes v1->v2:
- change return type of iss_net_configure() to void

 arch/xtensa/platforms/iss/network.c | 32 ++++++++++++++---------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index 2a22e80a488d..9ac46ab3a296 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -475,16 +475,15 @@ static void iss_net_pdev_release(struct device *dev)
 	free_netdev(lp->dev);
 }
 
-static int iss_net_configure(int index, char *init)
+static void iss_net_configure(int index, char *init)
 {
 	struct net_device *dev;
 	struct iss_net_private *lp;
-	int err;
 
 	dev = alloc_etherdev(sizeof(*lp));
 	if (dev == NULL) {
 		pr_err("eth_configure: failed to allocate device\n");
-		return 1;
+		return;
 	}
 
 	/* Initialize private element. */
@@ -511,7 +510,7 @@ static int iss_net_configure(int index, char *init)
 	if (!tuntap_probe(lp, index, init)) {
 		pr_err("%s: invalid arguments. Skipping device!\n",
 		       dev->name);
-		goto errout;
+		goto err_free_netdev;
 	}
 
 	pr_info("Netdevice %d (%pM)\n", index, dev->dev_addr);
@@ -519,14 +518,16 @@ static int iss_net_configure(int index, char *init)
 	/* sysfs register */
 
 	if (!driver_registered) {
-		platform_driver_register(&iss_net_driver);
+		if (platform_driver_register(&iss_net_driver))
+			goto err_free_netdev;
 		driver_registered = 1;
 	}
 
 	lp->pdev.id = index;
 	lp->pdev.name = DRIVER_NAME;
 	lp->pdev.dev.release = iss_net_pdev_release;
-	platform_device_register(&lp->pdev);
+	if (platform_device_register(&lp->pdev))
+		goto err_free_netdev;
 	SET_NETDEV_DEV(dev, &lp->pdev.dev);
 
 	dev->netdev_ops = &iss_netdev_ops;
@@ -535,23 +536,20 @@ static int iss_net_configure(int index, char *init)
 	dev->irq = -1;
 
 	rtnl_lock();
-	err = register_netdevice(dev);
-	rtnl_unlock();
-
-	if (err) {
+	if (register_netdevice(dev)) {
+		rtnl_unlock();
 		pr_err("%s: error registering net device!\n", dev->name);
-		/* XXX: should we call ->remove() here? */
-		free_netdev(dev);
-		return 1;
+		platform_device_unregister(&lp->pdev);
+		return;
 	}
+	rtnl_unlock();
 
 	timer_setup(&lp->tl, iss_net_user_timer_expire, 0);
 
-	return 0;
+	return;
 
-errout:
-	/* FIXME: unregister; free, etc.. */
-	return -EIO;
+err_free_netdev:
+	free_netdev(dev);
 }
 
 /* ------------------------------------------------------------------------- */
-- 
2.30.2