Received-SPF: pass (google.com: domain of linux-kernel+bounces-40244-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: <linux-kernel@vger.kernel.org>
CC: Rodrigo Vivi <rodrigo.vivi@intel.com>, Mukesh Ojha
	<quic_mojha@quicinc.com>, Johannes Berg <johannes@sipsolutions.net>, "Greg
 Kroah-Hartman" <gregkh@linuxfoundation.org>, "Rafael J . Wysocki"
	<rafael@kernel.org>, Jose Souza <jose.souza@intel.com>
Subject: [PATCH 2/2] devcoredump: Remove the mutex serialization
Date: Fri, 26 Jan 2024 10:11:20 -0500
Message-ID: <20240126151121.1076079-2-rodrigo.vivi@intel.com>
In-Reply-To: <20240126151121.1076079-1-rodrigo.vivi@intel.com>
References: <20240126151121.1076079-1-rodrigo.vivi@intel.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QBNe1IHJYW4hKW8fp7Wu83u/Fs7B0Guv79p9XJcO2KYk37xvDIL1bBwvzSTQ?=
 =?us-ascii?Q?+llhPahmDN7ht60mP5QJLZm7uF8McE7wFZLCUytD2b4B4BaYh3ZDJXhfpX7V?=
 =?us-ascii?Q?mLLP5DHpKCCwdeFscwnn+v5ddxVLKq8okbOGwn8yc0x8tIZZuQP7qK0Fgfk8?=
 =?us-ascii?Q?3acJg6bnl0aQ1TEuEYzHJXrvbL42sbVzffWG+3hyDlXvQI8Flyn+qPF/UHg8?=
 =?us-ascii?Q?6Ksir8RetSQZwdrXlij1nz1Ew6RViT2i8mSbG6bJO3GU0CbIMptV90EvEo5B?=
 =?us-ascii?Q?2Qu7PS54jx9oumH2yCarWSec4Fi8XxOWabzIfQ01+IHNyOvtWwN0Lfd1wO7s?=
 =?us-ascii?Q?C76XfXzCtOfBvQm+o9nhESkrqPJMKNIQ8wZXWrd495MosMNPmAcJ67FQ9Ezc?=
 =?us-ascii?Q?wbTSuy45XJpozGzNRU4728qMklTNva/qLSPsCCf22WFr1XVxB5dckr3LFW5U?=
 =?us-ascii?Q?sYy0FH88Xxu2UyQkSrqpjeCSHT24WbMFSMnc1IwGc3Z72q2j7WkSoYPDtZG5?=
 =?us-ascii?Q?ro2L8yQlYI02ywEEZCKaqflVu+dOxrmR9Cm9l4qY52tAfjX+mG+Szx6R7ta8?=
 =?us-ascii?Q?G4JA+1WKbspm8mjYagtJhnYgDG30Wqq1tX51Ml2TSGMHVsh1xXQ+Rzj5z82K?=
 =?us-ascii?Q?51M8GPvS7YzuMIFlsHpAfV+WKCiOk8nqRWEGcsIwZ0cdkD0jAmEmcsCXhmeb?=
 =?us-ascii?Q?Fi61qPhRw4uJ48cKnmN3IADTN7i1rI1s5qjeJcjDA36FLiw1GjPZd8xPJju+?=
 =?us-ascii?Q?UdEiiDsMpcBTKhuBIHRqxexLKIed9LfHaPOrAH2pc+RF0I8BkJVGGTxzVJUb?=
 =?us-ascii?Q?hokEREjlFOoQfOqMy7CSj9nBahzhqs+Pof+Gba+swb0rD5HYpKAtUo8d1oPg?=
 =?us-ascii?Q?Rx5NaWJII4IRfsJpV+9AJLrVYdZY3fNY2ipJQVJ7GgHXxTiueKqI6MAdkMB+?=
 =?us-ascii?Q?zYrf6UmTLvhGMgeuDM9B0fHO8mW3oeitbtEOwaGiF4273kDn0oMmzHtONsGR?=
 =?us-ascii?Q?GM4nIO2fbMe8cXIdYhjNv7HlC6yHWojwoa5SwrSkrOiN50g/5NhcdD+BagcU?=
 =?us-ascii?Q?PkSup1faAEHt+01iV0AraIsks50UE17TqJAY3DgrLcgKb98llyjc4FoXEE9t?=
 =?us-ascii?Q?9zo5Hm7zSiDqFlLFI6KsXzHb8yO+cdsSDXl22WymHz5ROtRskyjDayW1o3K+?=
 =?us-ascii?Q?ztejagW7ocXOsJUBpM88BTT2aZ1XGH6TI9BDt1IJ2oSGfpmw5uTtJfmZCAE2?=
 =?us-ascii?Q?iv0lvOr8dhbTC5Zn6wskiHZol8fgEKUn3DP6gVwuv/grntThuTWSF5t7LEXR?=
 =?us-ascii?Q?n44zgyzFVJNHuEBAfQfZmA3KjtTkeUNLc6xd0AL/DivmwGJhF4DaDXRbt/Ic?=
 =?us-ascii?Q?+oH5rKGK+NIzukI6UHur8BFVR8wa6LJnCAhALAuLW/tUvKFpogDuRqxmGHFf?=
 =?us-ascii?Q?7k6nTAro8Bj0yZUNX5GBFseAG+/hHEl5Vv3Ha9bJ8phoXgl44EmDZJrKgoJ6?=
 =?us-ascii?Q?sqiQ9kYjriPgN7Fkr4oKYH6JOt8ozqDHbQv4gx7ibBzWq3fha8JxcA1Hvaqp?=
 =?us-ascii?Q?m9HxSJrb5nRLFms5Ejx3R3dKi5UG463ZJYYywv8w?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a04cd9b-0e80-4923-4e9e-08dc1e8112ce
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6059.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 15:11:29.8029
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xf5ULGKfS6iyrAIwZw3GQyg0VUDJqgwnlYhM61vSMRlgMEf/Gw/iZ2AckwOeOFVmx99rAKktaOFl1Teoj7Dqnw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7873
X-OriginatorOrg: intel.com

The commit 01daccf74832 ("devcoredump : Serialize devcd_del work")
introduced the mutex to protect the case where mod_delayed_work
could be called before the delayed work even existed.

Instead, we can simply initialize the delayed work before the device
is added, so the race condition doesn't exist at first place.

The mutex_unlock is very problematic here. Although mod_delayed_work
is async, we have no warranty that the work is not finished before
the mutex_unlock(devcd->mutex), and if that happen 'devcd' is used
after freed.

Cc: Mukesh Ojha <quic_mojha@quicinc.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Jose Souza <jose.souza@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/base/devcoredump.c | 97 +++-----------------------------------
 1 file changed, 6 insertions(+), 91 deletions(-)

diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index 678ecc2fa242..0e26b1273920 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -25,47 +25,6 @@ struct devcd_entry {
 	struct device devcd_dev;
 	void *data;
 	size_t datalen;
-	/*
-	 * Here, mutex is required to serialize the calls to del_wk work between
-	 * user/kernel space which happens when devcd is added with device_add()
-	 * and that sends uevent to user space. User space reads the uevents,
-	 * and calls to devcd_data_write() which try to modify the work which is
-	 * not even initialized/queued from devcoredump.
-	 *
-	 *
-	 *
-	 *        cpu0(X)                                 cpu1(Y)
-	 *
-	 *        dev_coredump() uevent sent to user space
-	 *        device_add()  ======================> user space process Y reads the
-	 *                                              uevents writes to devcd fd
-	 *                                              which results into writes to
-	 *
-	 *                                             devcd_data_write()
-	 *                                               mod_delayed_work()
-	 *                                                 try_to_grab_pending()
-	 *                                                   del_timer()
-	 *                                                     debug_assert_init()
-	 *       INIT_DELAYED_WORK()
-	 *       schedule_delayed_work()
-	 *
-	 *
-	 * Also, mutex alone would not be enough to avoid scheduling of
-	 * del_wk work after it get flush from a call to devcd_free()
-	 * mentioned as below.
-	 *
-	 *	disabled_store()
-	 *        devcd_free()
-	 *          mutex_lock()             devcd_data_write()
-	 *          flush_delayed_work()
-	 *          mutex_unlock()
-	 *                                   mutex_lock()
-	 *                                   mod_delayed_work()
-	 *                                   mutex_unlock()
-	 * So, delete_work flag is required.
-	 */
-	struct mutex mutex;
-	bool delete_work;
 	struct module *owner;
 	ssize_t (*read)(char *buffer, loff_t offset, size_t count,
 			void *data, size_t datalen);
@@ -125,13 +84,8 @@ static ssize_t devcd_data_write(struct file *filp, struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct devcd_entry *devcd = dev_to_devcd(dev);
 
-	mutex_lock(&devcd->mutex);
-	if (!devcd->delete_work) {
-		devcd->delete_work = true;
-		mod_delayed_work(system_wq, &devcd->del_wk, 0);
-	}
-	mutex_unlock(&devcd->mutex);
-
+	/* This file needs to be closed before devcd can be deleted */
+	mod_delayed_work(system_wq, &devcd->del_wk, 0);
 	return count;
 }
 
@@ -158,12 +112,7 @@ static int devcd_free(struct device *dev, void *data)
 {
 	struct devcd_entry *devcd = dev_to_devcd(dev);
 
-	mutex_lock(&devcd->mutex);
-	if (!devcd->delete_work)
-		devcd->delete_work = true;
-
 	flush_delayed_work(&devcd->del_wk);
-	mutex_unlock(&devcd->mutex);
 	return 0;
 }
 
@@ -173,30 +122,6 @@ static ssize_t disabled_show(const struct class *class, const struct class_attri
 	return sysfs_emit(buf, "%d\n", devcd_disabled);
 }
 
-/*
- *
- *	disabled_store()                                	worker()
- *	 class_for_each_device(&devcd_class,
- *		NULL, NULL, devcd_free)
- *         ...
- *         ...
- *	   while ((dev = class_dev_iter_next(&iter))
- *                                                             devcd_del()
- *                                                               device_del()
- *                                                                 put_device() <- last reference
- *             error = fn(dev, data)                           devcd_dev_release()
- *             devcd_free(dev, data)                           kfree(devcd)
- *             mutex_lock(&devcd->mutex);
- *
- *
- * In the above diagram, It looks like disabled_store() would be racing with parallely
- * running devcd_del() and result in memory abort while acquiring devcd->mutex which
- * is called after kfree of devcd memory  after dropping its last reference with
- * put_device(). However, this will not happens as fn(dev, data) runs
- * with its own reference to device via klist_node so it is not its last reference.
- * so, above situation would not occur.
- */
-
 static ssize_t disabled_store(const struct class *class, const struct class_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -308,13 +233,7 @@ static void devcd_remove(void *data)
 {
 	struct devcd_entry *devcd = data;
 
-	mutex_lock(&devcd->mutex);
-	if (!devcd->delete_work) {
-		devcd->delete_work = true;
-		/* XXX: Cannot flush otherwise the mutex below will hit a UAF */
-		mod_delayed_work(system_wq, &devcd->del_wk, 0);
-	}
-	mutex_unlock(&devcd->mutex);
+	flush_delayed_work(&devcd->del_wk);
 }
 
 /**
@@ -365,16 +284,15 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 	devcd->read = read;
 	devcd->free = free;
 	devcd->failing_dev = get_device(dev);
-	devcd->delete_work = false;
 
-	mutex_init(&devcd->mutex);
 	device_initialize(&devcd->devcd_dev);
 
 	dev_set_name(&devcd->devcd_dev, "devcd%d",
 		     atomic_inc_return(&devcd_count));
 	devcd->devcd_dev.class = &devcd_class;
 
-	mutex_lock(&devcd->mutex);
+	INIT_DELAYED_WORK(&devcd->del_wk, devcd_del);
+	schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
 	dev_set_uevent_suppress(&devcd->devcd_dev, true);
 	if (device_add(&devcd->devcd_dev))
 		goto put_device;
@@ -392,15 +310,12 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 
 	dev_set_uevent_suppress(&devcd->devcd_dev, false);
 	kobject_uevent(&devcd->devcd_dev.kobj, KOBJ_ADD);
-	INIT_DELAYED_WORK(&devcd->del_wk, devcd_del);
-	schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
 	if (devm_add_action(dev, devcd_remove, devcd))
 		dev_warn(dev, "devcoredump managed auto-removal registration failed\n");
-	mutex_unlock(&devcd->mutex);
 	return;
  put_device:
+	cancel_delayed_work(&devcd->del_wk);
 	put_device(&devcd->devcd_dev);
-	mutex_unlock(&devcd->mutex);
  put_module:
 	module_put(owner);
  free:
-- 
2.43.0