[PATCH 0/2] platform/chrome: cros_ec_chardev: To be ec_dev registered-aware

[PATCH 0/2] platform/chrome: cros_ec_chardev: To be ec_dev registered-aware

[Tzung-Bi Shih]

Userland programs may wait forever for MKBP events after the underlying
struct cros_ec_device has been unregistered.  The series tries to return
appropriate errors when detecting such possibilities.

The 1st patch is a clean-up.

The 2nd patch fixes the issue via checking the ec_dev's availability in fops.

The series applies after
"[v3,0/8] platform/chrome: cros_ec_chardev: Fix a possible UAF"
(https://patchwork.kernel.org/project/chrome-platform/cover/20250721044456.2736300-1-tzungbi@kernel.org/).

Tzung-Bi Shih (2):
  platform/chrome: cros_ec_chardev: Don't use wait queue's lock
  platform/chrome: cros_ec_chardev: Check ec_dev's availability in fops

 drivers/platform/chrome/cros_ec_chardev.c | 49 +++++++++++++----------
 1 file changed, 28 insertions(+), 21 deletions(-)

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 1/2] platform/chrome: cros_ec_chardev: Don't use wait queue's lock

[Tzung-Bi Shih]

Lock inside wait_queue_head_t is not designed for protecting custom
data structures.  Don't use it.

Use a dedicated lock for the structure instead.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/platform/chrome/cros_ec_chardev.c | 40 +++++++++++------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index 1cb09ca3b850..5e9ff70c6cf5 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/init.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
@@ -23,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/poll.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
 
@@ -37,6 +39,8 @@ struct chardev_priv {
 	wait_queue_head_t wait_event;
 	unsigned long event_mask;
 	struct list_head events;
+	/* This protects `events`. */
+	spinlock_t lock;
 	size_t event_len;
 	u16 cmd_offset;
 };
@@ -109,11 +113,10 @@ static int cros_ec_chardev_mkbp_event(struct notifier_block *nb,
 	event->event_type = ec_dev->event_data.event_type;
 	memcpy(event->data, &ec_dev->event_data.data, ec_dev->event_size);
 
-	spin_lock(&priv->wait_event.lock);
+	guard(spinlock_irq)(&priv->lock);
 	list_add_tail(&event->node, &priv->events);
 	priv->event_len += total_size;
-	wake_up_locked(&priv->wait_event);
-	spin_unlock(&priv->wait_event.lock);
+	wake_up(&priv->wait_event);
 
 	return NOTIFY_OK;
 }
@@ -124,30 +127,23 @@ static struct ec_event *cros_ec_chardev_fetch_event(struct chardev_priv *priv,
 	struct ec_event *event;
 	int err;
 
-	spin_lock(&priv->wait_event.lock);
-	if (!block && list_empty(&priv->events)) {
-		event = ERR_PTR(-EWOULDBLOCK);
-		goto out;
-	}
+	guard(spinlock_irq)(&priv->lock);
 
-	if (!fetch) {
-		event = NULL;
-		goto out;
-	}
+	if (!block && list_empty(&priv->events))
+		return ERR_PTR(-EWOULDBLOCK);
 
-	err = wait_event_interruptible_locked(priv->wait_event,
-					      !list_empty(&priv->events));
-	if (err) {
-		event = ERR_PTR(err);
-		goto out;
-	}
+	if (!fetch)
+		return NULL;
+
+	err = wait_event_interruptible_lock_irq(priv->wait_event,
+						!list_empty(&priv->events),
+						priv->lock);
+	if (err)
+		return ERR_PTR(err);
 
 	event = list_first_entry(&priv->events, struct ec_event, node);
 	list_del(&event->node);
 	priv->event_len -= sizeof(*event) + event->size;
-
-out:
-	spin_unlock(&priv->wait_event.lock);
 	return event;
 }
 
@@ -170,6 +166,7 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
 	priv->cmd_offset = ec->cmd_offset;
 	filp->private_data = priv;
 	INIT_LIST_HEAD(&priv->events);
+	spin_lock_init(&priv->lock);
 	init_waitqueue_head(&priv->wait_event);
 	nonseekable_open(inode, filp);
 
@@ -191,6 +188,7 @@ static __poll_t cros_ec_chardev_poll(struct file *filp, poll_table *wait)
 
 	poll_wait(filp, &priv->wait_event, wait);
 
+	guard(spinlock_irq)(&priv->lock);
 	if (list_empty(&priv->events))
 		return 0;
 
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 2/2] platform/chrome: cros_ec_chardev: Check ec_dev's availability in fops

[Tzung-Bi Shih]

It is possible that a userland program is waiting for further MKBP
events while the underlying ec_dev is already unregistered.

Check ec_dev's availability in the fops to avoid the userland program
from waiting for some events that should never happen.

Note: if the userland program uses poll() without a timeout, e.g.:
: fd = os.open('/dev/cros_ec', os.O_RDONLY)
: fcntl.ioctl(fd, 0xec02, 0xfff)
: p = select.poll()
: p.register(fd, select.POLLIN)
:
: while True:
:     es = p.poll()            # <--- without a timeout
:     if not es:
:         print("no events")
:         continue
:
:     for f, e in es:
:         if e & select.POLLIN:
:             data = os.read(f, 100)
:             print(data)

It'd still block.  The patch doesn't help with the case.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/platform/chrome/cros_ec_chardev.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index 5e9ff70c6cf5..675bcca16cb5 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -135,11 +135,17 @@ static struct ec_event *cros_ec_chardev_fetch_event(struct chardev_priv *priv,
 	if (!fetch)
 		return NULL;
 
-	err = wait_event_interruptible_lock_irq(priv->wait_event,
-						!list_empty(&priv->events),
-						priv->lock);
-	if (err)
-		return ERR_PTR(err);
+	do {
+		err = wait_event_interruptible_lock_irq_timeout(priv->wait_event,
+								!list_empty(&priv->events),
+								priv->lock,
+								HZ);
+		if (err < 0)
+			return ERR_PTR(err);
+
+		if (!cros_ec_device_registered(priv->ec_dev))
+			return ERR_PTR(-ENODEV);
+	} while (!err);
 
 	event = list_first_entry(&priv->events, struct ec_event, node);
 	list_del(&event->node);
@@ -186,6 +192,9 @@ static __poll_t cros_ec_chardev_poll(struct file *filp, poll_table *wait)
 {
 	struct chardev_priv *priv = filp->private_data;
 
+	if (!cros_ec_device_registered(priv->ec_dev))
+		return EPOLLERR;
+
 	poll_wait(filp, &priv->wait_event, wait);
 
 	guard(spinlock_irq)(&priv->lock);
-- 
2.50.0.727.gbf7dc18ff4-goog