The cros_ec_chardev driver provides a character device interface to the
ChromeOS EC.  A file handle to this device can remain open in userspace
even if the underlying EC device is removed.

This creates a classic use-after-free vulnerability.  Any file operation
(ioctl, release, etc.) on the open handle after the EC device has gone
would access a stale pointer, leading to a system crash.

To prevent this, leverage the revocable and convert cros_ec_chardev to a
resource consumer of cros_ec_device.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v6:
- Rename REVOCABLE_TRY_ACCESS_WITH() -> REVOCABLE_TRY_ACCESS_SCOPED().
- Use new REVOCABLE_TRY_ACCESS_WITH() if applicable.

v4-v5:
- Doesn't exist.

v3: https://lore.kernel.org/chrome-platform/20250912081718.3827390-6-tzungbi@kernel.org/
- Use specific labels for different cleanup in cros_ec_chardev_open().

v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-6-tzungbi@kernel.org
- Rename "ref_proxy" -> "revocable".
- Fix a sparse warning by removing the redundant __rcu annotation.

v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-4-tzungbi@kernel.org

 drivers/platform/chrome/cros_ec_chardev.c | 71 ++++++++++++++++++-----
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c9d80ad5b57e..bc152c206fb8 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -22,6 +22,7 @@
 #include <linux/platform_data/cros_ec_proto.h>
 #include <linux/platform_device.h>
 #include <linux/poll.h>
+#include <linux/revocable.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
@@ -32,7 +33,7 @@
 #define CROS_MAX_EVENT_LEN	PAGE_SIZE
 
 struct chardev_priv {
-	struct cros_ec_device *ec_dev;
+	struct revocable *ec_dev_rev;
 	struct notifier_block notifier;
 	wait_queue_head_t wait_event;
 	unsigned long event_mask;
@@ -55,6 +56,7 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen)
 	};
 	struct ec_response_get_version *resp;
 	struct cros_ec_command *msg;
+	struct cros_ec_device *ec_dev;
 	int ret;
 
 	msg = kzalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);
@@ -64,7 +66,13 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen)
 	msg->command = EC_CMD_GET_VERSION + priv->cmd_offset;
 	msg->insize = sizeof(*resp);
 
