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)
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); + struct chardev_priv { + struct list_head list; + /* This protects 'ec_dev' */ + struct mutex lock; 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; + } + 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); 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); case CROS_EC_DEV_IOCEVENTMASK: priv->event_mask = arg; return 0; @@ -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`. + */ misc_deregister(misc); + + scoped_guard(mutex, &chardev_lock) { + list_for_each_entry(priv, &chardev_list, list) { + scoped_guard(mutex, &priv->lock) + priv->ec_dev = NULL; + } + } }
static const struct platform_device_id cros_ec_chardev_id[] = {