[PATCH 2/2] platform/chrome: cros_ec_chardev: Fix UAF of ec_dev

[PATCH 2/2] platform/chrome: cros_ec_chardev: Fix UAF of ec_dev

[Tzung-Bi Shih]

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[] = {
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH 2/2] platform/chrome: cros_ec_chardev: Fix UAF of ec_dev

[Greg KH]

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);
>  	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`.

What "following code block"?  What will reset the structure?

totally confused,

greg k-h

Re: [PATCH 2/2] platform/chrome: cros_ec_chardev: Fix UAF of ec_dev

[Tzung-Bi Shih]

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;
+		}
+	}

Re: [PATCH 2/2] platform/chrome: cros_ec_chardev: Fix UAF of ec_dev

[Greg KH]

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