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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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, ð_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