On Thu, Jul 03, 2025 at 11:35:09AM +0000, Tzung-Bi Shih wrote:
The lifecycle of misc device and `ec_dev` are independent. It is possible that the `ec_dev` is used after free.
The following script shows the concept: : import fcntl : import os : import struct : import time : : EC_CMD_HELLO = 0x1 : : fd = os.open('/dev/cros_fp', os.O_RDONLY) : s = struct.pack('IIIIII', 0, EC_CMD_HELLO, 4, 4, 0, 0) : fcntl.ioctl(fd, 0xc014ec00, s) : : time.sleep(1) : open('/sys/bus/spi/drivers/cros-ec-spi/unbind', 'w').write('spi10.0') : time.sleep(1) : open('/sys/bus/spi/drivers/cros-ec-spi/bind', 'w').write('spi10.0') : : time.sleep(3) : fcntl.ioctl(fd, 0xc014ec00, s) <--- The UAF happens here. : : os.close(fd)
As you have to be root to do this, it's not that big of a deal :)
But yes, one that people have been talking about and discussing generic ways of solving for years now, you have seen the plumbers talks about it, right?
Set `ec_dev` to NULL to let the misc device know the underlying protocol device is gone.
Fixes: eda2e30c6684 ("mfd / platform: cros_ec: Miscellaneous character device to talk with the EC") Cc: stable@vger.kernel.org Signed-off-by: Tzung-Bi Shih tzungbi@kernel.org
drivers/platform/chrome/cros_ec_chardev.c | 65 +++++++++++++++++++---- 1 file changed, 56 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c index 5c858d30dd52..87c800c30f31 100644 --- a/drivers/platform/chrome/cros_ec_chardev.c +++ b/drivers/platform/chrome/cros_ec_chardev.c @@ -11,11 +11,14 @@ */ #include <linux/init.h> +#include <linux/cleanup.h> #include <linux/device.h> #include <linux/fs.h> +#include <linux/list.h> #include <linux/miscdevice.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/notifier.h> #include <linux/platform_data/cros_ec_chardev.h> #include <linux/platform_data/cros_ec_commands.h> @@ -31,7 +34,14 @@ /* Arbitrary bounded size for the event queue */ #define CROS_MAX_EVENT_LEN PAGE_SIZE +/* This protects 'chardev_list' */ +static DEFINE_MUTEX(chardev_lock); +static LIST_HEAD(chardev_list);
Having a static list of chardevices feels very odd and driver-specific, right
struct chardev_priv {
- struct list_head list;
- /* This protects 'ec_dev' */
- struct mutex lock;
Protects it from what?
You now have two locks in play, one for the structure itself, and one for the list. Yet how do they interact as the list is a list of the objects which have their own lock?
Are you _SURE_ you need two locks here? If so, you need to document these really well.
struct cros_ec_dev *ec_dev; struct notifier_block notifier; wait_queue_head_t wait_event; @@ -176,9 +186,14 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp) if (ret) { dev_err(ec_dev->dev, "failed to register event notifier\n"); kfree(priv);
return ret;
- return ret;
- mutex_init(&priv->lock);
- INIT_LIST_HEAD(&priv->list);
- scoped_guard(mutex, &chardev_lock)
list_add_tail(&priv->list, &chardev_list);- return 0;
} static __poll_t cros_ec_chardev_poll(struct file *filp, poll_table *wait) @@ -199,7 +214,6 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer, char msg[sizeof(struct ec_response_get_version) + sizeof(CROS_EC_DEV_VERSION)]; struct chardev_priv *priv = filp->private_data;
- struct cros_ec_dev *ec_dev = priv->ec_dev; size_t count; int ret;
@@ -233,7 +247,12 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer, if (*offset != 0) return 0;
- ret = ec_get_version(ec_dev, msg, sizeof(msg));
- scoped_guard(mutex, &priv->lock) {
if (!priv->ec_dev)return -ENODEV;- }
- ret = ec_get_version(priv->ec_dev, msg, sizeof(msg)); if (ret) return ret;
@@ -249,11 +268,15 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer, static int cros_ec_chardev_release(struct inode *inode, struct file *filp) { struct chardev_priv *priv = filp->private_data;
- struct cros_ec_dev *ec_dev = priv->ec_dev; struct ec_event *event, *e;
- blocking_notifier_chain_unregister(&ec_dev->ec_dev->event_notifier,
&priv->notifier);
- scoped_guard(mutex, &priv->lock) {
if (priv->ec_dev)blocking_notifier_chain_unregister(&priv->ec_dev->ec_dev->event_notifier,&priv->notifier);- }
- scoped_guard(mutex, &chardev_lock)
list_del(&priv->list);list_for_each_entry_safe(event, e, &priv->events, node) { list_del(&event->node); @@ -341,16 +364,20 @@ static long cros_ec_chardev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { struct chardev_priv *priv = filp->private_data;
- struct cros_ec_dev *ec = priv->ec_dev;
if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC) return -ENOTTY;
- scoped_guard(mutex, &priv->lock) {
if (!priv->ec_dev)return -ENODEV;- }
What prevents ec_dev from changing now, after you have just checked it? This feels very wrong as:
- switch (cmd) { case CROS_EC_DEV_IOCXCMD:
return cros_ec_chardev_ioctl_xcmd(ec, (void __user *)arg);
return cros_ec_chardev_ioctl_xcmd(priv->ec_dev, (void __user *)arg);
Look, it could have gone away here, right? If not, how?
case CROS_EC_DEV_IOCRDMEM:
return cros_ec_chardev_ioctl_readmem(ec, (void __user *)arg);
return cros_ec_chardev_ioctl_readmem(priv->ec_dev, (void __user *)arg);@@ -394,8 +421,28 @@ static int cros_ec_chardev_probe(struct platform_device *pdev) static void cros_ec_chardev_remove(struct platform_device *pdev) { struct miscdevice *misc = dev_get_drvdata(&pdev->dev);
- struct chardev_priv *priv;
- /*
* Must deregister the misc device first so that the following* open fops get handled correctly.** misc device is serialized by `misc_mtx`.* 1) If misc_deregister() gets the lock earlier than misc_open(),* the open fops won't be called as the corresponding misc* device is already destroyed.* 2) If misc_open() gets the lock earlier than misc_deregister(),* the following code block resets the `ec_dev` to prevent* the rest of fops from accessing the obsolete `ec_dev`.
What "following code block"? What will reset the structure?
totally confused,
greg k-h