linux-kernel - [PATCH 7/7] soundwire: bus: Fix premature removal of sdw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Sep 2022 11:14:02 +0100
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     <vkoul@...nel.org>, <yung-chuan.liao@...ux.intel.com>,
        <pierre-louis.bossart@...ux.intel.com>, <lgirdwood@...il.com>,
        <peter.ujfalusi@...ux.intel.com>,
        <ranjani.sridharan@...ux.intel.com>,
        <kai.vehmanen@...ux.intel.com>, <daniel.baluta@....com>,
        <sanyog.r.kale@...el.com>, <broonie@...nel.org>
CC:     <alsa-devel@...a-project.org>,
        <sound-open-firmware@...a-project.org>,
        <linux-kernel@...r.kernel.org>, <patches@...nsource.cirrus.com>,
        Richard Fitzgerald <rf@...nsource.cirrus.com>
Subject: [PATCH 7/7] soundwire: bus: Fix premature removal of sdw_slave objects

When the bus manager is removed sdw_bus_master_delete() should not
be deleting the struct sdw_slave objects until the bus manager has
been stopped. The first step of removing child drivers should only
be calling device_unregister() on the child. The counterpart to
sdw_drv_probe() is sdw_drv_remove(), not sdw_delete_slave().

The sdw_slave objects are created by the bus manager probe() from
ACPI/DT information. They are not created when a child driver probes
so should not be deleted by a child driver remove.

Change-Id: I25cc145df12fdc7c126f8f594a5f76eedce25488
Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
---
 drivers/soundwire/bus.c   | 30 ++++++++++++++++++++++++++----
 drivers/soundwire/slave.c | 21 +++++++++++++++++----
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1327a312be86..5533eb589286 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -146,9 +146,8 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
 }
 EXPORT_SYMBOL(sdw_bus_master_add);
 
-static int sdw_delete_slave(struct device *dev, void *data)
+static int sdw_delete_slave(struct sdw_slave *slave)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
 	struct sdw_bus *bus = slave->bus;
 
 	sdw_slave_debugfs_exit(slave);
@@ -163,7 +162,24 @@ static int sdw_delete_slave(struct device *dev, void *data)
 	list_del_init(&slave->node);
 	mutex_unlock(&bus->bus_lock);
 
+	mutex_destroy(&slave->sdw_dev_lock);
+	kfree(slave);
+
+	return 0;
+}
+
+static int sdw_remove_child(struct device *dev, void *data)
+{
+	/*
+	 * Do not remove the struct sdw_slave yet. This is created by
+	 * the bus manager probe() from ACPI information and used by the
+	 * bus manager to hold status of each peripheral. Its lifetime
+	 * is that of the bus manager.
+	 */
+
+	/* This will call sdw_drv_remove() */
 	device_unregister(dev);
+
 	return 0;
 }
 
@@ -171,16 +187,22 @@ static int sdw_delete_slave(struct device *dev, void *data)
  * sdw_bus_master_delete() - delete the bus master instance
  * @bus: bus to be deleted
  *
- * Remove the instance, delete the child devices.
+ * Remove the child devices, remove the master instance.
  */
 void sdw_bus_master_delete(struct sdw_bus *bus)
 {
-	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
+	struct sdw_slave *slave, *tmp;
+
+	device_for_each_child(bus->dev, NULL, sdw_remove_child);
 
 	/* Children have been removed so it is now safe for the bus to stop */
 	if (bus->ops->remove)
 		bus->ops->remove(bus);
 
+	/* Now the bus is stopped it is safe to free things */
+	list_for_each_entry_safe(slave, tmp, &bus->slaves, node)
+		sdw_delete_slave(slave);
+
 	sdw_master_device_del(bus);
 
 	sdw_bus_debugfs_exit(bus);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index c1c1a2ac293a..b6161d002b97 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -10,10 +10,23 @@
 
 static void sdw_slave_release(struct device *dev)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
-
-	mutex_destroy(&slave->sdw_dev_lock);
-	kfree(slave);
+	/*
+	 * The release() callback should not be empty
+	 * (see Documentation/core-api/kobject.rst) but the ownership
+	 * of struct sdw_slave is muddled. It is used for two separate
+	 * purposes:
+	 * 1) by the bus driver to track its own state information for
+	 *    physical devices on the bus and found in ACPI/DT, whether
+	 *    or not there is a child driver for it;
+	 * 2) to hold the child driver object.
+	 *
+	 * The struct sdw_slave cannot be freed when the child driver
+	 * is released because it is holding info used by the bus
+	 * driver. It is freed when the bus driver is removed.
+	 *
+	 * Until the ownership issue is untangled this cannot free
+	 * the struct sdw_slave object containing the child dev.
+	 */
 }
 
 struct device_type sdw_slave_type = {
-- 
2.30.2