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

[Greg KH <gregkh@linuxfoundation.org>]

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