[PATCH] EDAC/device: Serialize poll_msec updates against device teardown

Jad Keskes <[email protected]>
Newsgroups org.kernel.vger.linux-edac,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
The poll_msec attribute was writable on interrupt-driven controllers,
where the value is meaningless, and a write racing with device removal
could re-arm the polling workqueue after it had been stopped and the
ctl_info freed -- use-after-free.

Restrict the attribute to controllers that are actually polled: _show()
and _store() now return -EPERM unless the controller is in the
OP_RUNNING_POLL state.

That check alone is not sufficient against the teardown race: _store()
can pass it and then be preempted by a concurrent del_device() that sets
OP_OFFLINE, stops the workqueue and frees the ctl_info once the store
completes.  So re-check the state inside reset_delay_period()
under device_ctls_mutex -- the same lock under which del_device()
updates op_state -- making the check atomic with the re-arm.  The mutex
is taken only around the check and re-arm, never across the stop/teardown
path, so it cannot deadlock against other pollers queued on the shared
EDAC workqueue.

Reported-by: Borislav Petkov <[email protected]>
Signed-off-by: Jad Keskes <[email protected]>
---
 drivers/edac/edac_device.c       | 11 +++++++++++
 drivers/edac/edac_device_sysfs.c |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
index 361dc985497b..519d1c6d0930 100644
--- a/drivers/edac/edac_device.c
+++ b/drivers/edac/edac_device.c
@@ -392,10 +392,21 @@ static void edac_device_workq_teardown(struct edac_device_ctl_info *edac_dev)
  */
 void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev, unsigned int msec)
 {
+	mutex_lock(&device_ctls_mutex);
+
+	/* Don't re-arm the workqueue once teardown has begun or when the
+	 * controller isn't polled; this check must be atomic with the
+	 * re-arm below so it can't race a concurrent del_device().
+	 */
+	if (edac_dev->op_state != OP_RUNNING_POLL)
+		goto out;
+
 	edac_dev->poll_msec = msec;
 	edac_dev->delay     = msecs_to_jiffies(msec);
 
 	edac_mod_work(&edac_dev->work, edac_dev->delay);
+out:
+	mutex_unlock(&device_ctls_mutex);
 }
 
 int edac_device_alloc_index(void)
diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
index e12122b2f42e..821a80806086 100644
--- a/drivers/edac/edac_device_sysfs.c
+++ b/drivers/edac/edac_device_sysfs.c
@@ -83,6 +83,10 @@ static ssize_t edac_device_ctl_panic_on_ue_store(struct edac_device_ctl_info
 static ssize_t edac_device_ctl_poll_msec_show(struct edac_device_ctl_info
 					*ctl_info, char *data)
 {
+	/* Interval is only meaningful while running under polling */
+	if (ctl_info->op_state != OP_RUNNING_POLL)
+		return -EPERM;
+
 	return sprintf(data, "%u\n", ctl_info->poll_msec);
 }
 
@@ -93,6 +97,10 @@ static ssize_t edac_device_ctl_poll_msec_store(struct edac_device_ctl_info
 	unsigned int value;
 	int ret;
 
+	/* Only meaningful when the device is running under polling */
+	if (ctl_info->op_state != OP_RUNNING_POLL)
+		return -EPERM;
+
 	/*
 	 * Get the value, make sure it is non-zero, must be at least one
 	 * millisecond for the delay period between scans.
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.