[PATCH 1/2] most: configfs: serialize mdev_link_list with a mutex

[PATCH 1/2] most: configfs: serialize mdev_link_list with a mutex

[Yu Zhang]

mdev_link_list is a single global list, but it is accessed with no
serialization. mdev_link_create_link_store() adds to it,
mdev_link_destroy_link_store() and mdev_link_release() remove from it
(the latter then frees the entry), and most_interface_register_notify()
walks it.

These run under different locks, or none. The create/destroy stores and
the item release belong to independent configfs subsystems (most_cdev,
most_net, most_video, most_sound), each with its own su_mutex, and
most_interface_register_notify() - called from interface registration -
holds none of them. So a create_link on one subsystem's item can run
concurrently with a rmdir/destroy_link freeing another subsystem's item
on the same global list, corrupting it, and the register-notify walk can
dereference an entry being freed, giving list corruption / a
use-after-free.

Add a dedicated mutex taken around every mdev_link_list access.

Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/most/configfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/most/configfs.c b/drivers/most/configfs.c
index 8f5158d..307a62c 100644
--- a/drivers/most/configfs.c
+++ b/drivers/most/configfs.c
@@ -34,6 +34,7 @@ struct mdev_link {
 };
 
 static struct list_head mdev_link_list;
+static DEFINE_MUTEX(mdev_link_mutex); /* protects mdev_link_list */
 
 static int set_cfg_buffer_size(struct mdev_link *link)
 {
@@ -125,7 +126,9 @@ static ssize_t mdev_link_create_link_store(struct config_item *item,
 	ret = set_config_and_add_link(mdev_link);
 	if (ret && ret != -ENODEV)
 		return ret;
+	mutex_lock(&mdev_link_mutex);
 	list_add_tail(&mdev_link->list, &mdev_link_list);
+	mutex_unlock(&mdev_link_mutex);
 	mdev_link->create_link = tmp;
 	mdev_link->destroy_link = false;
 
@@ -149,8 +152,10 @@ static ssize_t mdev_link_destroy_link_store(struct config_item *item,
 			       mdev_link->comp);
 	if (ret)
 		return ret;
+	mutex_lock(&mdev_link_mutex);
 	if (!list_empty(&mdev_link_list))
 		list_del(&mdev_link->list);
+	mutex_unlock(&mdev_link_mutex);
 
 	mdev_link->destroy_link = tmp;
 
@@ -392,8 +397,10 @@ static void mdev_link_release(struct config_item *item)
 		goto free_item;
 	}
 
+	mutex_lock(&mdev_link_mutex);
 	if (!list_empty(&mdev_link_list))
 		list_del(&mdev_link->list);
+	mutex_unlock(&mdev_link_mutex);
 
 free_item:
 	kfree(to_mdev_link(item));
@@ -679,6 +686,7 @@ void most_interface_register_notify(const char *mdev)
 	bool register_snd_card = false;
 	struct mdev_link *mdev_link;
 
+	mutex_lock(&mdev_link_mutex);
 	list_for_each_entry(mdev_link, &mdev_link_list, list) {
 		if (!strcmp(mdev_link->device, mdev)) {
 			set_config_and_add_link(mdev_link);
@@ -686,6 +694,7 @@ void most_interface_register_notify(const char *mdev)
 				register_snd_card = true;
 		}
 	}
+	mutex_unlock(&mdev_link_mutex);
 	if (register_snd_card)
 		most_cfg_complete("sound");
 }
-- 
2.43.0

[PATCH 2/2] most: core: serialize comp_list and pin the matched component

[Yu Zhang]

comp_list is walked by match_component() and components_show() and
mutated by most_register_component() (list_add_tail) and
most_deregister_component() (list_del), all with no lock - the only locks
in the core are per-channel. A components_show() read or a
match_component() lookup can therefore run concurrently with a component
module unregistering and unloading, walking into the list node that
most_deregister_component() removed and the module freed, i.e. a
list-traversal use-after-free.

match_component() also returned a bare pointer that its callers
(most_add_link(), most_remove_link(), most_cfg_complete()) dereference
after the lookup, so the component could be torn down between the lookup
and the use.

Add a mutex around every comp_list access, and have match_component()
take a reference to the component's module before returning so it stays
alive while the caller uses it; the callers drop it with module_put().

Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/most/core.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/most/core.c b/drivers/most/core.c
index c2616da..d17f538 100644
--- a/drivers/most/core.c
+++ b/drivers/most/core.c
@@ -28,6 +28,7 @@
 static struct ida mdev_id;
 static int dummy_num_buffers;
 static struct list_head comp_list;
