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[] = {
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
On Thu, Jul 03, 2025 at 01:52:02PM +0200, Greg KH wrote:
On Thu, Jul 03, 2025 at 11:35:09AM +0000, Tzung-Bi Shih wrote: 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?
Will check them.
@@ -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
The `chardev_list` is for recording all opened instances. Adding/removing entries in the .open()/.release() fops. The `chardev_lock` is for protecting from accessing the list simultaneously.
They are statically allocated because they can't follow the lifecycle of neither the platform_device (e.g. can be gone after unbinding the driver) nor the chardev (e.g. can be gone after closing the file).
Side note: realized an issue in current version: in the .remove() of platform_driver, it unconditionally resets `ec_dev` for all opened instances.
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 chardev_priv` is bound to chardev's lifecycle. It is allocated in the .open() fops; and freed in the .release() fops. The `lock` is for protecting from accessing the `ec_dev` simultaneously (one is chardev itself, another one is the .remove() of platform_driver).
@@ -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:
Ah, yes. I did it wrong. The `priv->lock` should be held at least for the following cros_ec_chardev_ioctl_xcmd() and cros_ec_chardev_ioctl_readmem().
- 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?
Yes, I did it wrong. The lock should be still held for them.
@@ -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?
+ scoped_guard(mutex, &chardev_lock) { + list_for_each_entry(priv, &chardev_list, list) { + scoped_guard(mutex, &priv->lock) + priv->ec_dev = NULL; + } + }
On Thu, Jul 03, 2025 at 01:14:42PM +0000, Tzung-Bi Shih wrote:
On Thu, Jul 03, 2025 at 01:52:02PM +0200, Greg KH wrote:
On Thu, Jul 03, 2025 at 11:35:09AM +0000, Tzung-Bi Shih wrote: 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?
Will check them.
@@ -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
The `chardev_list` is for recording all opened instances. Adding/removing entries in the .open()/.release() fops. The `chardev_lock` is for protecting from accessing the list simultaneously.
Ick, don't attempt to track objects across open/release, as that's not the driver's job, that's the owner of the object that is doing the open/release (i.e. the cdev) job. You "know" that when open/release happens, your object is "alive". Any other time, you have no idea, so don't attempt to try to track that please.
They are statically allocated because they can't follow the lifecycle of neither the platform_device (e.g. can be gone after unbinding the driver) nor the chardev (e.g. can be gone after closing the file).
Yes, and your object should not have 2 reference counts, which is why this is a common issue. Your single object is trying to span both of them, and the interactions is "messy".
You should be able to do this without the external list. I know many other drivers/subsystems have handled this properly, perhaps look at them?
Or better yet, split the thing apart into your platform and misc device portions?
Or even better, don't worry abouut it as NO ONE should be unbinding a platform device from a driver under real operations. If that does happen, they could be, and should be, doing worse things to your system. This is not a real-world situation that ever should be happening EXCEPT for maybe when you are doing driver development work. So it's a very very very low priority issue.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Tzung-Bi Shih