-	ret = cros_ec_cmd_xfer_status(priv->ec_dev, msg);
+	REVOCABLE_TRY_ACCESS_WITH(priv->ec_dev_rev, ec_dev);
+	if (!ec_dev) {
+		ret = -ENODEV;
+		goto exit;
+	}
+
+	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
 	if (ret < 0) {
 		snprintf(str, maxlen,
 			 "Unknown EC version, returned error: %d\n",
@@ -92,10 +100,17 @@ static int cros_ec_chardev_mkbp_event(struct notifier_block *nb,
 {
 	struct chardev_priv *priv = container_of(nb, struct chardev_priv,
 						 notifier);
-	struct cros_ec_device *ec_dev = priv->ec_dev;
+	struct cros_ec_device *ec_dev;
 	struct ec_event *event;
-	unsigned long event_bit = 1 << ec_dev->event_data.event_type;
-	int total_size = sizeof(*event) + ec_dev->event_size;
+	unsigned long event_bit;
+	int total_size;
+
+	REVOCABLE_TRY_ACCESS_WITH(priv->ec_dev_rev, ec_dev);
+	if (!ec_dev)
+		return NOTIFY_DONE;
+
+	event_bit = 1 << ec_dev->event_data.event_type;
+	total_size = sizeof(*event) + ec_dev->event_size;
 
 	if (!(event_bit & priv->event_mask) ||
 	    (priv->event_len + total_size) > CROS_MAX_EVENT_LEN)
@@ -166,7 +181,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
 	if (!priv)
 		return -ENOMEM;
 
-	priv->ec_dev = ec_dev;
+	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
+	if (!priv->ec_dev_rev) {
+		ret = -ENOMEM;
+		goto free_priv;
+	}
+
 	priv->cmd_offset = ec->cmd_offset;
 	filp->private_data = priv;
 	INIT_LIST_HEAD(&priv->events);
@@ -178,9 +198,14 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
 					       &priv->notifier);
 	if (ret) {
 		dev_err(ec_dev->dev, "failed to register event notifier\n");
-		kfree(priv);
+		goto free_revocable;
 	}
 
+	return 0;
+free_revocable:
+	revocable_free(priv->ec_dev_rev);
+free_priv:
+	kfree(priv);
 	return ret;
 }
 
@@ -251,11 +276,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_device *ec_dev = priv->ec_dev;
+	struct cros_ec_device *ec_dev;
 	struct ec_event *event, *e;
 
-	blocking_notifier_chain_unregister(&ec_dev->event_notifier,
-					   &priv->notifier);
+	REVOCABLE_TRY_ACCESS_SCOPED(priv->ec_dev_rev, ec_dev) {
+		if (ec_dev)
+			blocking_notifier_chain_unregister(&ec_dev->event_notifier,
+							   &priv->notifier);
+	}
+	revocable_free(priv->ec_dev_rev);
 
 	list_for_each_entry_safe(event, e, &priv->events, node) {
 		list_del(&event->node);
@@ -273,6 +302,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a
 {
 	struct cros_ec_command *s_cmd;
 	struct cros_ec_command u_cmd;
+	struct cros_ec_device *ec_dev;
 	long ret;
 
 	if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
@@ -299,10 +329,17 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a
 	}
 
 	s_cmd->command += priv->cmd_offset;
-	ret = cros_ec_cmd_xfer(priv->ec_dev, s_cmd);
-	/* Only copy data to userland if data was received. */
-	if (ret < 0)
-		goto exit;
+	REVOCABLE_TRY_ACCESS_SCOPED(priv->ec_dev_rev, ec_dev) {
+		if (!ec_dev) {
+			ret = -ENODEV;
+			goto exit;
+		}
+
+		ret = cros_ec_cmd_xfer(ec_dev, s_cmd);
+		/* Only copy data to userland if data was received. */
+		if (ret < 0)
+			goto exit;
+	}
 
 	if (copy_to_user(arg, s_cmd, sizeof(*s_cmd) + s_cmd->insize))
 		ret = -EFAULT;
@@ -313,10 +350,14 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a
 
 static long cros_ec_chardev_ioctl_readmem(struct chardev_priv *priv, void __user *arg)
 {
-	struct cros_ec_device *ec_dev = priv->ec_dev;
+	struct cros_ec_device *ec_dev;
 	struct cros_ec_readmem s_mem = { };
 	long num;
 
+	REVOCABLE_TRY_ACCESS_WITH(priv->ec_dev_rev, ec_dev);
+	if (!ec_dev)
+		return -ENODEV;
+
 	/* Not every platform supports direct reads */
 	if (!ec_dev->cmd_readmem)
 		return -ENOTTY;
-- 
2.48.1

On Thu, Nov 06, 2025 at 11:26:02PM +0800, Tzung-Bi Shih wrote:
> @@ -166,7 +181,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	priv->ec_dev = ec_dev;
> +	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
> +	if (!priv->ec_dev_rev) {
> +		ret = -ENOMEM;
> +		goto free_priv;
> +	}

The lifecyle of ec_dev->ec_dev->revocable_provider memory is
controlled by dev:

+       ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);

Under the lifecycle of some other driver.

The above only works because misc calls open under the misc_mtx so it
open has "sync" behavior during misc_unregister, and other rules
ensure that ec_dev is valid during the full lifecycle of this driver.

So, I think this cross-driver design an abusive use of the revocable
idea.

It should not be allocated by the parent driver, it should be fully
contained to this driver alone and used only to synchronize the
fops. This would make it clear that the ec_dev pointer must be valid
during the *entire* lifecycle of this driver.

What you have here by putting the providing in another driver is too
magic and obfuscates what the actual lifetime rules are while
providing a giant foot gun for someone to think that just because it
is marked revocable it is fully safe to touch revocable_provider at
any time.

Broadly I think embedding a revocable in the memory that it is trying
to protect is probably an anti-pattern as you must somehow already
have a valid pointer to thing to get the revocable in the first place.
This severely muddies the whole notion of when it can actually be
revoked nor not.

Jason

On Thu, Nov 06, 2025 at 11:59:51AM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 06, 2025 at 11:26:02PM +0800, Tzung-Bi Shih wrote:
> > @@ -166,7 +181,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > -	priv->ec_dev = ec_dev;
> > +	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
> > +	if (!priv->ec_dev_rev) {
> > +		ret = -ENOMEM;
> > +		goto free_priv;
> > +	}
> 
> The lifecyle of ec_dev->ec_dev->revocable_provider memory is
> controlled by dev:
> 
> +       ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
> 
> Under the lifecycle of some other driver.
> 
> The above only works because misc calls open under the misc_mtx so it
> open has "sync" behavior during misc_unregister, and other rules

My understanding is that the file is available to be opened if and only if
the miscdevice is registered.  Are there any other exceptions or scenarios
I might be unaware of?

> ensure that ec_dev is valid during the full lifecycle of this driver.

To clarify, ec_dev is only required to be valid during the .open() call
itself, not for the entire lifecycle of the driver.  Since ec_dev can
become invalid at any other time, the driver uses ec_dev_rev to ensure
safe access.

> So, I think this cross-driver design an abusive use of the revocable
> idea.
> 
> It should not be allocated by the parent driver, it should be fully
> contained to this driver alone and used only to synchronize the
> fops. This would make it clear that the ec_dev pointer must be valid
  ^^^^
ec_dev_rev serves this purpose, not revocable_provider.

> during the *entire* lifecycle of this driver.
> 
> What you have here by putting the providing in another driver is too
> magic and obfuscates what the actual lifetime rules are while
> providing a giant foot gun for someone to think that just because it
> is marked revocable it is fully safe to touch revocable_provider at
> any time.
> 
> Broadly I think embedding a revocable in the memory that it is trying
> to protect is probably an anti-pattern as you must somehow already
> have a valid pointer to thing to get the revocable in the first place.
> This severely muddies the whole notion of when it can actually be
> revoked nor not.

ec_dev->revocable_provider should only be accessed directly within the
.open(), as ec_dev is guaranteed to be valid there.  For all other cases,
it uses ec_dev_rev and checks the validity with revocable_try_access()
to determine if ec_dev has been revoked.

On Wed, Nov 26, 2025 at 04:16:29AM +0000, Tzung-Bi Shih wrote:
> On Thu, Nov 06, 2025 at 11:59:51AM -0400, Jason Gunthorpe wrote:
> > On Thu, Nov 06, 2025 at 11:26:02PM +0800, Tzung-Bi Shih wrote:
> > > @@ -166,7 +181,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >  
> > > -	priv->ec_dev = ec_dev;
> > > +	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
> > > +	if (!priv->ec_dev_rev) {
> > > +		ret = -ENOMEM;
> > > +		goto free_priv;
> > > +	}
> > 
> > The lifecyle of ec_dev->ec_dev->revocable_provider memory is
> > controlled by dev:
> > 
> > +       ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
> > 
> > Under the lifecycle of some other driver.
> > 
> > The above only works because misc calls open under the misc_mtx so it
> > open has "sync" behavior during misc_unregister, and other rules
> 
> My understanding is that the file is available to be opened if and only if
> the miscdevice is registered.  

Yes, through misc_mtx.

> > ensure that ec_dev is valid during the full lifecycle of this driver.
> 
> To clarify, ec_dev is only required to be valid during the .open() call
> itself, not for the entire lifecycle of the driver.  Since ec_dev can
> become invalid at any other time, the driver uses ec_dev_rev to ensure
> safe access.

open can be called during the entire lifecycle of the driver,
misc_deregister() is called during remove. So this is a meaningless
distinction.

ec_dev cannot become invalid while the driver is bound.

> > So, I think this cross-driver design an abusive use of the revocable
> > idea.
> > 
> > It should not be allocated by the parent driver, it should be fully
> > contained to this driver alone and used only to synchronize the
> > fops. This would make it clear that the ec_dev pointer must be valid
>   ^^^^
> ec_dev_rev serves this purpose, not revocable_provider.

How does this detail matter? It is still created by the wrong driver.

> > What you have here by putting the providing in another driver is too
> > magic and obfuscates what the actual lifetime rules are while
> > providing a giant foot gun for someone to think that just because it
> > is marked revocable it is fully safe to touch revocable_provider at
> > any time.
> > 
> > Broadly I think embedding a revocable in the memory that it is trying
> > to protect is probably an anti-pattern as you must somehow already
> > have a valid pointer to thing to get the revocable in the first place.
> > This severely muddies the whole notion of when it can actually be
> > revoked nor not.
> 
> ec_dev->revocable_provider should only be accessed directly within the
> .open(), as ec_dev is guaranteed to be valid there.  For all other cases,
> it uses ec_dev_rev and checks the validity with revocable_try_access()
> to determine if ec_dev has been revoked.

I understand what this does and why it works, I am saying it is an
anti-pattern bad design to cross a revocable between two drivers like
this.

You want the driver creating the fops to revoke a pointer from its own
fops - not span across multiple drivers to achieve the same thing. It
significantly confuses what the actual lifecycle rules are.

Jason