+static DEFINE_MUTEX(comp_list_mutex); /* protects comp_list */
 
 struct pipe {
 	struct most_component *comp;
@@ -445,14 +446,21 @@ static const struct attribute_group *interface_attr_groups[] = {
 	NULL,
 };
 
+/* On success takes a module ref; caller must module_put(comp->mod). */
 static struct most_component *match_component(char *name)
 {
 	struct most_component *comp;
 
+	mutex_lock(&comp_list_mutex);
 	list_for_each_entry(comp, &comp_list, list) {
-		if (!strcmp(comp->name, name))
-			return comp;
+		if (strcmp(comp->name, name))
+			continue;
+		if (!try_module_get(comp->mod))
+			comp = NULL;
+		mutex_unlock(&comp_list_mutex);
+		return comp;
 	}
+	mutex_unlock(&comp_list_mutex);
 	return NULL;
 }
 
@@ -517,10 +525,12 @@ static ssize_t components_show(struct device_driver *drv, char *buf)
 	struct most_component *comp;
 	int offs = 0;
 
+	mutex_lock(&comp_list_mutex);
 	list_for_each_entry(comp, &comp_list, list) {
 		offs += scnprintf(buf + offs, PAGE_SIZE - offs, "%s\n",
 				 comp->name);
 	}
+	mutex_unlock(&comp_list_mutex);
 	return offs;
 }
 
@@ -666,12 +676,15 @@ int most_set_cfg_packets_xact(char *mdev, char *mdev_ch, u16 val)
 int most_cfg_complete(char *comp_name)
 {
 	struct most_component *comp;
+	int ret;
 
 	comp = match_component(comp_name);
 	if (!comp)
 		return -ENODEV;
 
-	return comp->cfg_complete();
+	ret = comp->cfg_complete();
+	module_put(comp->mod);
+	return ret;
 }
 
 int most_add_link(char *mdev, char *mdev_ch, char *comp_name, char *link_name,
@@ -679,11 +692,17 @@ int most_add_link(char *mdev, char *mdev_ch, char *comp_name, char *link_name,
 {
 	struct most_channel *c = get_channel(mdev, mdev_ch);
 	struct most_component *comp = match_component(comp_name);
+	int ret;
 
-	if (!c || !comp)
+	if (!c || !comp) {
+		if (comp)
+			module_put(comp->mod);
 		return -ENODEV;
+	}
 
-	return link_channel_to_component(c, comp, link_name, comp_param);
+	ret = link_channel_to_component(c, comp, link_name, comp_param);
+	module_put(comp->mod);
+	return ret;
 }
 
 int most_remove_link(char *mdev, char *mdev_ch, char *comp_name)
@@ -695,15 +714,20 @@ int most_remove_link(char *mdev, char *mdev_ch, char *comp_name)
 	if (!comp)
 		return -ENODEV;
 	c = get_channel(mdev, mdev_ch);
-	if (!c)
+	if (!c) {
+		module_put(comp->mod);
 		return -ENODEV;
+	}
 
-	if (comp->disconnect_channel(c->iface, c->channel_id))
+	if (comp->disconnect_channel(c->iface, c->channel_id)) {
+		module_put(comp->mod);
 		return -EIO;
+	}
 	if (c->pipe0.comp == comp)
 		c->pipe0.comp = NULL;
 	if (c->pipe1.comp == comp)
 		c->pipe1.comp = NULL;
+	module_put(comp->mod);
 	return 0;
 }
 
@@ -1222,7 +1246,9 @@ int most_register_component(struct most_component *comp)
 		pr_err("Bad component\n");
 		return -EINVAL;
 	}
+	mutex_lock(&comp_list_mutex);
 	list_add_tail(&comp->list, &comp_list);
+	mutex_unlock(&comp_list_mutex);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(most_register_component);
@@ -1257,7 +1283,9 @@ int most_deregister_component(struct most_component *comp)
 	}
 
 	bus_for_each_dev(&mostbus, NULL, comp, disconnect_channels);
+	mutex_lock(&comp_list_mutex);
 	list_del(&comp->list);
+	mutex_unlock(&comp_list_mutex);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(most_deregister_component);
-- 
2.43.0