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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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