[PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Tzung-Bi Shih]

This is a follow-up series of [1].  It tries to fix a possible UAF in the
fops of cros_ec_chardev after the underlying protocol device has gone by
using revocable.

The 1st patch introduces the revocable which is an implementation of ideas
from the talk [2].

The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.

The 4th patch converts existing protocol devices to resource providers
of cros_ec_device.

The 5th patch converts cros_ec_chardev to a resource consumer of
cros_ec_device to fix the UAF.

[1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@kernel.org/
[2] https://lpc.events/event/17/contributions/1627/

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>

v3:
- Rebase onto https://lore.kernel.org/chrome-platform/20250828083601.856083-1-tzungbi@kernel.org
  and next-20250912.
- Change the 4th patch accordingly.

v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-1-tzungbi@kernel.org
- Rename "ref_proxy" -> "revocable".
- Add test cases in Kunit and selftest.

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

Tzung-Bi Shih (5):
  revocable: Revocable resource management
  revocable: Add Kunit test cases
  selftests: revocable: Add kselftest cases
  platform/chrome: Protect cros_ec_device lifecycle with revocable
  platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable

 .../driver-api/driver-model/index.rst         |   1 +
 .../driver-api/driver-model/revocable.rst     | 151 ++++++++++++
 MAINTAINERS                                   |   9 +
 drivers/base/Kconfig                          |   8 +
 drivers/base/Makefile                         |   5 +-
 drivers/base/revocable.c                      | 229 ++++++++++++++++++
 drivers/base/revocable_test.c                 | 110 +++++++++
 drivers/platform/chrome/cros_ec.c             |   5 +
 drivers/platform/chrome/cros_ec_chardev.c     | 124 +++++++---
 include/linux/platform_data/cros_ec_proto.h   |   4 +
 include/linux/revocable.h                     |  37 +++
 tools/testing/selftests/Makefile              |   1 +
 .../selftests/drivers/base/revocable/Makefile |   7 +
 .../drivers/base/revocable/revocable_test.c   | 116 +++++++++
 .../drivers/base/revocable/test-revocable.sh  |  39 +++
 .../base/revocable/test_modules/Makefile      |  10 +
 .../revocable/test_modules/revocable_test.c   | 188 ++++++++++++++
 17 files changed, 1003 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/driver-api/driver-model/revocable.rst
 create mode 100644 drivers/base/revocable.c
 create mode 100644 drivers/base/revocable_test.c
 create mode 100644 include/linux/revocable.h
 create mode 100644 tools/testing/selftests/drivers/base/revocable/Makefile
 create mode 100644 tools/testing/selftests/drivers/base/revocable/revocable_test.c
 create mode 100755 tools/testing/selftests/drivers/base/revocable/test-revocable.sh
 create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
 create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c

-- 
2.51.0.384.g4c02a37b29-goog

[PATCH v3 1/5] revocable: Revocable resource management

[Tzung-Bi Shih]

Some resources can be removed asynchronously, for example, resources
provided by a hot-pluggable device like USB.  When holding a reference
to such a resource, it's possible for the resource to be removed and
its memory freed, leading to use-after-free errors on subsequent access.

Introduce the revocable to establish weak references to such resources.
It allows a resource consumer to safely attempt to access a resource
that might be freed at any time by the resource provider.

The implementation uses a provider/consumer model built on Sleepable
RCU (SRCU) to guarantee safe memory access:

 - A resource provider allocates a struct revocable_provider and
   initializes it with a pointer to the resource.

 - A resource consumer that wants to access the resource allocates a
   struct revocable which holds a reference to the provider.

 - To access the resource, the consumer uses revocable_try_access().
   This function enters an SRCU read-side critical section and returns
   the pointer to the resource.  If the provider has already freed the
   resource, it returns NULL.  After use, the consumer calls
   revocable_release() to exit the SRCU critical section.  The
   REVOCABLE() is a convenient helper for doing that.

 - When the provider needs to remove the resource, it calls
   revocable_provider_free().  This function sets the internal resource
   pointer to NULL and then calls synchronize_srcu() to wait for all
   current readers to finish before the resource can be completely torn
   down.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v3:
- No changes.

v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-2-tzungbi@kernel.org
- Rename "ref_proxy" -> "revocable".
- Add introduction in kernel-doc format in revocable.c.
- Add MAINTAINERS entry.
- Add copyright.
- Move from lib/ to drivers/base/.
- EXPORT_SYMBOL() -> EXPORT_SYMBOL_GPL().
- Add Documentation/.
- Rename _get() -> try_access(); _put() -> release().
- Fix a sparse warning by removing the redundant __rcu annotations.
- Fix a sparse warning by adding __acquires() and __releases() annotations.

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

 .../driver-api/driver-model/index.rst         |   1 +
 .../driver-api/driver-model/revocable.rst     | 151 ++++++++++++
 MAINTAINERS                                   |   7 +
 drivers/base/Makefile                         |   2 +-
 drivers/base/revocable.c                      | 229 ++++++++++++++++++
 include/linux/revocable.h                     |  37 +++
 6 files changed, 426 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/driver-api/driver-model/revocable.rst
 create mode 100644 drivers/base/revocable.c
 create mode 100644 include/linux/revocable.h

diff --git a/Documentation/driver-api/driver-model/index.rst b/Documentation/driver-api/driver-model/index.rst
index 4831bdd92e5c..8e1ee21185df 100644
--- a/Documentation/driver-api/driver-model/index.rst
+++ b/Documentation/driver-api/driver-model/index.rst
@@ -14,6 +14,7 @@ Driver Model
    overview
    platform
    porting
+   revocable
 
 .. only::  subproject and html
 
diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst
new file mode 100644
index 000000000000..b9e2968ba9c1
--- /dev/null
+++ b/Documentation/driver-api/driver-model/revocable.rst
@@ -0,0 +1,151 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==============================
+Revocable Resource Management
+==============================
+
+Overview
+========
+
+In a system with hot-pluggable devices, such as USB, resources provided by
+these devices can be removed asynchronously.  If a consumer holds a reference
+to such a resource, the resource might be deallocated while the reference is
+still held, leading to use-after-free errors upon subsequent access.
+
+The "revocable" mechanism addresses this by establishing a weak reference to a
+resource that might be freed at any time.  It allows a resource consumer to
+safely attempt to access the resource, guaranteeing that the access is valid
+for the duration of its use, or it fails safely if the resource has already
+been revoked.
+
+The implementation is based on a provider/consumer model that uses Sleepable
+RCU (SRCU) to ensure safe memory access without traditional locking.
+
+How It Works
+============
+
+1.  **Provider**: A resource provider, such as a driver for a hot-pluggable
+    device, allocates a ``struct revocable_provider``.  This structure is
+    initialized with a pointer to the actual resource it manages.
+
+2.  **Consumer**: A consumer that needs to access the resource is given a
+    ``struct revocable``, which acts as a handle containing a reference to
+    the provider.
+
+3.  **Accessing the Resource**: To access the resource, the consumer uses
+    ``revocable_try_access()``.  This function enters an SRCU read-side
+    critical section and returns a pointer to the resource.  If the provider
+    has already revoked the resource, this function returns ``NULL``.  The
+    consumer must check for this ``NULL`` return.
+
+4.  **Releasing the Resource**: After the consumer has finished using the
+    resource, it must call ``revocable_release()`` to exit the SRCU critical
+    section.  This signals that the consumer no longer requires access.  The
+    ``REVOCABLE()`` macro is provided as a convenient and safe way to manage
+    the access-release cycle.
+
+5.  **Revoking the Resource**: When the provider needs to remove the resource
+    (e.g., the device is unplugged), it calls ``revocable_provider_free()``.
+    This function first sets the internal resource pointer to ``NULL``,
+    preventing any new consumers from accessing it.  It then calls
+    ``synchronize_srcu()``, which waits for all existing consumers currently
+    in the SRCU critical section to finish their work.  Once all consumers
+    have released their access, the resource can be safely deallocated.
+
+Revocable vs. Device-Managed (devm) Resources
+=============================================
+
+It's important to understand the distinction between a standard
+device-managed (devm) resource and a resource managed by a
+``revocable_provider``.
+
+The key difference is their lifetime:
+
+*   A **devm resource** is tied to the lifetime of the device.  It is
+    automatically freed when the device is unbound.
+*   A **revocable_provider** persists as long as there are active references
+    to it from ``revocable`` consumer handles.
+
+This means that a ``revocable_provider`` can outlive the device that created
+it.  This is a deliberate design feature, allowing consumers to hold a
+reference to a resource even after the underlying device has been removed,
+without causing a fault.  When the consumer attempts to access the resource,
+it will simply be informed that the resource is no longer available.
+
+API and Usage
+=============
+
+For Resource Providers
+----------------------
+
+``struct revocable_provider *revocable_provider_alloc(void *res);``
+    Allocates a provider handle for the given resource ``res``.  It returns a
+    pointer to the ``revocable_provider`` on success, or ``NULL`` on failure.
+
+``struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, void *res);``
+    A device-managed version of ``revocable_provider_alloc``.  It is
+    convenient to allocate providers via this function if the ``res`` is also
+    tied to the lifetime of the ``dev``.  ``revocable_provider_free`` will be
+    called automatically when the device is unbound.
+
+``void revocable_provider_free(struct revocable_provider *rp);``
+    Revokes the resource.  This function marks the resource as unavailable and
+    waits for all current consumers to finish before the underlying memory
+    can be freed.
+
+For Resource Consumers
+----------------------
+
+``struct revocable *revocable_alloc(struct revocable_provider *rp);``
+    Allocates a consumer handle for a given provider ``rp``.
+
+``void revocable_free(struct revocable *rev);``
+    Frees a consumer handle.
+
+``void *revocable_try_access(struct revocable *rev);``
+    Attempts to gain access to the resource.  Returns a pointer to the
+    resource on success or ``NULL`` if it has been revoked.
+
+``void revocable_release(struct revocable *rev);``
+    Releases access to the resource, exiting the SRCU critical section.
+
+The ``REVOCABLE()`` Macro
+=========================
+
+The ``REVOCABLE()`` macro simplifies the access-release cycle for consumers,
+ensuring that ``revocable_release()`` is always called, even in the case of
+an early exit.
+
+``REVOCABLE(rev, res)``
+    *   ``rev``: The consumer's ``struct revocable *`` handle.
+    *   ``res``: A pointer variable that will be assigned the resource.
+
+The macro creates a ``for`` loop that executes exactly once.  Inside the loop,
+``res`` is populated with the result of ``revocable_try_access()``.  The
+consumer code **must** check if ``res`` is ``NULL`` before using it.  The
+``revocable_release()`` function is automatically called when the scope of
+the loop is exited.
+
+Example Usage
+-------------
+
+.. code-block:: c
+
+    void consumer_use_resource(struct revocable *rev)
+    {
+        struct foo_resource *res;
+
+        REVOCABLE(rev, res) {
+            // Always check if the resource is valid.
+            if (!res) {
+                pr_warn("Resource is not available\n");
+                return;
+            }
+
+            // At this point, 'res' is guaranteed to be valid until
+            // this block exits.
+            do_something_with(res);
+        }
+
+        // revocable_release() is automatically called here.
+    }
diff --git a/MAINTAINERS b/MAINTAINERS
index fa7f80bd7b2f..5d11aeeb546e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21877,6 +21877,13 @@ F:	include/uapi/linux/rseq.h
 F:	kernel/rseq.c
 F:	tools/testing/selftests/rseq/
 
+REVOCABLE RESOURCE MANAGEMENT
+M:	Tzung-Bi Shih <tzungbi@kernel.org>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	drivers/base/revocable.c
+F:	include/linux/revocable.h
+
 RFKILL
 M:	Johannes Berg <johannes@sipsolutions.net>
 L:	linux-wireless@vger.kernel.org
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 8074a10183dc..bdf854694e39 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -6,7 +6,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
 			   topology.o container.o property.o cacheinfo.o \
-			   swnode.o faux.o
+			   swnode.o faux.o revocable.o
 obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
new file mode 100644
index 000000000000..80a48896b241
--- /dev/null
+++ b/drivers/base/revocable.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google LLC
+ *
+ * Revocable resource management
+ */
+
+#include <linux/device.h>
+#include <linux/kref.h>
+#include <linux/revocable.h>
+#include <linux/slab.h>
+#include <linux/srcu.h>
+
+/**
+ * DOC: Overview
+ *
+ * Some resources can be removed asynchronously, for example, resources
+ * provided by a hot-pluggable device like USB.  When holding a reference
+ * to such a resource, it's possible for the resource to be removed and
+ * its memory freed, leading to use-after-free errors on subsequent access.
+ *
+ * Introduce the revocable to establish weak references to such resources.
+ * It allows a resource consumer to safely attempt to access a resource
+ * that might be freed at any time by the resource provider.
+ *
+ * The implementation uses a provider/consumer model built on Sleepable
+ * RCU (SRCU) to guarantee safe memory access:
+ *
+ * - A resource provider allocates a struct revocable_provider and
+ *   initializes it with a pointer to the resource.
+ *
+ * - A resource consumer that wants to access the resource allocates a
+ *   struct revocable which holds a reference to the provider.
+ *
+ * - To access the resource, the consumer uses revocable_try_access().
+ *   This function enters an SRCU read-side critical section and returns
+ *   the pointer to the resource.  If the provider has already freed the
+ *   resource, it returns NULL.  After use, the consumer calls
+ *   revocable_release() to exit the SRCU critical section.  The
+ *   REVOCABLE() is a convenient helper for doing that.
+ *
+ * - When the provider needs to remove the resource, it calls
+ *   revocable_provider_free().  This function sets the internal resource
+ *   pointer to NULL and then calls synchronize_srcu() to wait for all
+ *   current readers to finish before the resource can be completely torn
+ *   down.
+ */
+
+/**
+ * struct revocable_provider - A handle for resource provider.
+ * @srcu: The SRCU to protect the resource.
+ * @res:  The pointer of resource.  It can point to anything.
+ * @kref: The refcount for this handle.
+ */
+struct revocable_provider {
+	struct srcu_struct srcu;
+	void __rcu *res;
+	struct kref kref;
+};
+
+/**
+ * struct revocable - A handle for resource consumer.
+ * @rp: The pointer of resource provider.
+ * @idx: The index for the RCU critical section.
+ */
+struct revocable {
+	struct revocable_provider *rp;
+	int idx;
+};
+
+/**
+ * revocable_provider_alloc() - Allocate struct revocable_provider.
+ * @res: The pointer of resource.
+ *
+ * This holds an initial refcount to the struct.
+ *
+ * Return: The pointer of struct revocable_provider.  NULL on errors.
+ */
+struct revocable_provider *revocable_provider_alloc(void *res)
+{
+	struct revocable_provider *rp;
+
+	rp = kzalloc(sizeof(*rp), GFP_KERNEL);
+	if (!rp)
+		return NULL;
+
+	init_srcu_struct(&rp->srcu);
+	rcu_assign_pointer(rp->res, res);
+	synchronize_srcu(&rp->srcu);
+	kref_init(&rp->kref);
+
+	return rp;
+}
+EXPORT_SYMBOL_GPL(revocable_provider_alloc);
+
+static void revocable_provider_release(struct kref *kref)
+{
+	struct revocable_provider *rp = container_of(kref,
+			struct revocable_provider, kref);
+
+	cleanup_srcu_struct(&rp->srcu);
+	kfree(rp);
+}
+
+/**
+ * revocable_provider_free() - Free struct revocable_provider.
+ * @rp: The pointer of resource provider.
+ *
+ * This sets the resource `(struct revocable_provider *)->res` to NULL to
+ * indicate the resource has gone.
+ *
+ * This drops the refcount to the resource provider.  If it is the final
+ * reference, revocable_provider_release() will be called to free the struct.
+ */
+void revocable_provider_free(struct revocable_provider *rp)
+{
+	rcu_assign_pointer(rp->res, NULL);
+	synchronize_srcu(&rp->srcu);
+	kref_put(&rp->kref, revocable_provider_release);
+}
+EXPORT_SYMBOL_GPL(revocable_provider_free);
+
+static void devm_revocable_provider_free(void *data)
+{
+	struct revocable_provider *rp = data;
+
+	revocable_provider_free(rp);
+}
+
+/**
+ * devm_revocable_provider_alloc() - Dev-managed revocable_provider_alloc().
+ * @dev: The device.
+ * @res: The pointer of resource.
+ *
+ * It is convenient to allocate providers via this function if the @res is
+ * also tied to the lifetime of the @dev.  revocable_provider_free() will
+ * be called automatically when the device is unbound.
+ *
+ * This holds an initial refcount to the struct.
+ *
+ * Return: The pointer of struct revocable_provider.  NULL on errors.
+ */
+struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
+							 void *res)
+{
+	struct revocable_provider *rp;
+
+	rp = revocable_provider_alloc(res);
+	if (!rp)
+		return NULL;
+
+	if (devm_add_action_or_reset(dev, devm_revocable_provider_free, rp))
+		return NULL;
+
+	return rp;
+}
+EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc);
+
+/**
+ * revocable_alloc() - Allocate struct revocable_provider.
+ * @rp: The pointer of resource provider.
+ *
+ * This holds a refcount to the resource provider.
+ *
+ * Return: The pointer of struct revocable_provider.  NULL on errors.
+ */
+struct revocable *revocable_alloc(struct revocable_provider *rp)
+{
+	struct revocable *rev;
+
+	rev = kzalloc(sizeof(*rev), GFP_KERNEL);
+	if (!rev)
+		return NULL;
+
+	rev->rp = rp;
+	kref_get(&rp->kref);
+
+	return rev;
+}
+EXPORT_SYMBOL_GPL(revocable_alloc);
+
+/**
+ * revocable_free() - Free struct revocable.
+ * @rev: The pointer of struct revocable.
+ *
+ * This drops a refcount to the resource provider.  If it is the final
+ * reference, revocable_provider_release() will be called to free the struct.
+ */
+void revocable_free(struct revocable *rev)
+{
+	struct revocable_provider *rp = rev->rp;
+
+	kref_put(&rp->kref, revocable_provider_release);
+	kfree(rev);
+}
+EXPORT_SYMBOL_GPL(revocable_free);
+
+/**
+ * revocable_try_access() - Try to access the resource.
+ * @rev: The pointer of struct revocable.
+ *
+ * This tries to de-reference to the resource and enters a RCU critical
+ * section.
+ *
+ * Return: The pointer to the resource.  NULL if the resource has gone.
+ */
+void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu)
+{
+	struct revocable_provider *rp = rev->rp;
+
+	rev->idx = srcu_read_lock(&rp->srcu);
+	return rcu_dereference(rp->res);
+}
+EXPORT_SYMBOL_GPL(revocable_try_access);
+
+/**
+ * revocable_release() - Stop accessing to the resource.
+ * @rev: The pointer of struct revocable.
+ *
+ * Call this function to indicate the resource is no longer used.  It exits
+ * the RCU critical section.
+ */
+void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu)
+{
+	struct revocable_provider *rp = rev->rp;
+
+	srcu_read_unlock(&rp->srcu, rev->idx);
+}
+EXPORT_SYMBOL_GPL(revocable_release);
diff --git a/include/linux/revocable.h b/include/linux/revocable.h
new file mode 100644
index 000000000000..17d9b7ce633d
--- /dev/null
+++ b/include/linux/revocable.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2025 Google LLC
+ */
+
+#ifndef __LINUX_REVOCABLE_H
+#define __LINUX_REVOCABLE_H
+
+#include <linux/cleanup.h>
+
+struct device;
+struct revocable;
+struct revocable_provider;
+
+struct revocable_provider *revocable_provider_alloc(void *res);
+void revocable_provider_free(struct revocable_provider *rp);
+struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
+							 void *res);
+
+struct revocable *revocable_alloc(struct revocable_provider *rp);
+void revocable_free(struct revocable *rev);
+void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu);
+void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu);
+
+DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
+
+#define _REVOCABLE(_rev, _label, _res)						\
+	for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev;	\
+	     (_res = revocable_try_access(_rev)) || true; ({ goto _label; }))	\
+		if (0) {							\
+_label:										\
+			break;							\
+		} else
+
+#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
+
+#endif /* __LINUX_REVOCABLE_H */
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH v3 2/5] revocable: Add Kunit test cases

[Tzung-Bi Shih]

Add Kunit test cases for the revocable API.

The test cases cover the following scenarios:
- Basic: Verifies that a consumer can successfully access the resource
  provided via the provider.
- Revocation: Verifies that after the provider revokes the resource,
  the consumer correctly receives a NULL pointer on a subsequent access.
- Macro: Same as "Revocation" but uses the REVOCABLE().

A way to run the test:
$ ./tools/testing/kunit/kunit.py run \
        --kconfig_add CONFIG_REVOCABLE_KUNIT_TEST=y \
        revocable_test

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v3:
- No changes.

v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-3-tzungbi@kernel.org
- New in the series.

 MAINTAINERS                   |   1 +
 drivers/base/Kconfig          |   8 +++
 drivers/base/Makefile         |   3 +
 drivers/base/revocable_test.c | 110 ++++++++++++++++++++++++++++++++++
 4 files changed, 122 insertions(+)
 create mode 100644 drivers/base/revocable_test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d11aeeb546e..74930474f1bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21882,6 +21882,7 @@ M:	Tzung-Bi Shih <tzungbi@kernel.org>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	drivers/base/revocable.c
+F:	drivers/base/revocable_test.c
 F:	include/linux/revocable.h
 
 RFKILL
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 064eb52ff7e2..a6488035f63d 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -244,3 +244,11 @@ config FW_DEVLINK_SYNC_STATE_TIMEOUT
 	  work on.
 
 endmenu
+
+# Kunit test cases
+config REVOCABLE_KUNIT_TEST
+	tristate "Kunit tests for revocable" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Kunit tests for the revocable API.
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index bdf854694e39..4185aaa9bbb9 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -35,3 +35,6 @@ ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 # define_trace.h needs to know how to find our header
 CFLAGS_trace.o		:= -I$(src)
 obj-$(CONFIG_TRACING)	+= trace.o
+
+# Kunit test cases
+obj-$(CONFIG_REVOCABLE_KUNIT_TEST)	+= revocable_test.o
diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
new file mode 100644
index 000000000000..d1ec7e47cd1d
--- /dev/null
+++ b/drivers/base/revocable_test.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google LLC
+ *
+ * Kunit tests for the revocable API.
+ *
+ * The test cases cover the following scenarios:
+ *
+ * - Basic: Verifies that a consumer can successfully access the resource
+ *   provided via the provider.
+ *
+ * - Revocation: Verifies that after the provider revokes the resource,
+ *   the consumer correctly receives a NULL pointer on a subsequent access.
+ *
+ * - Macro: Same as "Revocation" but uses the REVOCABLE().
+ */
+
+#include <kunit/test.h>
+#include <linux/revocable.h>
+
+static void revocable_test_basic(struct kunit *test)
+{
+	struct revocable_provider *rp;
+	struct revocable *rev;
+	void *real_res = (void *)0x12345678, *res;
+
+	rp = revocable_provider_alloc(real_res);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
+
+	rev = revocable_alloc(rp);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
+
+	res = revocable_try_access(rev);
+	KUNIT_EXPECT_PTR_EQ(test, res, real_res);
+	revocable_release(rev);
+
+	revocable_free(rev);
+	revocable_provider_free(rp);
+}
+
+static void revocable_test_revocation(struct kunit *test)
+{
+	struct revocable_provider *rp;
+	struct revocable *rev;
+	void *real_res = (void *)0x12345678, *res;
+
+	rp = revocable_provider_alloc(real_res);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
+
+	rev = revocable_alloc(rp);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
+
+	res = revocable_try_access(rev);
+	KUNIT_EXPECT_PTR_EQ(test, res, real_res);
+	revocable_release(rev);
+
+	revocable_provider_free(rp);
+
+	res = revocable_try_access(rev);
+	KUNIT_EXPECT_PTR_EQ(test, res, NULL);
+	revocable_release(rev);
+
+	revocable_free(rev);
+}
+
+static void revocable_test_macro(struct kunit *test)
+{
+	struct revocable_provider *rp;
+	struct revocable *rev;
+	void *real_res = (void *)0x12345678, *res;
+	bool accessed;
+
+	rp = revocable_provider_alloc(real_res);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
+
+	rev = revocable_alloc(rp);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
+
+	accessed = false;
+	REVOCABLE(rev, res) {
+		KUNIT_EXPECT_PTR_EQ(test, res, real_res);
+		accessed = true;
+	}
+	KUNIT_EXPECT_TRUE(test, accessed);
+
+	revocable_provider_free(rp);
+
+	accessed = false;
+	REVOCABLE(rev, res) {
+		KUNIT_EXPECT_PTR_EQ(test, res, NULL);
+		accessed = true;
+	}
+	KUNIT_EXPECT_TRUE(test, accessed);
+
+	revocable_free(rev);
+}
+
+static struct kunit_case revocable_test_cases[] = {
+	KUNIT_CASE(revocable_test_basic),
+	KUNIT_CASE(revocable_test_revocation),
+	KUNIT_CASE(revocable_test_macro),
+	{}
+};
+
+static struct kunit_suite revocable_test_suite = {
+	.name = "revocable_test",
+	.test_cases = revocable_test_cases,
+};
+
+kunit_test_suite(revocable_test_suite);
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH v3 3/5] selftests: revocable: Add kselftest cases

[Tzung-Bi Shih]

Add kselftest cases for the revocable API.

The test consists of three parts:
- A kernel module (revocable_test.ko) that creates a debugfs interface
  with `/provider` and `/consumer` files.
- A user-space C program (revocable_test) that uses the kselftest
  harness to interact with the debugfs files.
- An orchestrating shell script (test-revocable.sh) that loads the
  module, runs the C program, and unloads the module.

The test cases cover the following scenarios:
- Basic: Verifies that a consumer can successfully access the resource
  provided via the provider.
- Revocation: Verifies that after the provider revokes the resource,
  the consumer correctly receives a NULL pointer on a subsequent access.
- Macro: Same as "Revocation" but uses the REVOCABLE().

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v3:
- No changes.

v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-4-tzungbi@kernel.org
- New in the series.

 MAINTAINERS                                   |   1 +
 tools/testing/selftests/Makefile              |   1 +
 .../selftests/drivers/base/revocable/Makefile |   7 +
 .../drivers/base/revocable/revocable_test.c   | 116 +++++++++++
 .../drivers/base/revocable/test-revocable.sh  |  39 ++++
 .../base/revocable/test_modules/Makefile      |  10 +
 .../revocable/test_modules/revocable_test.c   | 188 ++++++++++++++++++
 7 files changed, 362 insertions(+)
 create mode 100644 tools/testing/selftests/drivers/base/revocable/Makefile
 create mode 100644 tools/testing/selftests/drivers/base/revocable/revocable_test.c
 create mode 100755 tools/testing/selftests/drivers/base/revocable/test-revocable.sh
 create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
 create mode 100644 tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 74930474f1bd..583d818262c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21884,6 +21884,7 @@ S:	Maintained
 F:	drivers/base/revocable.c
 F:	drivers/base/revocable_test.c
 F:	include/linux/revocable.h
+F:	tools/testing/selftests/drivers/base/revocable/
 
 RFKILL
 M:	Johannes Berg <johannes@sipsolutions.net>
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 850dacb09929..d9700ce9eb64 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -17,6 +17,7 @@ TARGETS += damon
 TARGETS += devices/error_logs
 TARGETS += devices/probe
 TARGETS += dmabuf-heaps
+TARGETS += drivers/base/revocable
 TARGETS += drivers/dma-buf
 TARGETS += drivers/ntsync
 TARGETS += drivers/s390x/uvdevice
diff --git a/tools/testing/selftests/drivers/base/revocable/Makefile b/tools/testing/selftests/drivers/base/revocable/Makefile
new file mode 100644
index 000000000000..afa5ca0fa452
--- /dev/null
+++ b/tools/testing/selftests/drivers/base/revocable/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+TEST_GEN_MODS_DIR := test_modules
+TEST_GEN_PROGS_EXTENDED := revocable_test
+TEST_PROGS := test-revocable.sh
+
+include ../../../lib.mk
diff --git a/tools/testing/selftests/drivers/base/revocable/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/revocable_test.c
new file mode 100644
index 000000000000..cdef49dc4095
--- /dev/null
+++ b/tools/testing/selftests/drivers/base/revocable/revocable_test.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google LLC
+ *
+ * A selftest for the revocable API.
+ *
+ * The test cases cover the following scenarios:
+ *
+ * - Basic: Verifies that a consumer can successfully access the resource
+ *   provided via the provider.
+ *
+ * - Revocation: Verifies that after the provider revokes the resource,
+ *   the consumer correctly receives a NULL pointer on a subsequent access.
+ *
+ * - Macro: Same as "Revocation" but uses the REVOCABLE().
+ */
+
+#include <fcntl.h>
+#include <unistd.h>
+
+#include "../../../kselftest_harness.h"
+
+#define DEBUGFS_PATH "/sys/kernel/debug/revocable_test"
+#define TEST_CMD_RESOURCE_GONE "resource_gone"
+#define TEST_DATA "12345678"
+#define TEST_MAGIC_OFFSET 0x1234
+
+FIXTURE(revocable_fixture) {
+	int pfd;
+	int cfd;
+};
+
+FIXTURE_SETUP(revocable_fixture) {
+	int ret;
+
+	self->pfd = open(DEBUGFS_PATH "/provider", O_WRONLY);
+	ASSERT_NE(-1, self->pfd)
+		TH_LOG("failed to open provider fd");
+
+	ret = write(self->pfd, TEST_DATA, strlen(TEST_DATA));
+	ASSERT_NE(-1, ret) {
+		close(self->pfd);
+		TH_LOG("failed to write test data");
+	}
+
+	self->cfd = open(DEBUGFS_PATH "/consumer", O_RDONLY);
+	ASSERT_NE(-1, self->cfd)
+		TH_LOG("failed to open consumer fd");
+}
+
+FIXTURE_TEARDOWN(revocable_fixture) {
+	close(self->cfd);
+	close(self->pfd);
+}
+
+/*
+ * ASSERT_* is only available in TEST or TEST_F block.  Use
+ * macro for the helper.
+ */
+#define READ_TEST_DATA(_fd, _offset, _data, _msg)			\
+	do {								\
+		int ret;						\
+									\
+		ret = lseek(_fd, _offset, SEEK_SET);			\
+		ASSERT_NE(-1, ret)					\
+			TH_LOG("failed to lseek");			\
+									\
+		ret = read(_fd, _data, sizeof(_data) - 1);		\
+		ASSERT_NE(-1, ret)					\
+			TH_LOG(_msg);					\
+		data[ret] = '\0';					\
+	} while (0)
+
+TEST_F(revocable_fixture, basic) {
+	char data[16];
+
+	READ_TEST_DATA(self->cfd, 0, data, "failed to read test data");
+	EXPECT_STREQ(TEST_DATA, data);
+}
+
+TEST_F(revocable_fixture, revocation) {
+	char data[16];
+	int ret;
+
+	READ_TEST_DATA(self->cfd, 0, data, "failed to read test data");
+	EXPECT_STREQ(TEST_DATA, data);
+
+	ret = write(self->pfd, TEST_CMD_RESOURCE_GONE,
+		    strlen(TEST_CMD_RESOURCE_GONE));
+	ASSERT_NE(-1, ret)
+		TH_LOG("failed to write resource gone cmd");
+
+	READ_TEST_DATA(self->cfd, 0, data,
+		       "failed to read test data after resource gone");
+	EXPECT_STREQ("(null)", data);
+}
+
+TEST_F(revocable_fixture, macro) {
+	char data[16];
+	int ret;
+
+	READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET, data,
+		       "failed to read test data");
+	EXPECT_STREQ(TEST_DATA, data);
+
+	ret = write(self->pfd, TEST_CMD_RESOURCE_GONE,
+		    strlen(TEST_CMD_RESOURCE_GONE));
+	ASSERT_NE(-1, ret)
+		TH_LOG("failed to write resource gone cmd");
+
+	READ_TEST_DATA(self->cfd, TEST_MAGIC_OFFSET, data,
+		       "failed to read test data after resource gone");
+	EXPECT_STREQ("(null)", data);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/drivers/base/revocable/test-revocable.sh b/tools/testing/selftests/drivers/base/revocable/test-revocable.sh
new file mode 100755
index 000000000000..3a34be28001a
--- /dev/null
+++ b/tools/testing/selftests/drivers/base/revocable/test-revocable.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+mod_name="revocable_test"
+ksft_fail=1
+ksft_skip=4
+
+if [ "$(id -u)" -ne 0 ]; then
+	echo "$0: Must be run as root"
+	exit "$ksft_skip"
+fi
+
+if ! which insmod > /dev/null 2>&1; then
+	echo "$0: Need insmod"
+	exit "$ksft_skip"
+fi
+
+if ! which rmmod > /dev/null 2>&1; then
+	echo "$0: Need rmmod"
+	exit "$ksft_skip"
+fi
+
+insmod test_modules/"${mod_name}".ko
+
+if [ ! -d /sys/kernel/debug/revocable_test/ ]; then
+	mount -t debugfs none /sys/kernel/debug/
+
+	if [ ! -d /sys/kernel/debug/revocable_test/ ]; then
+		echo "$0: Error mounting debugfs"
+		exit "$ksft_fail"
+	fi
+fi
+
+./revocable_test
+ret=$?
+
+rmmod "${mod_name}"
+
+exit "${ret}"
diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile b/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
new file mode 100644
index 000000000000..f29e4f909402
--- /dev/null
+++ b/tools/testing/selftests/drivers/base/revocable/test_modules/Makefile
@@ -0,0 +1,10 @@
+TESTMODS_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST)))))
+KDIR ?= /lib/modules/$(shell uname -r)/build
+
+obj-m += revocable_test.o
+
+all:
+	$(Q)$(MAKE) -C $(KDIR) M=$(TESTMODS_DIR)
+
+clean:
+	$(Q)$(MAKE) -C $(KDIR) M=$(TESTMODS_DIR) clean
diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
new file mode 100644
index 000000000000..9166b860a55a
--- /dev/null
+++ b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google LLC
+ *
+ * A kernel module for testing the revocable API.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/module.h>
+#include <linux/revocable.h>
+#include <linux/slab.h>
+
+#define TEST_CMD_RESOURCE_GONE "resource_gone"
+#define TEST_MAGIC_OFFSET 0x1234
+
+static struct dentry *debugfs_dir;
+
+struct revocable_test_provider_priv {
+	struct revocable_provider *rp;
+	struct dentry *dentry;
+	char res[16];
+};
+
+static int revocable_test_consumer_open(struct inode *inode, struct file *filp)
+{
+	struct revocable *rev;
+	struct revocable_provider *rp = inode->i_private;
+
+	rev = revocable_alloc(rp);
+	if (!rev)
+		return -ENOMEM;
+	filp->private_data = rev;
+
+	return 0;
+}
+
+static int revocable_test_consumer_release(struct inode *inode,
+					   struct file *filp)
+{
+	struct revocable *rev = filp->private_data;
+
+	revocable_free(rev);
+	return 0;
+}
+
+static ssize_t revocable_test_consumer_read(struct file *filp,
+					    char __user *buf,
+					    size_t count, loff_t *offset)
+{
+	char *res;
+	char data[16];
+	size_t len;
+	struct revocable *rev = filp->private_data;
+
+	switch (*offset) {
+	case 0:
+		res = revocable_try_access(rev);
+		snprintf(data, sizeof(data), "%s", res ?: "(null)");
+		revocable_release(rev);
+		break;
+	case TEST_MAGIC_OFFSET:
+		REVOCABLE(rev, res)
+			snprintf(data, sizeof(data), "%s", res ?: "(null)");
+		break;
+	default:
+		return 0;
+	}
+
+	len = min_t(size_t, strlen(data), count);
+	if (copy_to_user(buf, data, len))
+		return -EFAULT;
+
+	*offset = len;
+	return len;
+}
+
+static const struct file_operations revocable_test_consumer_fops = {
+	.open = revocable_test_consumer_open,
+	.release = revocable_test_consumer_release,
+	.read = revocable_test_consumer_read,
+	.llseek = default_llseek,
+};
+
+static int revocable_test_provider_open(struct inode *inode, struct file *filp)
+{
+	struct revocable_test_provider_priv *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	filp->private_data = priv;
+
+	return 0;
+}
+
+static int revocable_test_provider_release(struct inode *inode,
+					   struct file *filp)
+{
+	struct revocable_test_provider_priv *priv = filp->private_data;
+
+	debugfs_remove(priv->dentry);
+	if (priv->rp)
+		revocable_provider_free(priv->rp);
+	kfree(priv);
+
+	return 0;
+}
+
+static ssize_t revocable_test_provider_write(struct file *filp,
+					     const char __user *buf,
+					     size_t count, loff_t *offset)
+{
+	size_t copied;
+	char data[64];
+	struct revocable_test_provider_priv *priv = filp->private_data;
+
+	copied = strncpy_from_user(data, buf, sizeof(data));
+	if (copied < 0)
+		return copied;
+	if (copied == sizeof(data))
+		data[sizeof(data) - 1] = '\0';
+
+	/*
+	 * Note: The test can't just close the FD for signaling the
+	 * resource gone.  Subsequent file operations on the opening
+	 * FD of debugfs return -EIO after calling debugfs_remove().
+	 * See also debugfs_file_get().
+	 *
+	 * Here is a side command channel for signaling the resource
+	 * gone.
+	 */
+	if (!strcmp(data, TEST_CMD_RESOURCE_GONE)) {
+		revocable_provider_free(priv->rp);
+		priv->rp = NULL;
+	} else {
+		if (priv->res[0] != '\0')
+			return 0;
+
+		strscpy(priv->res, data);
+
+		priv->rp = revocable_provider_alloc(&priv->res);
+		if (!priv->rp)
+			return -ENOMEM;
+
+		priv->dentry = debugfs_create_file("consumer", 0400,
+						   debugfs_dir, priv->rp,
+						   &revocable_test_consumer_fops);
+		if (!priv->dentry) {
+			revocable_provider_free(priv->rp);
+			return -ENOMEM;
+		}
+	}
+
+	return copied;
+}
+
+static const struct file_operations revocable_test_provider_fops = {
+	.open = revocable_test_provider_open,
+	.release = revocable_test_provider_release,
+	.write = revocable_test_provider_write,
+};
+
+static int __init revocable_test_init(void)
+{
+	debugfs_dir = debugfs_create_dir("revocable_test", NULL);
+	if (!debugfs_dir)
+		return -ENOMEM;
+
+	if (!debugfs_create_file("provider", 0200, debugfs_dir, NULL,
+				 &revocable_test_provider_fops)) {
+		debugfs_remove_recursive(debugfs_dir);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void __exit revocable_test_exit(void)
+{
+	debugfs_remove_recursive(debugfs_dir);
+}
+
+module_init(revocable_test_init);
+module_exit(revocable_test_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tzung-Bi Shih <tzungbi@kernel.org>");
+MODULE_DESCRIPTION("Revocable Kselftest");
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH v3 4/5] platform/chrome: Protect cros_ec_device lifecycle with revocable

[Tzung-Bi Shih]

The cros_ec_device can be unregistered when the underlying device is
removed.  Other kernel drivers that interact with the EC may hold a
pointer to the cros_ec_device, creating a risk of a use-after-free
error if the EC device is removed while still being referenced.

To prevent this, leverage the revocable and convert the underlying
device drivers to resource providers of cros_ec_device.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v3:
- Initialize the revocable provider in cros_ec_device_alloc() instead of
  spreading in protocol device drivers.

v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-5-tzungbi@kernel.org
- Rename "ref_proxy" -> "revocable".

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

 drivers/platform/chrome/cros_ec.c           | 5 +++++
 include/linux/platform_data/cros_ec_proto.h | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 1da79e3d215b..95e3e898e3da 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
+#include <linux/revocable.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
 
@@ -47,6 +48,10 @@ struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
 	if (!ec_dev)
 		return NULL;
 
+	ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+	if (!ec_dev->revocable_provider)
+		return NULL;
+
 	ec_dev->din_size = sizeof(struct ec_host_response) +
 			   sizeof(struct ec_response_get_protocol_info) +
 			   EC_MAX_RESPONSE_OVERHEAD;
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index de14923720a5..fbb6ca34a40f 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -12,6 +12,7 @@
 #include <linux/lockdep_types.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
+#include <linux/revocable.h>
 
 #include <linux/platform_data/cros_ec_commands.h>
 
@@ -165,6 +166,7 @@ struct cros_ec_command {
  * @pd: The platform_device used by the mfd driver to interface with the
  *      PD behind an EC.
  * @panic_notifier: EC panic notifier.
+ * @revocable_provider: The revocable_provider to this device.
  */
 struct cros_ec_device {
 	/* These are used by other drivers that want to talk to the EC */
@@ -211,6 +213,8 @@ struct cros_ec_device {
 	struct platform_device *pd;
 
 	struct blocking_notifier_head panic_notifier;
+
+	struct revocable_provider *revocable_provider;
 };
 
 /**
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH v3 5/5] platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable

[Tzung-Bi Shih]

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>
---
v3:
- 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 | 124 +++++++++++++++-------
 1 file changed, 84 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c9d80ad5b57e..2677b756e31c 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,12 +66,19 @@ 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);
-	if (ret < 0) {
-		snprintf(str, maxlen,
-			 "Unknown EC version, returned error: %d\n",
-			 msg->result);
-		goto exit;
+	REVOCABLE(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",
+				 msg->result);
+			goto exit;
+		}
 	}
 
 	resp = (struct ec_response_get_version *)msg->data;
@@ -92,22 +101,30 @@ 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(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)
-		return NOTIFY_DONE;
+		if (!(event_bit & priv->event_mask) ||
+		    (priv->event_len + total_size) > CROS_MAX_EVENT_LEN)
+			return NOTIFY_DONE;
 
-	event = kzalloc(total_size, GFP_KERNEL);
-	if (!event)
-		return NOTIFY_DONE;
+		event = kzalloc(total_size, GFP_KERNEL);
+		if (!event)
+			return NOTIFY_DONE;
 
-	event->size = ec_dev->event_size;
-	event->event_type = ec_dev->event_data.event_type;
-	memcpy(event->data, &ec_dev->event_data.data, ec_dev->event_size);
+		event->size = ec_dev->event_size;
+		event->event_type = ec_dev->event_data.event_type;
+		memcpy(event->data, &ec_dev->event_data.data, ec_dev->event_size);
+	}
 
 	spin_lock(&priv->wait_event.lock);
 	list_add_tail(&event->node, &priv->events);
@@ -166,7 +183,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 +200,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 +278,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(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 +304,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 +331,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(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,24 +352,29 @@ 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;
 
-	/* Not every platform supports direct reads */
-	if (!ec_dev->cmd_readmem)
-		return -ENOTTY;
+	REVOCABLE(priv->ec_dev_rev, ec_dev) {
+		if (!ec_dev)
+			return -ENODEV;
 
-	if (copy_from_user(&s_mem, arg, sizeof(s_mem)))
-		return -EFAULT;
+		/* Not every platform supports direct reads */
+		if (!ec_dev->cmd_readmem)
+			return -ENOTTY;
 
-	if (s_mem.bytes > sizeof(s_mem.buffer))
-		return -EINVAL;
+		if (copy_from_user(&s_mem, arg, sizeof(s_mem)))
+			return -EFAULT;
 
-	num = ec_dev->cmd_readmem(ec_dev, s_mem.offset, s_mem.bytes,
-				  s_mem.buffer);
-	if (num <= 0)
-		return num;
+		if (s_mem.bytes > sizeof(s_mem.buffer))
+			return -EINVAL;
+
+		num = ec_dev->cmd_readmem(ec_dev, s_mem.offset, s_mem.bytes,
+					  s_mem.buffer);
+		if (num <= 0)
+			return num;
+	}
 
 	if (copy_to_user((void __user *)arg, &s_mem, sizeof(s_mem)))
 		return -EFAULT;
-- 
2.51.0.384.g4c02a37b29-goog

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Greg Kroah-Hartman]

On Fri, Sep 12, 2025 at 08:17:12AM +0000, Tzung-Bi Shih wrote:
> This is a follow-up series of [1].  It tries to fix a possible UAF in the
> fops of cros_ec_chardev after the underlying protocol device has gone by
> using revocable.
> 
> The 1st patch introduces the revocable which is an implementation of ideas
> from the talk [2].
> 
> The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
> 
> The 4th patch converts existing protocol devices to resource providers
> of cros_ec_device.
> 
> The 5th patch converts cros_ec_chardev to a resource consumer of
> cros_ec_device to fix the UAF.
> 
> [1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@kernel.org/
> [2] https://lpc.events/event/17/contributions/1627/
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>

This is, frankly, wonderful work.  Thanks so much for doing this, it's
what many of us have been wanting to see for a very long time but none
of us got around to actually doing it.

And it has tests!  And documentation!  Couldn't ask for more.

We can bikeshed about the REVOCABLE() macro name, but frankly, you wrote
it, you get to pick it :)

Laurent, Bartosz, Wolfram, any objection to this series?  I think this
addresses the issues that all of you have been raising for years with
our access of pointers that have different lifecycles from other
structures (i.e. struct cdev from struct device).

Also, Danilo, if you get the chance, can you give this a review as well?
At first glance it looks good to me, but as you wrote the Rust
implementation of this feature, a second pair of eyes would be great to
have if you have the time.

thanks,

greg k-h

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Danilo Krummrich]

On 9/12/25 10:30 AM, Greg Kroah-Hartman wrote:
> Also, Danilo, if you get the chance, can you give this a review as well?
> At first glance it looks good to me, but as you wrote the Rust
> implementation of this feature, a second pair of eyes would be great to
> have if you have the time.

Sure, I will follow up on this later today.

Re: [PATCH v3 1/5] revocable: Revocable resource management

[Danilo Krummrich]

On Fri Sep 12, 2025 at 10:17 AM CEST, Tzung-Bi Shih wrote:
> +/**
> + * struct revocable_provider - A handle for resource provider.
> + * @srcu: The SRCU to protect the resource.
> + * @res:  The pointer of resource.  It can point to anything.
> + * @kref: The refcount for this handle.
> + */
> +struct revocable_provider {
> +	struct srcu_struct srcu;
> +	void __rcu *res;
> +	struct kref kref;
> +};

I think a revocable provider should provide an optional revoke() callback where
users of the revocable provider can release the revoked resource.

But this can also be done as a follow-up.

> +/**
> + * struct revocable - A handle for resource consumer.
> + * @rp: The pointer of resource provider.
> + * @idx: The index for the RCU critical section.
> + */
> +struct revocable {
> +	struct revocable_provider *rp;
> +	int idx;
> +};

I think I asked about this in the previous version (but I don't remember if
there was an answer):

In Rust we get away with a single Revocable<T> structure, but we're using RCU
instead of SRCU. It seems to me that the split between struct revocable and
struct revocable_provider is only for the SRCU index? Or is there any other
reason?

> +/**
> + * revocable_provider_free() - Free struct revocable_provider.
> + * @rp: The pointer of resource provider.
> + *
> + * This sets the resource `(struct revocable_provider *)->res` to NULL to
> + * indicate the resource has gone.
> + *
> + * This drops the refcount to the resource provider.  If it is the final
> + * reference, revocable_provider_release() will be called to free the struct.
> + */
> +void revocable_provider_free(struct revocable_provider *rp)
> +{
> +	rcu_assign_pointer(rp->res, NULL);
> +	synchronize_srcu(&rp->srcu);
> +	kref_put(&rp->kref, revocable_provider_release);
> +}
> +EXPORT_SYMBOL_GPL(revocable_provider_free);

I think naming this "free" is a bit misleading, since what it basically does is

  (1) Revoke access to the resource.

  (2) Drop a reference count of struct revocable_provider.

In a typical application there may still be struct revocable instances that have
a reference to the provider, so we can't claim that it's freed here.

So, given that, I'd rather call this revocable_provider_revoke().

> +static void devm_revocable_provider_free(void *data)
> +{
> +	struct revocable_provider *rp = data;
> +
> +	revocable_provider_free(rp);
> +}

Same here, I'd call this devm_revocable_provider_revoke().

> +DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
> +
> +#define _REVOCABLE(_rev, _label, _res)						\
> +	for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev;	\
> +	     (_res = revocable_try_access(_rev)) || true; ({ goto _label; }))	\
> +		if (0) {							\
> +_label:										\
> +			break;							\
> +		} else
> +
> +#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)

This is basically the same as Revocable::try_access_with() [1] in Rust, i.e.
try to access and run a closure.

Admittedly, REVOCABLE_TRY_ACCESS_WITH() is pretty verbose and I also do not have
a great idea to shorten it.

Maybe you have a good idea, otherwise I'm also fine with the current name.

Otherwise, maybe it's worth to link to the Rust Revocable API for reference?

With *_free() renamed to *_revoke(), feel free to add:

	Acked-by: Danilo Krummrich <dakr@kernel.org>

[1] https://rust.docs.kernel.org/kernel/revocable/struct.Revocable.html#method.try_access_with

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Krzysztof Kozlowski]

On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> This is a follow-up series of [1].  It tries to fix a possible UAF in the
> fops of cros_ec_chardev after the underlying protocol device has gone by
> using revocable.
> 
> The 1st patch introduces the revocable which is an implementation of ideas
> from the talk [2].
> 
> The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
> 
> The 4th patch converts existing protocol devices to resource providers
> of cros_ec_device.
> 
> The 5th patch converts cros_ec_chardev to a resource consumer of
> cros_ec_device to fix the UAF.
> 
> [1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@kernel.org/
> [2] https://lpc.events/event/17/contributions/1627/
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for the work. Just a note, please start using b4, so above Cc
will be propagated to all patches. Folks above received only the cover
letter...

Best regards,
Krzysztof

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 10:30:45AM +0200, Greg KH wrote:
> On Fri, Sep 12, 2025 at 08:17:12AM +0000, Tzung-Bi Shih wrote:
> > This is a follow-up series of [1].  It tries to fix a possible UAF in the
> > fops of cros_ec_chardev after the underlying protocol device has gone by
> > using revocable.
> > 
> > The 1st patch introduces the revocable which is an implementation of ideas
> > from the talk [2].
> > 
> > The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
> > 
> > The 4th patch converts existing protocol devices to resource providers
> > of cros_ec_device.
> > 
> > The 5th patch converts cros_ec_chardev to a resource consumer of
> > cros_ec_device to fix the UAF.
> > 
> > [1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@kernel.org/
> > [2] https://lpc.events/event/17/contributions/1627/
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> This is, frankly, wonderful work.  Thanks so much for doing this, it's
> what many of us have been wanting to see for a very long time but none
> of us got around to actually doing it.
> 
> And it has tests!  And documentation!  Couldn't ask for more.
> 
> We can bikeshed about the REVOCABLE() macro name, but frankly, you wrote
> it, you get to pick it :)
> 
> Laurent, Bartosz, Wolfram, any objection to this series?  I think this
> addresses the issues that all of you have been raising for years with
> our access of pointers that have different lifecycles from other
> structures (i.e. struct cdev from struct device).

I'll check this either later today or over the weekend.

> Also, Danilo, if you get the chance, can you give this a review as well?
> At first glance it looks good to me, but as you wrote the Rust
> implementation of this feature, a second pair of eyes would be great to
> have if you have the time.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Bartosz Golaszewski]

On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> > This is a follow-up series of [1].  It tries to fix a possible UAF in the
> > fops of cros_ec_chardev after the underlying protocol device has gone by
> > using revocable.
> >
> > The 1st patch introduces the revocable which is an implementation of ideas
> > from the talk [2].
> >
> > The 2nd and 3rd patches add test cases for revocable in Kunit and selftest.
> >
> > The 4th patch converts existing protocol devices to resource providers
> > of cros_ec_device.
> >
> > The 5th patch converts cros_ec_chardev to a resource consumer of
> > cros_ec_device to fix the UAF.
> >
> > [1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-6-tzungbi@kernel.org/
> > [2] https://lpc.events/event/17/contributions/1627/
> >
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Thanks for the work. Just a note, please start using b4, so above Cc
> will be propagated to all patches. Folks above received only the cover
> letter...
>

Thanks to Krzysztof for making me aware of this. Could you please Cc
my brgl@bgdev.pl address on the next iteration.

I haven't looked into the details yet but the small size of the first
patch strikes me as odd. The similar changes I did for GPIO were quite
big and they were designed just for a single sub-system.

During the talk you reference, after I suggested a library like this,
Greg KH can be heard saying: do this for two big subsystems so that
you're sure it's a generic solution. Here you're only using it in a
single driver which makes me wonder if we can actually use it to
improve bigger offenders, like for example I2C, or even replace the
custom, SRCU-based solution in GPIO we have now. Have you considered
at least doing a PoC in a wider kernel framework?

Just my two cents.

Bartosz

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Tzung-Bi Shih]

On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
> On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> > Thanks for the work. Just a note, please start using b4, so above Cc
> > will be propagated to all patches. Folks above received only the cover
> > letter...

Thank you for bringing this to my attention.  I wasn't aware of that and
will ensure this is handled correctly in the future.

> Thanks to Krzysztof for making me aware of this. Could you please Cc
> my brgl@bgdev.pl address on the next iteration.

Sure, will do.

> I haven't looked into the details yet but the small size of the first
> patch strikes me as odd. The similar changes I did for GPIO were quite
> big and they were designed just for a single sub-system.
> 
> During the talk you reference, after I suggested a library like this,
> Greg KH can be heard saying: do this for two big subsystems so that
> you're sure it's a generic solution. Here you're only using it in a
> single driver which makes me wonder if we can actually use it to
> improve bigger offenders, like for example I2C, or even replace the
> custom, SRCU-based solution in GPIO we have now. Have you considered
> at least doing a PoC in a wider kernel framework?

Yes, I'm happy to take this on.

To help me get started, could you please point me to some relevant code
locations?  Also, could you let me know if any specific physical devices
will be needed for testing?

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
> On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
> > On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
> > > On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > >
> > > Thanks for the work. Just a note, please start using b4, so above Cc
> > > will be propagated to all patches. Folks above received only the cover
> > > letter...
> 
> Thank you for bringing this to my attention.  I wasn't aware of that and
> will ensure this is handled correctly in the future.
> 
> > Thanks to Krzysztof for making me aware of this. Could you please Cc
> > my brgl@bgdev.pl address on the next iteration.
> 
> Sure, will do.
> 
> > I haven't looked into the details yet but the small size of the first
> > patch strikes me as odd. The similar changes I did for GPIO were quite
> > big and they were designed just for a single sub-system.
> > 
> > During the talk you reference, after I suggested a library like this,
> > Greg KH can be heard saying: do this for two big subsystems so that
> > you're sure it's a generic solution. Here you're only using it in a
> > single driver which makes me wonder if we can actually use it to
> > improve bigger offenders, like for example I2C, or even replace the
> > custom, SRCU-based solution in GPIO we have now. Have you considered
> > at least doing a PoC in a wider kernel framework?
> 
> Yes, I'm happy to take this on.
> 
> To help me get started, could you please point me to some relevant code
> locations?  Also, could you let me know if any specific physical devices
> will be needed for testing?

One interesting test would be to move the logic to the cdev layer. The
use-after-free problem isn't specific to one type of character device,
and so shouldn't require a fix in every driver instantiating a cdev
directly (or indirectly). See [1] for a previous attempt to handle this
at the V4L2 level and [2] for an attempt to handle it at the cdev level.

In [1], two new functions named video_device_enter() and
video_device_exit() flag the beginning and end of protected code
sections. The equivalent in [2] is the manual get/put of cdev->qactive,
and if I understand things correctly, your series creates a REVOCABLE()
macro to do the same. I'm sure we'll bikesheed about names at some
point, but for the time being, what I'd like to see if this being done
in fs/char_dev.c to cover all entry points from userspace at the cdev
level.

We then have video_device_unplug() in [1], which I think is more or less
the equivalent of revocable_provider_free(). I don't think we'll be able
to hide this completely from drivers, at least not in all cases. We
should however design the API to make it easy for drivers, likely with
subsystem-specific wrappers.

What I have in mind is roughly the following:

1. Protect all access to the cdev from userspace with enter/exit calls
   that flag if a call is in progress. This can be done with explicit
   function calls, or with a scope guard as in your series.

2. At .remove() time, start by flagging that the device is being
   removed. That has to be an explicit call from drivers I believe,
   likely using subsystem-specific wrappers to simplify things.

3. Once the device is marked as being removed, all enter() calls should
   fail at the cdev level.

4. In .remove(), proceed to perform driver-specific operations that will
   stop the device and wake up any userspace task blocked on a syscall
   protected by enter()/remove(). This isn't needed for
   drivers/subsystems that don't provide any blocking API, but is
   required otherwise.

5. Unregister, still in .remove(), the cdev (likely through
   subsystem-specific APIs in most cases). This should block until all
   protected sections have exited.

6. The cdev is now unregistered, can't be opened anymore, and any
   new syscall on any opened file handle will return an error. The
   driver's .remove() function can proceed to free data, there won't be
   any UAF caused by userspace.

[1] implemented this fairly naively with flags and spinlocks. An
RCU-based implementation is probably more efficient, even if I don't
know how performance-sensitive all this is.

Does this align with your design, and do you think you could give a try
at pushing revocable resource handling to the cdev level ?

On a separate note, I'm not sure "revocable" is the right name here. I
believe a revocable resource API is needed, and well-named, for
in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the
userspace syscalls racing with .remove(), I don't think we're dealing
with "revocable resources". Now, if a "revocable resources" API were to
support the in-kernel users, and be usable as a building block to fix
the cdev issue, I would have nothing against it, but the "revocable"
name should be internal in that case, used in the cdev layer only, and
not exposed to drivers (or even subsystem helpers that should wrap cdev
functions instead).

[1] https://lore.kernel.org/r/20171116003349.19235-1-laurent.pinchart+renesas@ideasonboard.com
[2] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/5] revocable: Revocable resource management

[Jonathan Corbet]

Tzung-Bi Shih <tzungbi@kernel.org> writes:

> Some resources can be removed asynchronously, for example, resources
> provided by a hot-pluggable device like USB.  When holding a reference
> to such a resource, it's possible for the resource to be removed and
> its memory freed, leading to use-after-free errors on subsequent access.

Far be it from me to complain about a new feature that comes with nice
documentation!  I will make one small observation, though, for
consideration.

We have the document itself:

> diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst
> new file mode 100644
> index 000000000000..b9e2968ba9c1
> --- /dev/null
> +++ b/Documentation/driver-api/driver-model/revocable.rst
> @@ -0,0 +1,151 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==============================
> +Revocable Resource Management
> +==============================
> +
> +Overview
> +========
> +
> +In a system with hot-pluggable devices, such as USB, resources provided by
> +these devices can be removed asynchronously.  If a consumer holds a reference
> +to such a resource, the resource might be deallocated while the reference is
> +still held, leading to use-after-free errors upon subsequent access.
> +
> +The "revocable" mechanism addresses this by establishing a weak reference to a
> +resource that might be freed at any time.  It allows a resource consumer to
> +safely attempt to access the resource, guaranteeing that the access is valid
> +for the duration of its use, or it fails safely if the resource has already
> +been revoked.

[...]

Then there is the in-code documentation:

> diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
> new file mode 100644
> index 000000000000..80a48896b241
> --- /dev/null
> +++ b/drivers/base/revocable.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 Google LLC
> + *
> + * Revocable resource management
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kref.h>
> +#include <linux/revocable.h>
> +#include <linux/slab.h>
> +#include <linux/srcu.h>
> +
> +/**
> + * DOC: Overview
> + *
> + * Some resources can be removed asynchronously, for example, resources
> + * provided by a hot-pluggable device like USB.  When holding a reference
> + * to such a resource, it's possible for the resource to be removed and
> + * its memory freed, leading to use-after-free errors on subsequent access.
> + *
> + * Introduce the revocable to establish weak references to such resources.
> + * It allows a resource consumer to safely attempt to access a resource
> + * that might be freed at any time by the resource provider.
> + *
> + * The implementation uses a provider/consumer model built on Sleepable
> + * RCU (SRCU) to guarantee safe memory access:
> + *
> + * - A resource provider allocates a struct revocable_provider and
> + *   initializes it with a pointer to the resource.

There is a certain amount of duplication here, stuff that might go out
of sync at some point.  I would consider pushing the bulk of the
information into the kerneldoc comments, then actually *using* those
comments in the .rst file (with kernel-doc directives) to create the
rendered version.

Thanks,

jon

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Greg Kroah-Hartman]

On Fri, Sep 12, 2025 at 04:26:56PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
> > On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
> > > On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
> > > > On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > >
> > > > Thanks for the work. Just a note, please start using b4, so above Cc
> > > > will be propagated to all patches. Folks above received only the cover
> > > > letter...
> > 
> > Thank you for bringing this to my attention.  I wasn't aware of that and
> > will ensure this is handled correctly in the future.
> > 
> > > Thanks to Krzysztof for making me aware of this. Could you please Cc
> > > my brgl@bgdev.pl address on the next iteration.
> > 
> > Sure, will do.
> > 
> > > I haven't looked into the details yet but the small size of the first
> > > patch strikes me as odd. The similar changes I did for GPIO were quite
> > > big and they were designed just for a single sub-system.
> > > 
> > > During the talk you reference, after I suggested a library like this,
> > > Greg KH can be heard saying: do this for two big subsystems so that
> > > you're sure it's a generic solution. Here you're only using it in a
> > > single driver which makes me wonder if we can actually use it to
> > > improve bigger offenders, like for example I2C, or even replace the
> > > custom, SRCU-based solution in GPIO we have now. Have you considered
> > > at least doing a PoC in a wider kernel framework?
> > 
> > Yes, I'm happy to take this on.
> > 
> > To help me get started, could you please point me to some relevant code
> > locations?  Also, could you let me know if any specific physical devices
> > will be needed for testing?
> 
> One interesting test would be to move the logic to the cdev layer. The
> use-after-free problem isn't specific to one type of character device,
> and so shouldn't require a fix in every driver instantiating a cdev
> directly (or indirectly). See [1] for a previous attempt to handle this
> at the V4L2 level and [2] for an attempt to handle it at the cdev level.
> 
> In [1], two new functions named video_device_enter() and
> video_device_exit() flag the beginning and end of protected code
> sections. The equivalent in [2] is the manual get/put of cdev->qactive,
> and if I understand things correctly, your series creates a REVOCABLE()
> macro to do the same. I'm sure we'll bikesheed about names at some
> point, but for the time being, what I'd like to see if this being done
> in fs/char_dev.c to cover all entry points from userspace at the cdev
> level.
> 
> We then have video_device_unplug() in [1], which I think is more or less
> the equivalent of revocable_provider_free(). I don't think we'll be able
> to hide this completely from drivers, at least not in all cases. We
> should however design the API to make it easy for drivers, likely with
> subsystem-specific wrappers.
> 
> What I have in mind is roughly the following:
> 
> 1. Protect all access to the cdev from userspace with enter/exit calls
>    that flag if a call is in progress. This can be done with explicit
>    function calls, or with a scope guard as in your series.
> 
> 2. At .remove() time, start by flagging that the device is being
>    removed. That has to be an explicit call from drivers I believe,
>    likely using subsystem-specific wrappers to simplify things.
> 
> 3. Once the device is marked as being removed, all enter() calls should
>    fail at the cdev level.
> 
> 4. In .remove(), proceed to perform driver-specific operations that will
>    stop the device and wake up any userspace task blocked on a syscall
>    protected by enter()/remove(). This isn't needed for
>    drivers/subsystems that don't provide any blocking API, but is
>    required otherwise.
> 
> 5. Unregister, still in .remove(), the cdev (likely through
>    subsystem-specific APIs in most cases). This should block until all
>    protected sections have exited.
> 
> 6. The cdev is now unregistered, can't be opened anymore, and any
>    new syscall on any opened file handle will return an error. The
>    driver's .remove() function can proceed to free data, there won't be
>    any UAF caused by userspace.
> 
> [1] implemented this fairly naively with flags and spinlocks. An
> RCU-based implementation is probably more efficient, even if I don't
> know how performance-sensitive all this is.
> 
> Does this align with your design, and do you think you could give a try
> at pushing revocable resource handling to the cdev level ?
> 
> On a separate note, I'm not sure "revocable" is the right name here. I
> believe a revocable resource API is needed, and well-named, for
> in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the
> userspace syscalls racing with .remove(), I don't think we're dealing
> with "revocable resources". Now, if a "revocable resources" API were to
> support the in-kernel users, and be usable as a building block to fix
> the cdev issue, I would have nothing against it, but the "revocable"
> name should be internal in that case, used in the cdev layer only, and
> not exposed to drivers (or even subsystem helpers that should wrap cdev
> functions instead).

I think the name makes sense as it matches up with how things are
working (the backend structure is "revoked"), but naming is tough :)

I have no objection moving this to the cdev api, BUT given that 'struct
cdev' is embedded everywhere, I don't think it's going to be a simple
task, but rather have to be done one-driver-at-a-time like the patch in
this series does it.

And that's fine, we have "interns" that we can set loose on this type of
code conversions, I think we just need to wrap the access to the cdev
with this api, which will take a bit of rewriting in many drivers.

Anyway, just my thought, if someone else can see how this could drop
into the core cdev code without any changes needed, that would be great,
but I don't see it at the moment.  cdev is just too "raw" for that.

thanks,

greg k-h

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 03:39:45PM +0200, Greg KH wrote:
> On Fri, Sep 12, 2025 at 04:26:56PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
> > > On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
> > > > On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
> > > > > On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > > >
> > > > > Thanks for the work. Just a note, please start using b4, so above Cc
> > > > > will be propagated to all patches. Folks above received only the cover
> > > > > letter...
> > > 
> > > Thank you for bringing this to my attention.  I wasn't aware of that and
> > > will ensure this is handled correctly in the future.
> > > 
> > > > Thanks to Krzysztof for making me aware of this. Could you please Cc
> > > > my brgl@bgdev.pl address on the next iteration.
> > > 
> > > Sure, will do.
> > > 
> > > > I haven't looked into the details yet but the small size of the first
> > > > patch strikes me as odd. The similar changes I did for GPIO were quite
> > > > big and they were designed just for a single sub-system.
> > > > 
> > > > During the talk you reference, after I suggested a library like this,
> > > > Greg KH can be heard saying: do this for two big subsystems so that
> > > > you're sure it's a generic solution. Here you're only using it in a
> > > > single driver which makes me wonder if we can actually use it to
> > > > improve bigger offenders, like for example I2C, or even replace the
> > > > custom, SRCU-based solution in GPIO we have now. Have you considered
> > > > at least doing a PoC in a wider kernel framework?
> > > 
> > > Yes, I'm happy to take this on.
> > > 
> > > To help me get started, could you please point me to some relevant code
> > > locations?  Also, could you let me know if any specific physical devices
> > > will be needed for testing?
> > 
> > One interesting test would be to move the logic to the cdev layer. The
> > use-after-free problem isn't specific to one type of character device,
> > and so shouldn't require a fix in every driver instantiating a cdev
> > directly (or indirectly). See [1] for a previous attempt to handle this
> > at the V4L2 level and [2] for an attempt to handle it at the cdev level.
> > 
> > In [1], two new functions named video_device_enter() and
> > video_device_exit() flag the beginning and end of protected code
> > sections. The equivalent in [2] is the manual get/put of cdev->qactive,
> > and if I understand things correctly, your series creates a REVOCABLE()
> > macro to do the same. I'm sure we'll bikesheed about names at some
> > point, but for the time being, what I'd like to see if this being done
> > in fs/char_dev.c to cover all entry points from userspace at the cdev
> > level.
> > 
> > We then have video_device_unplug() in [1], which I think is more or less
> > the equivalent of revocable_provider_free(). I don't think we'll be able
> > to hide this completely from drivers, at least not in all cases. We
> > should however design the API to make it easy for drivers, likely with
> > subsystem-specific wrappers.
> > 
> > What I have in mind is roughly the following:
> > 
> > 1. Protect all access to the cdev from userspace with enter/exit calls
> >    that flag if a call is in progress. This can be done with explicit
> >    function calls, or with a scope guard as in your series.
> > 
> > 2. At .remove() time, start by flagging that the device is being
> >    removed. That has to be an explicit call from drivers I believe,
> >    likely using subsystem-specific wrappers to simplify things.
> > 
> > 3. Once the device is marked as being removed, all enter() calls should
> >    fail at the cdev level.
> > 
> > 4. In .remove(), proceed to perform driver-specific operations that will
> >    stop the device and wake up any userspace task blocked on a syscall
> >    protected by enter()/remove(). This isn't needed for
> >    drivers/subsystems that don't provide any blocking API, but is
> >    required otherwise.
> > 
> > 5. Unregister, still in .remove(), the cdev (likely through
> >    subsystem-specific APIs in most cases). This should block until all
> >    protected sections have exited.
> > 
> > 6. The cdev is now unregistered, can't be opened anymore, and any
> >    new syscall on any opened file handle will return an error. The
> >    driver's .remove() function can proceed to free data, there won't be
> >    any UAF caused by userspace.
> > 
> > [1] implemented this fairly naively with flags and spinlocks. An
> > RCU-based implementation is probably more efficient, even if I don't
> > know how performance-sensitive all this is.
> > 
> > Does this align with your design, and do you think you could give a try
> > at pushing revocable resource handling to the cdev level ?
> > 
> > On a separate note, I'm not sure "revocable" is the right name here. I
> > believe a revocable resource API is needed, and well-named, for
> > in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the
> > userspace syscalls racing with .remove(), I don't think we're dealing
> > with "revocable resources". Now, if a "revocable resources" API were to
> > support the in-kernel users, and be usable as a building block to fix
> > the cdev issue, I would have nothing against it, but the "revocable"
> > name should be internal in that case, used in the cdev layer only, and
> > not exposed to drivers (or even subsystem helpers that should wrap cdev
> > functions instead).
> 
> I think the name makes sense as it matches up with how things are
> working (the backend structure is "revoked"), but naming is tough :)
> 
> I have no objection moving this to the cdev api, BUT given that 'struct
> cdev' is embedded everywhere, I don't think it's going to be a simple
> task, but rather have to be done one-driver-at-a-time like the patch in
> this series does it.

For the .remove() code paths, yes, I expect driver changes. We need
subsystem-level helpers that will make those easy and hide the
complexity. For the code paths from userspace into the drivers through
cdev file operations, there should be no driver change required.

> And that's fine, we have "interns" that we can set loose on this type of
> code conversions, I think we just need to wrap the access to the cdev
> with this api, which will take a bit of rewriting in many drivers.
> 
> Anyway, just my thought, if someone else can see how this could drop
> into the core cdev code without any changes needed, that would be great,
> but I don't see it at the moment.  cdev is just too "raw" for that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Bartosz Golaszewski]

On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> I have no objection moving this to the cdev api, BUT given that 'struct
> cdev' is embedded everywhere, I don't think it's going to be a simple
> task, but rather have to be done one-driver-at-a-time like the patch in
> this series does it.
>

I don't think cdev is the right place for this as user-space keeping a
reference to a file-descriptor whose "backend" disappeared is not the
only possible problem. We can easily create a use-case of a USB I2C
expander being used by some in-kernel consumer and then unplugged.
This has nothing to do with the character device. I believe the
sub-system level is the right place for this and every driver
subsystem would have to integrate it separately, taking its various
quirks into account.

Bartosz

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
> >
> > I have no objection moving this to the cdev api, BUT given that 'struct
> > cdev' is embedded everywhere, I don't think it's going to be a simple
> > task, but rather have to be done one-driver-at-a-time like the patch in
> > this series does it.
> 
> I don't think cdev is the right place for this as user-space keeping a
> reference to a file-descriptor whose "backend" disappeared is not the
> only possible problem. We can easily create a use-case of a USB I2C
> expander being used by some in-kernel consumer and then unplugged.
> This has nothing to do with the character device. I believe the
> sub-system level is the right place for this and every driver
> subsystem would have to integrate it separately, taking its various
> quirks into account.

That's why I mentioned in-kernel users previously. Drivers routinely
acquire resources provided by other drivers, and having a way to revoke
those is needed.

It is a different but related problem compared to userspace racing with
.remove(). Could we solve both using the same backend concepts ?
Perhaps, time will tell, and if that works nicely, great. But we still
have lots of drivers exposing character devices to userspace (usually
through a subsystem-specific API, drivers that create a cdev manually
are the minority). That problem is in my opinion more urgent than
handling the removal of in-kernel resources, because it's more common,
and is easily triggerable by userspace. The good news is that it should
also be simpler to solve, we should be able to address the enter/exit
part entirely in cdev, and limit the changes to drivers in .remove() to
the strict minimum.

What I'd like to see is if the proposed implementation of revocable
resources can be used as a building block to fix the cdev issue. If it
ca, great, let's solve it then. If it can't, that's still fine, it will
still be useful for in-kernel resources, even if we need a different
implementation for cdev.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Greg Kroah-Hartman]

On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
> > >
> > > I have no objection moving this to the cdev api, BUT given that 'struct
> > > cdev' is embedded everywhere, I don't think it's going to be a simple
> > > task, but rather have to be done one-driver-at-a-time like the patch in
> > > this series does it.
> > 
> > I don't think cdev is the right place for this as user-space keeping a
> > reference to a file-descriptor whose "backend" disappeared is not the
> > only possible problem. We can easily create a use-case of a USB I2C
> > expander being used by some in-kernel consumer and then unplugged.
> > This has nothing to do with the character device. I believe the
> > sub-system level is the right place for this and every driver
> > subsystem would have to integrate it separately, taking its various
> > quirks into account.
> 
> That's why I mentioned in-kernel users previously. Drivers routinely
> acquire resources provided by other drivers, and having a way to revoke
> those is needed.
> 
> It is a different but related problem compared to userspace racing with
> .remove(). Could we solve both using the same backend concepts ?
> Perhaps, time will tell, and if that works nicely, great. But we still
> have lots of drivers exposing character devices to userspace (usually
> through a subsystem-specific API, drivers that create a cdev manually
> are the minority). That problem is in my opinion more urgent than
> handling the removal of in-kernel resources, because it's more common,
> and is easily triggerable by userspace. The good news is that it should
> also be simpler to solve, we should be able to address the enter/exit
> part entirely in cdev, and limit the changes to drivers in .remove() to
> the strict minimum.
> 
> What I'd like to see is if the proposed implementation of revocable
> resources can be used as a building block to fix the cdev issue. If it
> ca, great, let's solve it then. If it can't, that's still fine, it will
> still be useful for in-kernel resources, even if we need a different
> implementation for cdev.

Patch 5/5 in this series does just this for a specific use of a cdev in
the driver.  Is that what you are looking for?

thanks,

greg k-h

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

(CC'ing Dan Williams)

On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
> On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
> > > On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
> > > >
> > > > I have no objection moving this to the cdev api, BUT given that 'struct
> > > > cdev' is embedded everywhere, I don't think it's going to be a simple
> > > > task, but rather have to be done one-driver-at-a-time like the patch in
> > > > this series does it.
> > > 
> > > I don't think cdev is the right place for this as user-space keeping a
> > > reference to a file-descriptor whose "backend" disappeared is not the
> > > only possible problem. We can easily create a use-case of a USB I2C
> > > expander being used by some in-kernel consumer and then unplugged.
> > > This has nothing to do with the character device. I believe the
> > > sub-system level is the right place for this and every driver
> > > subsystem would have to integrate it separately, taking its various
> > > quirks into account.
> > 
> > That's why I mentioned in-kernel users previously. Drivers routinely
> > acquire resources provided by other drivers, and having a way to revoke
> > those is needed.
> > 
> > It is a different but related problem compared to userspace racing with
> > .remove(). Could we solve both using the same backend concepts ?
> > Perhaps, time will tell, and if that works nicely, great. But we still
> > have lots of drivers exposing character devices to userspace (usually
> > through a subsystem-specific API, drivers that create a cdev manually
> > are the minority). That problem is in my opinion more urgent than
> > handling the removal of in-kernel resources, because it's more common,
> > and is easily triggerable by userspace. The good news is that it should
> > also be simpler to solve, we should be able to address the enter/exit
> > part entirely in cdev, and limit the changes to drivers in .remove() to
> > the strict minimum.
> > 
> > What I'd like to see is if the proposed implementation of revocable
> > resources can be used as a building block to fix the cdev issue. If it
> > ca, great, let's solve it then. If it can't, that's still fine, it will
> > still be useful for in-kernel resources, even if we need a different
> > implementation for cdev.
> 
> Patch 5/5 in this series does just this for a specific use of a cdev in
> the driver.  Is that what you are looking for?

Not quite, I would like to see the enter/exit (aka revocable scope if my
understanding is correct) being pushed to char_dev.c, as Dan did in [1].
I'm fine having to add an extra function call in the .remove() path of
drivers, but I'm not fine with having to mark revocable sections
manually in drivers. That part belongs to cdev.

[1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Greg Kroah-Hartman]

On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
> (CC'ing Dan Williams)
> 
> On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
> > On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
> > > > On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
> > > > >
> > > > > I have no objection moving this to the cdev api, BUT given that 'struct
> > > > > cdev' is embedded everywhere, I don't think it's going to be a simple
> > > > > task, but rather have to be done one-driver-at-a-time like the patch in
> > > > > this series does it.
> > > > 
> > > > I don't think cdev is the right place for this as user-space keeping a
> > > > reference to a file-descriptor whose "backend" disappeared is not the
> > > > only possible problem. We can easily create a use-case of a USB I2C
> > > > expander being used by some in-kernel consumer and then unplugged.
> > > > This has nothing to do with the character device. I believe the
> > > > sub-system level is the right place for this and every driver
> > > > subsystem would have to integrate it separately, taking its various
> > > > quirks into account.
> > > 
> > > That's why I mentioned in-kernel users previously. Drivers routinely
> > > acquire resources provided by other drivers, and having a way to revoke
> > > those is needed.
> > > 
> > > It is a different but related problem compared to userspace racing with
> > > .remove(). Could we solve both using the same backend concepts ?
> > > Perhaps, time will tell, and if that works nicely, great. But we still
> > > have lots of drivers exposing character devices to userspace (usually
> > > through a subsystem-specific API, drivers that create a cdev manually
> > > are the minority). That problem is in my opinion more urgent than
> > > handling the removal of in-kernel resources, because it's more common,
> > > and is easily triggerable by userspace. The good news is that it should
> > > also be simpler to solve, we should be able to address the enter/exit
> > > part entirely in cdev, and limit the changes to drivers in .remove() to
> > > the strict minimum.
> > > 
> > > What I'd like to see is if the proposed implementation of revocable
> > > resources can be used as a building block to fix the cdev issue. If it
> > > ca, great, let's solve it then. If it can't, that's still fine, it will
> > > still be useful for in-kernel resources, even if we need a different
> > > implementation for cdev.
> > 
> > Patch 5/5 in this series does just this for a specific use of a cdev in
> > the driver.  Is that what you are looking for?
> 
> Not quite, I would like to see the enter/exit (aka revocable scope if my
> understanding is correct) being pushed to char_dev.c, as Dan did in [1].
> I'm fine having to add an extra function call in the .remove() path of
> drivers, but I'm not fine with having to mark revocable sections
> manually in drivers. That part belongs to cdev.
> 
> [1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com

Dan's proposal here is a good start, but the "sleep in cdev_del() until
the device drains all existing opens" is going to not really work well
for what we want.

So sure, make a new cdev api to use this, that's fine, then we will have
what, 5 different ways to use a cdev?  :)

Seriously, that would be good, then we can work to convert things over,
but I think overall it will look much the same as what patch 5/5 does
here.  But details matter, I don't really known for sure...

Either way, I think this patch series stands on its own, it doesn't
require cdev to implement it, drivers can use it to wrap a cdev if they
want to.  We have other structures that want to do this type of thing
today as is proof with the rust implementation for the devm api.

thanks,

greg k-h

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Bartosz Golaszewski]

On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Dan's proposal here is a good start, but the "sleep in cdev_del() until
> the device drains all existing opens" is going to not really work well
> for what we want.
>
> So sure, make a new cdev api to use this, that's fine, then we will have
> what, 5 different ways to use a cdev?  :)
>
> Seriously, that would be good, then we can work to convert things over,
> but I think overall it will look much the same as what patch 5/5 does
> here.  But details matter, I don't really known for sure...
>
> Either way, I think this patch series stands on its own, it doesn't
> require cdev to implement it, drivers can use it to wrap a cdev if they
> want to.  We have other structures that want to do this type of thing
> today as is proof with the rust implementation for the devm api.
>

Yeah, I'm not against this going upstream. If more development is
needed for this to be usable in other parts of the kernel, that can be
done gradually. Literally no subsystem ever was perfect on day 1.

Tzung-Bi: I'm not sure if you did submit anything but I'd love to see
this discussed during Linux Plumbers in Tokyo, it's the perfect fit
for the kernel summit.

Bartosz

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 04:40:27PM +0200, Greg KH wrote:
> On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
> > (CC'ing Dan Williams)
> > 
> > On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
> > > On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
> > > > > On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
> > > > > >
> > > > > > I have no objection moving this to the cdev api, BUT given that 'struct
> > > > > > cdev' is embedded everywhere, I don't think it's going to be a simple
> > > > > > task, but rather have to be done one-driver-at-a-time like the patch in
> > > > > > this series does it.
> > > > > 
> > > > > I don't think cdev is the right place for this as user-space keeping a
> > > > > reference to a file-descriptor whose "backend" disappeared is not the
> > > > > only possible problem. We can easily create a use-case of a USB I2C
> > > > > expander being used by some in-kernel consumer and then unplugged.
> > > > > This has nothing to do with the character device. I believe the
> > > > > sub-system level is the right place for this and every driver
> > > > > subsystem would have to integrate it separately, taking its various
> > > > > quirks into account.
> > > > 
> > > > That's why I mentioned in-kernel users previously. Drivers routinely
> > > > acquire resources provided by other drivers, and having a way to revoke
> > > > those is needed.
> > > > 
> > > > It is a different but related problem compared to userspace racing with
> > > > .remove(). Could we solve both using the same backend concepts ?
> > > > Perhaps, time will tell, and if that works nicely, great. But we still
> > > > have lots of drivers exposing character devices to userspace (usually
> > > > through a subsystem-specific API, drivers that create a cdev manually
> > > > are the minority). That problem is in my opinion more urgent than
> > > > handling the removal of in-kernel resources, because it's more common,
> > > > and is easily triggerable by userspace. The good news is that it should
> > > > also be simpler to solve, we should be able to address the enter/exit
> > > > part entirely in cdev, and limit the changes to drivers in .remove() to
> > > > the strict minimum.
> > > > 
> > > > What I'd like to see is if the proposed implementation of revocable
> > > > resources can be used as a building block to fix the cdev issue. If it
> > > > ca, great, let's solve it then. If it can't, that's still fine, it will
> > > > still be useful for in-kernel resources, even if we need a different
> > > > implementation for cdev.
> > > 
> > > Patch 5/5 in this series does just this for a specific use of a cdev in
> > > the driver.  Is that what you are looking for?
> > 
> > Not quite, I would like to see the enter/exit (aka revocable scope if my
> > understanding is correct) being pushed to char_dev.c, as Dan did in [1].
> > I'm fine having to add an extra function call in the .remove() path of
> > drivers, but I'm not fine with having to mark revocable sections
> > manually in drivers. That part belongs to cdev.
> > 
> > [1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com
> 
> Dan's proposal here is a good start, but the "sleep in cdev_del() until
> the device drains all existing opens" is going to not really work well
> for what we want.

I think you missed the fact that the code doesn't wait for all open file
handles to be closed. It waits for all in-progress syscalls to return
from the driver. That's a big difference.

> So sure, make a new cdev api to use this, that's fine, then we will have
> what, 5 different ways to use a cdev?  :)
> 
> Seriously, that would be good, then we can work to convert things over,
> but I think overall it will look much the same as what patch 5/5 does
> here.  But details matter, I don't really known for sure...

What I don't want to see is drivers using this new API directly to
protect the cdev race. That would be a big step in the wrong direction.
Patch 5/5 needs to move driver code to the cdev level. That shouldn't be
difficult, so I really not see why it can't be done in v4 to see how it
will look like.

> Either way, I think this patch series stands on its own, it doesn't
> require cdev to implement it, drivers can use it to wrap a cdev if they
> want to.

No, drivers should *not* do that manually. That's a recipe for disaster
that we would regret later.

> We have other structures that want to do this type of thing
> today as is proof with the rust implementation for the devm api.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Dan's proposal here is a good start, but the "sleep in cdev_del() until
> > the device drains all existing opens" is going to not really work well
> > for what we want.
> >
> > So sure, make a new cdev api to use this, that's fine, then we will have
> > what, 5 different ways to use a cdev?  :)
> >
> > Seriously, that would be good, then we can work to convert things over,
> > but I think overall it will look much the same as what patch 5/5 does
> > here.  But details matter, I don't really known for sure...
> >
> > Either way, I think this patch series stands on its own, it doesn't
> > require cdev to implement it, drivers can use it to wrap a cdev if they
> > want to.  We have other structures that want to do this type of thing
> > today as is proof with the rust implementation for the devm api.
> 
> Yeah, I'm not against this going upstream. If more development is
> needed for this to be usable in other parts of the kernel, that can be
> done gradually. Literally no subsystem ever was perfect on day 1.

To be clear, I'm not against the API being merged for the use cases that
would benefit from it, but I don't want to see drivers using it to
protect from the cdev/unregistration race.

> Tzung-Bi: I'm not sure if you did submit anything but I'd love to see
> this discussed during Linux Plumbers in Tokyo, it's the perfect fit
> for the kernel summit.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Danilo Krummrich]

On 9/12/25 4:54 PM, Laurent Pinchart wrote:
> On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
>> On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> Either way, I think this patch series stands on its own, it doesn't
>>> require cdev to implement it, drivers can use it to wrap a cdev if they
>>> want to.  We have other structures that want to do this type of thing
>>> today as is proof with the rust implementation for the devm api.
>>
>> Yeah, I'm not against this going upstream. If more development is
>> needed for this to be usable in other parts of the kernel, that can be
>> done gradually. Literally no subsystem ever was perfect on day 1.
> 
> To be clear, I'm not against the API being merged for the use cases that
> would benefit from it, but I don't want to see drivers using it to
> protect from the cdev/unregistration race.

I mean, revocable is really a synchronization primitive in the end that
"revokes" access to some resource in a race free way.

So, technically, it probably belongs into lib/.

I think the reason it ended up in drivers/base/ is that one common use case is
to revoke a device resource from a driver when the device is unbound from this
driver; or in other words devres is an obvious user.

So, I think that any other API (cdev, devres, etc.) should  be built on top of it.

This is also what we do in Rust, Revocable is just a common synchronization
primitive and the (only) user it has is currently Devres building on top of it.

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Tzung-Bi Shih]

On Fri, Sep 12, 2025 at 05:54:16PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > Dan's proposal here is a good start, but the "sleep in cdev_del() until
> > > the device drains all existing opens" is going to not really work well
> > > for what we want.
> > >
> > > So sure, make a new cdev api to use this, that's fine, then we will have
> > > what, 5 different ways to use a cdev?  :)
> > >
> > > Seriously, that would be good, then we can work to convert things over,
> > > but I think overall it will look much the same as what patch 5/5 does
> > > here.  But details matter, I don't really known for sure...
> > >
> > > Either way, I think this patch series stands on its own, it doesn't
> > > require cdev to implement it, drivers can use it to wrap a cdev if they
> > > want to.  We have other structures that want to do this type of thing
> > > today as is proof with the rust implementation for the devm api.
> > 
> > Yeah, I'm not against this going upstream. If more development is
> > needed for this to be usable in other parts of the kernel, that can be
> > done gradually. Literally no subsystem ever was perfect on day 1.
> 
> To be clear, I'm not against the API being merged for the use cases that
> would benefit from it, but I don't want to see drivers using it to
> protect from the cdev/unregistration race.

Based on the discussion thread, my main takeaways are:

- Current `revocable` is considered a low level API.  We shouldn't (and
  likely can't) stop drivers, like the one in patch 5/5 in the series,
  from using it directly to fix UAFs.

- Subsystems (like cdev) should build on this API to provide an easier
  interface for their drivers to manage revocable resources.

I'll create a PoC based on this.

> > Tzung-Bi: I'm not sure if you did submit anything but I'd love to see
> > this discussed during Linux Plumbers in Tokyo, it's the perfect fit
> > for the kernel summit.

Yes, and I just realized that in addition to the website submission, a
separate email is also required (or at least encouraged).  I've just sent
that email and am hoping it's not too late.

Re: [PATCH v3 1/5] revocable: Revocable resource management

[Tzung-Bi Shih]

On Fri, Sep 12, 2025 at 07:27:26AM -0600, Jonathan Corbet wrote:
> There is a certain amount of duplication here, stuff that might go out
> of sync at some point.  I would consider pushing the bulk of the
> information into the kerneldoc comments, then actually *using* those
> comments in the .rst file (with kernel-doc directives) to create the
> rendered version.

Ack, will fix it in the next version.

Re: [PATCH v3 1/5] revocable: Revocable resource management

[Tzung-Bi Shih]

On Fri, Sep 12, 2025 at 11:05:20AM +0200, Danilo Krummrich wrote:
> On Fri Sep 12, 2025 at 10:17 AM CEST, Tzung-Bi Shih wrote:
> > +/**
> > + * struct revocable_provider - A handle for resource provider.
> > + * @srcu: The SRCU to protect the resource.
> > + * @res:  The pointer of resource.  It can point to anything.
> > + * @kref: The refcount for this handle.
> > + */
> > +struct revocable_provider {
> > +	struct srcu_struct srcu;
> > +	void __rcu *res;
> > +	struct kref kref;
> > +};
> 
> I think a revocable provider should provide an optional revoke() callback where
> users of the revocable provider can release the revoked resource.
>
> But this can also be done as a follow-up.

Understood.  Since this effectively delegates the memory of `res` to the
struct revocable provider, I propose we name the callback .release().

> > +/**
> > + * struct revocable - A handle for resource consumer.
> > + * @rp: The pointer of resource provider.
> > + * @idx: The index for the RCU critical section.
> > + */
> > +struct revocable {
> > +	struct revocable_provider *rp;
> > +	int idx;
> > +};
> 
> I think I asked about this in the previous version (but I don't remember if
> there was an answer):

Yes, in v1 https://lore.kernel.org/chrome-platform/aJ7HUJ0boqYndbtD@google.com/.

> In Rust we get away with a single Revocable<T> structure, but we're using RCU
> instead of SRCU. It seems to me that the split between struct revocable and
> struct revocable_provider is only for the SRCU index? Or is there any other
> reason?

Yes, for the SRCU index.

> > +/**
> > + * revocable_provider_free() - Free struct revocable_provider.
> > + * @rp: The pointer of resource provider.
> > + *
> > + * This sets the resource `(struct revocable_provider *)->res` to NULL to
> > + * indicate the resource has gone.
> > + *
> > + * This drops the refcount to the resource provider.  If it is the final
> > + * reference, revocable_provider_release() will be called to free the struct.
> > + */
> > +void revocable_provider_free(struct revocable_provider *rp)
> > +{
> > +	rcu_assign_pointer(rp->res, NULL);
> > +	synchronize_srcu(&rp->srcu);
> > +	kref_put(&rp->kref, revocable_provider_release);
> > +}
> > +EXPORT_SYMBOL_GPL(revocable_provider_free);
> 
> I think naming this "free" is a bit misleading, since what it basically does is
> 
>   (1) Revoke access to the resource.
> 
>   (2) Drop a reference count of struct revocable_provider.
> 
> In a typical application there may still be struct revocable instances that have
> a reference to the provider, so we can't claim that it's freed here.
> 
> So, given that, I'd rather call this revocable_provider_revoke().

Ack, will fix it in the next version.

> > +static void devm_revocable_provider_free(void *data)
> > +{
> > +	struct revocable_provider *rp = data;
> > +
> > +	revocable_provider_free(rp);
> > +}
> 
> Same here, I'd call this devm_revocable_provider_revoke().

Ack, will fix it in the next version.

> > +DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
> > +
> > +#define _REVOCABLE(_rev, _label, _res)						\
> > +	for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev;	\
> > +	     (_res = revocable_try_access(_rev)) || true; ({ goto _label; }))	\
> > +		if (0) {							\
> > +_label:										\
> > +			break;							\
> > +		} else
> > +
> > +#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
> 
> This is basically the same as Revocable::try_access_with() [1] in Rust, i.e.
> try to access and run a closure.
> 
> Admittedly, REVOCABLE_TRY_ACCESS_WITH() is pretty verbose and I also do not have
> a great idea to shorten it.
> 
> Maybe you have a good idea, otherwise I'm also fine with the current name.
> 
> Otherwise, maybe it's worth to link to the Rust Revocable API for reference?

No, I don't have a good idea either.  Will use REVOCABLE_TRY_ACCESS_WITH()
to align with Rust Revocable API in the next version.

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Sat, Sep 13, 2025 at 11:55:45PM +0800, Tzung-Bi Shih wrote:
> On Fri, Sep 12, 2025 at 05:54:16PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
> > > On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
> > > >
> > > > Dan's proposal here is a good start, but the "sleep in cdev_del() until
> > > > the device drains all existing opens" is going to not really work well
> > > > for what we want.
> > > >
> > > > So sure, make a new cdev api to use this, that's fine, then we will have
> > > > what, 5 different ways to use a cdev?  :)
> > > >
> > > > Seriously, that would be good, then we can work to convert things over,
> > > > but I think overall it will look much the same as what patch 5/5 does
> > > > here.  But details matter, I don't really known for sure...
> > > >
> > > > Either way, I think this patch series stands on its own, it doesn't
> > > > require cdev to implement it, drivers can use it to wrap a cdev if they
> > > > want to.  We have other structures that want to do this type of thing
> > > > today as is proof with the rust implementation for the devm api.
> > > 
> > > Yeah, I'm not against this going upstream. If more development is
> > > needed for this to be usable in other parts of the kernel, that can be
> > > done gradually. Literally no subsystem ever was perfect on day 1.
> > 
> > To be clear, I'm not against the API being merged for the use cases that
> > would benefit from it, but I don't want to see drivers using it to
> > protect from the cdev/unregistration race.
> 
> Based on the discussion thread, my main takeaways are:
> 
> - Current `revocable` is considered a low level API.  We shouldn't (and
>   likely can't) stop drivers, like the one in patch 5/5 in the series,
>   from using it directly to fix UAFs.

Why shouldn't we ? We have enough precedents where driver authors rushed
to adopt brand new APIs without understand the implications.
devm_kzalloc() is a prime example of a small new API that very quickly
got misused everywhere. If we had taken the time to clearly explain when
it should be used and when it should *not* be used, we wouldn't be
plagued by as many device removal race conditions today. Let's not
repeat the same mistake, I'd like this new API to make things better,
not worse.

> - Subsystems (like cdev) should build on this API to provide an easier
>   interface for their drivers to manage revocable resources.
> 
> I'll create a PoC based on this.

I'm looking forward to that. Please let me know if there's anything you
would like to discuss. I didn't dive deep in technical details in this
thread, and I don't expect anyone to guess what I have in mind if I
failed to express it :-) I'm very confident the cdev race condition can
be fixed in a neat way, so let's do that.

> > > Tzung-Bi: I'm not sure if you did submit anything but I'd love to see
> > > this discussed during Linux Plumbers in Tokyo, it's the perfect fit
> > > for the kernel summit.
> 
> Yes, and I just realized that in addition to the website submission, a
> separate email is also required (or at least encouraged).  I've just sent
> that email and am hoping it's not too late.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Laurent Pinchart]

On Fri, Sep 12, 2025 at 06:22:48PM +0200, Danilo Krummrich wrote:
> On 9/12/25 4:54 PM, Laurent Pinchart wrote:
> > On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
> >> On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
> >>> Either way, I think this patch series stands on its own, it doesn't
> >>> require cdev to implement it, drivers can use it to wrap a cdev if they
> >>> want to.  We have other structures that want to do this type of thing
> >>> today as is proof with the rust implementation for the devm api.
> >>
> >> Yeah, I'm not against this going upstream. If more development is
> >> needed for this to be usable in other parts of the kernel, that can be
> >> done gradually. Literally no subsystem ever was perfect on day 1.
> > 
> > To be clear, I'm not against the API being merged for the use cases that
> > would benefit from it, but I don't want to see drivers using it to
> > protect from the cdev/unregistration race.
> 
> I mean, revocable is really a synchronization primitive in the end that
> "revokes" access to some resource in a race free way.
> 
> So, technically, it probably belongs into lib/.
> 
> I think the reason it ended up in drivers/base/ is that one common use case is
> to revoke a device resource from a driver when the device is unbound from this
> driver; or in other words devres is an obvious user.
> 
> So, I think that any other API (cdev, devres, etc.) should  be built on top of it.

No issue with that. I'm sure there are people who have better knowledge
than me when it comes to implementing the low-level primitive in the
most efficient way. What I have lots of experience with is the impact of
API design on drivers, and the API misuse (including through cargo-cult
programming) this can generate. Let's design the API towards drivers
correctly.

> This is also what we do in Rust, Revocable is just a common synchronization
> primitive and the (only) user it has is currently Devres building on top of it.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/5] revocable: Revocable resource management

[Tzung-Bi Shih]

On Fri, Sep 12, 2025 at 08:17:13AM +0000, Tzung-Bi Shih wrote:
> +void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu)
> +{
> +	struct revocable_provider *rp = rev->rp;
> +
> +	rev->idx = srcu_read_lock(&rp->srcu);
> +	return rcu_dereference(rp->res);

Got a warning from lock debugging.  This should be srcu_dereference().  Will
fix it in the next version.

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Jason Gunthorpe]

On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
> Not quite, I would like to see the enter/exit (aka revocable scope if my
> understanding is correct) being pushed to char_dev.c, as Dan did in [1].
> I'm fine having to add an extra function call in the .remove() path of
> drivers, but I'm not fine with having to mark revocable sections
> manually in drivers. That part belongs to cdev.

+1 I've open coded something here many times.

The implementation challenge is performance..

The big ask would be to make file_operations replaceable without
races. I can't see a way to do that at the struct file level without
slowing down everything.

Dan's idea to provide a wrapper file_operations that then effectively
calls another set of file_operations under a synchornization is more
reasonable, but the performance cost is now a percpu ref and another
indirect function call for every file operation. It also relies on the
inode which some cdev users end up replacing.

Open coding the lock in the subsystems avoids the indirect function
call, flexible inode, and subsystems can choose their locking
preference (rwsem, srcu, percpu).

As was said later in this thread, it would be a real shame to see
people implement revocable in drivers instead of rely on subsystems to
have sane unregistration semantics where the subsystem guarentees that
no driver callbacks are running after unregister. You never need
driver revocable in a world like that.

Jason

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Danilo Krummrich]

On Mon Sep 22, 2025 at 5:10 PM CEST, Jason Gunthorpe wrote:
> As was said later in this thread, it would be a real shame to see
> people implement revocable in drivers instead of rely on subsystems to
> have sane unregistration semantics where the subsystem guarentees that
> no driver callbacks are running after unregister. You never need
> driver revocable in a world like that.

I fully agree with that, in C there is indeed no value of a revocable type when
subsystems can guarantee "sane unregistration semantics".

I say "in C" because in C there is no way to get a proof by the compiler that
we're in a scope (e.g. through the subsystem guarentee) where the device is
guaranteed to be bound (which we can in Rust).

So, effectively, we're not getting any value out of the revocable in C in such a
case: In the best case, we're just bypassing the revocable by accessing the
pointer unchecked (regardless whether that's valid or not); in the worst case
we're introducing a useless SRCU read side critical section.

(In Rust the compiler will stop us from accessing the pointer unchecked if we're
not in a scope where unchecked access is valid.)

So, I think in C revocable should be restricted to use-cases where scopes are
unbound by design. DRM device callbacks are an example for that and it's the
reason why things like drm_dev_{enter,exit}() and drm_dev_unplug() exist. In the
end, those are exactly the same as revocable implemented in a slightly different
way.

- Danilo

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Jason Gunthorpe]

On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
> I fully agree with that, in C there is indeed no value of a revocable type when
> subsystems can guarantee "sane unregistration semantics".

Indeed, I agree with your message. If I look at the ec_cros presented
here, there is no reason for any revocable. In fact it already seems
like an abuse of the idea.

The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev"
that is alreay properly lifetime controlled - the platform is already
removed during the remove of the cros_ec.c.

So, there is no need for a revocable that spans two drivers like this!

The bug is that cros_ec_chardev.c doesn't implement itself correctly
and doesn't have a well designed remove() for something that creates a
char dev. This issue should be entirely handled within
cros_ec_chardev.c and not leak out to other files.

1) Using a miscdevice means loosing out on any refcount managed
memory. When using a file you need some per-device memory that lives
for as long as all files are open. So don't use miscdev, the better
pattern is:

struct chardev_data {
	struct device dev;
	struct cdev cdev;

Now you get to have a struct device linked refcount and a free
function. The file can savely hold onto a chardev_data for its
lifetime.

2) Add locking so the file can exist after the driver is removed:

struct chardev_data {
[..]
	struct rw_semaphore sem;
	struct cros_ec_dev *ec_dev;
};

Refcount the chardev_data::dev in the file operations open/release,
refer to it via the private_data.

3) At the start of every fop take the sem and check the ec_dev:

ACQUIRE(rwsem_read, ecdev_sem)(&data->sem);
if (ACQUIRE_ERR(ecdev_sem) || !data->ec_dev)
   return -ENODEV;

4) After unregistering the cdev, but before destroying the device take
the write side of the rwsem and NULL ec_dev.

5) Purge all the devm usage from cros_ec_chardev, the only allocation
is refcounted instead.

Simple. No need for any synchronize_srcu() for such a simple
non-performance oriented driver.

Yes, this can be made better, there is a bit too much boilerplate, but
revocable is not the way for cros_ec.

Jason

Re: [PATCH v3 1/5] revocable: Revocable resource management

[Simona Vetter]

On Fri, Sep 12, 2025 at 08:17:13AM +0000, Tzung-Bi Shih wrote:
> Some resources can be removed asynchronously, for example, resources
> provided by a hot-pluggable device like USB.  When holding a reference
> to such a resource, it's possible for the resource to be removed and
> its memory freed, leading to use-after-free errors on subsequent access.
> 
> Introduce the revocable to establish weak references to such resources.
> It allows a resource consumer to safely attempt to access a resource
> that might be freed at any time by the resource provider.
> 
> The implementation uses a provider/consumer model built on Sleepable
> RCU (SRCU) to guarantee safe memory access:
> 
>  - A resource provider allocates a struct revocable_provider and
>    initializes it with a pointer to the resource.
> 
>  - A resource consumer that wants to access the resource allocates a
>    struct revocable which holds a reference to the provider.
> 
>  - To access the resource, the consumer uses revocable_try_access().
>    This function enters an SRCU read-side critical section and returns
>    the pointer to the resource.  If the provider has already freed the
>    resource, it returns NULL.  After use, the consumer calls
>    revocable_release() to exit the SRCU critical section.  The
>    REVOCABLE() is a convenient helper for doing that.
> 
>  - When the provider needs to remove the resource, it calls
>    revocable_provider_free().  This function sets the internal resource
>    pointer to NULL and then calls synchronize_srcu() to wait for all
>    current readers to finish before the resource can be completely torn
>    down.
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Want. Want. Want.

Acked-by: Simona Vetter <simona.vetter@ffwll.ch>

SRCU isn't the greatest choice in theory, which is why Rust uses plain
RCU. But with C's real-world track record at getting error paths right,
it's imo the pragmatic choice since it allows you to cheat a bit and cut
some corners. Rust is flat out just massively better at this, despite
linux/cleanup.h and all the other improvements we've landed over the
years.

Since DRM uses the same concept in drm_dev_enter() and drm_dev_exit() it
would be really neat to have a patch that switches these over internally
to use revocable resources. That way drivers could use REVOCEABLE() and we
could slowly convert away from that DRM-ism.

Cheers, Sima

> ---
> v3:
> - No changes.
> 
> v2: https://lore.kernel.org/chrome-platform/20250820081645.847919-2-tzungbi@kernel.org
> - Rename "ref_proxy" -> "revocable".
> - Add introduction in kernel-doc format in revocable.c.
> - Add MAINTAINERS entry.
> - Add copyright.
> - Move from lib/ to drivers/base/.
> - EXPORT_SYMBOL() -> EXPORT_SYMBOL_GPL().
> - Add Documentation/.
> - Rename _get() -> try_access(); _put() -> release().
> - Fix a sparse warning by removing the redundant __rcu annotations.
> - Fix a sparse warning by adding __acquires() and __releases() annotations.
> 
> v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-2-tzungbi@kernel.org
> 
>  .../driver-api/driver-model/index.rst         |   1 +
>  .../driver-api/driver-model/revocable.rst     | 151 ++++++++++++
>  MAINTAINERS                                   |   7 +
>  drivers/base/Makefile                         |   2 +-
>  drivers/base/revocable.c                      | 229 ++++++++++++++++++
>  include/linux/revocable.h                     |  37 +++
>  6 files changed, 426 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/driver-api/driver-model/revocable.rst
>  create mode 100644 drivers/base/revocable.c
>  create mode 100644 include/linux/revocable.h
> 
> diff --git a/Documentation/driver-api/driver-model/index.rst b/Documentation/driver-api/driver-model/index.rst
> index 4831bdd92e5c..8e1ee21185df 100644
> --- a/Documentation/driver-api/driver-model/index.rst
> +++ b/Documentation/driver-api/driver-model/index.rst
> @@ -14,6 +14,7 @@ Driver Model
>     overview
>     platform
>     porting
> +   revocable
>  
>  .. only::  subproject and html
>  
> diff --git a/Documentation/driver-api/driver-model/revocable.rst b/Documentation/driver-api/driver-model/revocable.rst
> new file mode 100644
> index 000000000000..b9e2968ba9c1
> --- /dev/null
> +++ b/Documentation/driver-api/driver-model/revocable.rst
> @@ -0,0 +1,151 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==============================
> +Revocable Resource Management
> +==============================
> +
> +Overview
> +========
> +
> +In a system with hot-pluggable devices, such as USB, resources provided by
> +these devices can be removed asynchronously.  If a consumer holds a reference
> +to such a resource, the resource might be deallocated while the reference is
> +still held, leading to use-after-free errors upon subsequent access.
> +
> +The "revocable" mechanism addresses this by establishing a weak reference to a
> +resource that might be freed at any time.  It allows a resource consumer to
> +safely attempt to access the resource, guaranteeing that the access is valid
> +for the duration of its use, or it fails safely if the resource has already
> +been revoked.
> +
> +The implementation is based on a provider/consumer model that uses Sleepable
> +RCU (SRCU) to ensure safe memory access without traditional locking.
> +
> +How It Works
> +============
> +
> +1.  **Provider**: A resource provider, such as a driver for a hot-pluggable
> +    device, allocates a ``struct revocable_provider``.  This structure is
> +    initialized with a pointer to the actual resource it manages.
> +
> +2.  **Consumer**: A consumer that needs to access the resource is given a
> +    ``struct revocable``, which acts as a handle containing a reference to
> +    the provider.
> +
> +3.  **Accessing the Resource**: To access the resource, the consumer uses
> +    ``revocable_try_access()``.  This function enters an SRCU read-side
> +    critical section and returns a pointer to the resource.  If the provider
> +    has already revoked the resource, this function returns ``NULL``.  The
> +    consumer must check for this ``NULL`` return.
> +
> +4.  **Releasing the Resource**: After the consumer has finished using the
> +    resource, it must call ``revocable_release()`` to exit the SRCU critical
> +    section.  This signals that the consumer no longer requires access.  The
> +    ``REVOCABLE()`` macro is provided as a convenient and safe way to manage
> +    the access-release cycle.
> +
> +5.  **Revoking the Resource**: When the provider needs to remove the resource
> +    (e.g., the device is unplugged), it calls ``revocable_provider_free()``.
> +    This function first sets the internal resource pointer to ``NULL``,
> +    preventing any new consumers from accessing it.  It then calls
> +    ``synchronize_srcu()``, which waits for all existing consumers currently
> +    in the SRCU critical section to finish their work.  Once all consumers
> +    have released their access, the resource can be safely deallocated.
> +
> +Revocable vs. Device-Managed (devm) Resources
> +=============================================
> +
> +It's important to understand the distinction between a standard
> +device-managed (devm) resource and a resource managed by a
> +``revocable_provider``.
> +
> +The key difference is their lifetime:
> +
> +*   A **devm resource** is tied to the lifetime of the device.  It is
> +    automatically freed when the device is unbound.
> +*   A **revocable_provider** persists as long as there are active references
> +    to it from ``revocable`` consumer handles.
> +
> +This means that a ``revocable_provider`` can outlive the device that created
> +it.  This is a deliberate design feature, allowing consumers to hold a
> +reference to a resource even after the underlying device has been removed,
> +without causing a fault.  When the consumer attempts to access the resource,
> +it will simply be informed that the resource is no longer available.
> +
> +API and Usage
> +=============
> +
> +For Resource Providers
> +----------------------
> +
> +``struct revocable_provider *revocable_provider_alloc(void *res);``
> +    Allocates a provider handle for the given resource ``res``.  It returns a
> +    pointer to the ``revocable_provider`` on success, or ``NULL`` on failure.
> +
> +``struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, void *res);``
> +    A device-managed version of ``revocable_provider_alloc``.  It is
> +    convenient to allocate providers via this function if the ``res`` is also
> +    tied to the lifetime of the ``dev``.  ``revocable_provider_free`` will be
> +    called automatically when the device is unbound.
> +
> +``void revocable_provider_free(struct revocable_provider *rp);``
> +    Revokes the resource.  This function marks the resource as unavailable and
> +    waits for all current consumers to finish before the underlying memory
> +    can be freed.
> +
> +For Resource Consumers
> +----------------------
> +
> +``struct revocable *revocable_alloc(struct revocable_provider *rp);``
> +    Allocates a consumer handle for a given provider ``rp``.
> +
> +``void revocable_free(struct revocable *rev);``
> +    Frees a consumer handle.
> +
> +``void *revocable_try_access(struct revocable *rev);``
> +    Attempts to gain access to the resource.  Returns a pointer to the
> +    resource on success or ``NULL`` if it has been revoked.
> +
> +``void revocable_release(struct revocable *rev);``
> +    Releases access to the resource, exiting the SRCU critical section.
> +
> +The ``REVOCABLE()`` Macro
> +=========================
> +
> +The ``REVOCABLE()`` macro simplifies the access-release cycle for consumers,
> +ensuring that ``revocable_release()`` is always called, even in the case of
> +an early exit.
> +
> +``REVOCABLE(rev, res)``
> +    *   ``rev``: The consumer's ``struct revocable *`` handle.
> +    *   ``res``: A pointer variable that will be assigned the resource.
> +
> +The macro creates a ``for`` loop that executes exactly once.  Inside the loop,
> +``res`` is populated with the result of ``revocable_try_access()``.  The
> +consumer code **must** check if ``res`` is ``NULL`` before using it.  The
> +``revocable_release()`` function is automatically called when the scope of
> +the loop is exited.
> +
> +Example Usage
> +-------------
> +
> +.. code-block:: c
> +
> +    void consumer_use_resource(struct revocable *rev)
> +    {
> +        struct foo_resource *res;
> +
> +        REVOCABLE(rev, res) {
> +            // Always check if the resource is valid.
> +            if (!res) {
> +                pr_warn("Resource is not available\n");
> +                return;
> +            }
> +
> +            // At this point, 'res' is guaranteed to be valid until
> +            // this block exits.
> +            do_something_with(res);
> +        }
> +
> +        // revocable_release() is automatically called here.
> +    }
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fa7f80bd7b2f..5d11aeeb546e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21877,6 +21877,13 @@ F:	include/uapi/linux/rseq.h
>  F:	kernel/rseq.c
>  F:	tools/testing/selftests/rseq/
>  
> +REVOCABLE RESOURCE MANAGEMENT
> +M:	Tzung-Bi Shih <tzungbi@kernel.org>
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	drivers/base/revocable.c
> +F:	include/linux/revocable.h
> +
>  RFKILL
>  M:	Johannes Berg <johannes@sipsolutions.net>
>  L:	linux-wireless@vger.kernel.org
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 8074a10183dc..bdf854694e39 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -6,7 +6,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
>  			   attribute_container.o transport_class.o \
>  			   topology.o container.o property.o cacheinfo.o \
> -			   swnode.o faux.o
> +			   swnode.o faux.o revocable.o
>  obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o
>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-y			+= power/
> diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
> new file mode 100644
> index 000000000000..80a48896b241
> --- /dev/null
> +++ b/drivers/base/revocable.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 Google LLC
> + *
> + * Revocable resource management
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kref.h>
> +#include <linux/revocable.h>
> +#include <linux/slab.h>
> +#include <linux/srcu.h>
> +
> +/**
> + * DOC: Overview
> + *
> + * Some resources can be removed asynchronously, for example, resources
> + * provided by a hot-pluggable device like USB.  When holding a reference
> + * to such a resource, it's possible for the resource to be removed and
> + * its memory freed, leading to use-after-free errors on subsequent access.
> + *
> + * Introduce the revocable to establish weak references to such resources.
> + * It allows a resource consumer to safely attempt to access a resource
> + * that might be freed at any time by the resource provider.
> + *
> + * The implementation uses a provider/consumer model built on Sleepable
> + * RCU (SRCU) to guarantee safe memory access:
> + *
> + * - A resource provider allocates a struct revocable_provider and
> + *   initializes it with a pointer to the resource.
> + *
> + * - A resource consumer that wants to access the resource allocates a
> + *   struct revocable which holds a reference to the provider.
> + *
> + * - To access the resource, the consumer uses revocable_try_access().
> + *   This function enters an SRCU read-side critical section and returns
> + *   the pointer to the resource.  If the provider has already freed the
> + *   resource, it returns NULL.  After use, the consumer calls
> + *   revocable_release() to exit the SRCU critical section.  The
> + *   REVOCABLE() is a convenient helper for doing that.
> + *
> + * - When the provider needs to remove the resource, it calls
> + *   revocable_provider_free().  This function sets the internal resource
> + *   pointer to NULL and then calls synchronize_srcu() to wait for all
> + *   current readers to finish before the resource can be completely torn
> + *   down.
> + */
> +
> +/**
> + * struct revocable_provider - A handle for resource provider.
> + * @srcu: The SRCU to protect the resource.
> + * @res:  The pointer of resource.  It can point to anything.
> + * @kref: The refcount for this handle.
> + */
> +struct revocable_provider {
> +	struct srcu_struct srcu;
> +	void __rcu *res;
> +	struct kref kref;
> +};
> +
> +/**
> + * struct revocable - A handle for resource consumer.
> + * @rp: The pointer of resource provider.
> + * @idx: The index for the RCU critical section.
> + */
> +struct revocable {
> +	struct revocable_provider *rp;
> +	int idx;
> +};
> +
> +/**
> + * revocable_provider_alloc() - Allocate struct revocable_provider.
> + * @res: The pointer of resource.
> + *
> + * This holds an initial refcount to the struct.
> + *
> + * Return: The pointer of struct revocable_provider.  NULL on errors.
> + */
> +struct revocable_provider *revocable_provider_alloc(void *res)
> +{
> +	struct revocable_provider *rp;
> +
> +	rp = kzalloc(sizeof(*rp), GFP_KERNEL);
> +	if (!rp)
> +		return NULL;
> +
> +	init_srcu_struct(&rp->srcu);
> +	rcu_assign_pointer(rp->res, res);
> +	synchronize_srcu(&rp->srcu);
> +	kref_init(&rp->kref);
> +
> +	return rp;
> +}
> +EXPORT_SYMBOL_GPL(revocable_provider_alloc);
> +
> +static void revocable_provider_release(struct kref *kref)
> +{
> +	struct revocable_provider *rp = container_of(kref,
> +			struct revocable_provider, kref);
> +
> +	cleanup_srcu_struct(&rp->srcu);
> +	kfree(rp);
> +}
> +
> +/**
> + * revocable_provider_free() - Free struct revocable_provider.
> + * @rp: The pointer of resource provider.
> + *
> + * This sets the resource `(struct revocable_provider *)->res` to NULL to
> + * indicate the resource has gone.
> + *
> + * This drops the refcount to the resource provider.  If it is the final
> + * reference, revocable_provider_release() will be called to free the struct.
> + */
> +void revocable_provider_free(struct revocable_provider *rp)
> +{
> +	rcu_assign_pointer(rp->res, NULL);
> +	synchronize_srcu(&rp->srcu);
> +	kref_put(&rp->kref, revocable_provider_release);
> +}
> +EXPORT_SYMBOL_GPL(revocable_provider_free);
> +
> +static void devm_revocable_provider_free(void *data)
> +{
> +	struct revocable_provider *rp = data;
> +
> +	revocable_provider_free(rp);
> +}
> +
> +/**
> + * devm_revocable_provider_alloc() - Dev-managed revocable_provider_alloc().
> + * @dev: The device.
> + * @res: The pointer of resource.
> + *
> + * It is convenient to allocate providers via this function if the @res is
> + * also tied to the lifetime of the @dev.  revocable_provider_free() will
> + * be called automatically when the device is unbound.
> + *
> + * This holds an initial refcount to the struct.
> + *
> + * Return: The pointer of struct revocable_provider.  NULL on errors.
> + */
> +struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
> +							 void *res)
> +{
> +	struct revocable_provider *rp;
> +
> +	rp = revocable_provider_alloc(res);
> +	if (!rp)
> +		return NULL;
> +
> +	if (devm_add_action_or_reset(dev, devm_revocable_provider_free, rp))
> +		return NULL;
> +
> +	return rp;
> +}
> +EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc);
> +
> +/**
> + * revocable_alloc() - Allocate struct revocable_provider.
> + * @rp: The pointer of resource provider.
> + *
> + * This holds a refcount to the resource provider.
> + *
> + * Return: The pointer of struct revocable_provider.  NULL on errors.
> + */
> +struct revocable *revocable_alloc(struct revocable_provider *rp)
> +{
> +	struct revocable *rev;
> +
> +	rev = kzalloc(sizeof(*rev), GFP_KERNEL);
> +	if (!rev)
> +		return NULL;
> +
> +	rev->rp = rp;
> +	kref_get(&rp->kref);
> +
> +	return rev;
> +}
> +EXPORT_SYMBOL_GPL(revocable_alloc);
> +
> +/**
> + * revocable_free() - Free struct revocable.
> + * @rev: The pointer of struct revocable.
> + *
> + * This drops a refcount to the resource provider.  If it is the final
> + * reference, revocable_provider_release() will be called to free the struct.
> + */
> +void revocable_free(struct revocable *rev)
> +{
> +	struct revocable_provider *rp = rev->rp;
> +
> +	kref_put(&rp->kref, revocable_provider_release);
> +	kfree(rev);
> +}
> +EXPORT_SYMBOL_GPL(revocable_free);
> +
> +/**
> + * revocable_try_access() - Try to access the resource.
> + * @rev: The pointer of struct revocable.
> + *
> + * This tries to de-reference to the resource and enters a RCU critical
> + * section.
> + *
> + * Return: The pointer to the resource.  NULL if the resource has gone.
> + */
> +void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu)
> +{
> +	struct revocable_provider *rp = rev->rp;
> +
> +	rev->idx = srcu_read_lock(&rp->srcu);
> +	return rcu_dereference(rp->res);
> +}
> +EXPORT_SYMBOL_GPL(revocable_try_access);
> +
> +/**
> + * revocable_release() - Stop accessing to the resource.
> + * @rev: The pointer of struct revocable.
> + *
> + * Call this function to indicate the resource is no longer used.  It exits
> + * the RCU critical section.
> + */
> +void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu)
> +{
> +	struct revocable_provider *rp = rev->rp;
> +
> +	srcu_read_unlock(&rp->srcu, rev->idx);
> +}
> +EXPORT_SYMBOL_GPL(revocable_release);
> diff --git a/include/linux/revocable.h b/include/linux/revocable.h
> new file mode 100644
> index 000000000000..17d9b7ce633d
> --- /dev/null
> +++ b/include/linux/revocable.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2025 Google LLC
> + */
> +
> +#ifndef __LINUX_REVOCABLE_H
> +#define __LINUX_REVOCABLE_H
> +
> +#include <linux/cleanup.h>
> +
> +struct device;
> +struct revocable;
> +struct revocable_provider;
> +
> +struct revocable_provider *revocable_provider_alloc(void *res);
> +void revocable_provider_free(struct revocable_provider *rp);
> +struct revocable_provider *devm_revocable_provider_alloc(struct device *dev,
> +							 void *res);
> +
> +struct revocable *revocable_alloc(struct revocable_provider *rp);
> +void revocable_free(struct revocable *rev);
> +void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu);
> +void revocable_release(struct revocable *rev) __releases(&rev->rp->srcu);
> +
> +DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
> +
> +#define _REVOCABLE(_rev, _label, _res)						\
> +	for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev;	\
> +	     (_res = revocable_try_access(_rev)) || true; ({ goto _label; }))	\
> +		if (0) {							\
> +_label:										\
> +			break;							\
> +		} else
> +
> +#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
> +
> +#endif /* __LINUX_REVOCABLE_H */
> -- 
> 2.51.0.384.g4c02a37b29-goog
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Greg Kroah-Hartman]

On Mon, Sep 22, 2025 at 02:40:10PM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
> > I fully agree with that, in C there is indeed no value of a revocable type when
> > subsystems can guarantee "sane unregistration semantics".
> 
> Indeed, I agree with your message. If I look at the ec_cros presented
> here, there is no reason for any revocable. In fact it already seems
> like an abuse of the idea.
> 
> The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev"
> that is alreay properly lifetime controlled - the platform is already
> removed during the remove of the cros_ec.c.
> 
> So, there is no need for a revocable that spans two drivers like this!

It's a very common pattern, you have a struct device, and a userspace
access, and need to "split" them apart, as you say below.  This logic
here handles that pattern.

See the many talks about this in the past at Plumbers and other
conferences, this isn't a new thing, and it isn't quite as simple as I
think you are making it out to be to solve.

> The bug is that cros_ec_chardev.c doesn't implement itself correctly
> and doesn't have a well designed remove() for something that creates a
> char dev. This issue should be entirely handled within
> cros_ec_chardev.c and not leak out to other files.
> 
> 1) Using a miscdevice means loosing out on any refcount managed
> memory. When using a file you need some per-device memory that lives
> for as long as all files are open. So don't use miscdev, the better
> pattern is:
> 
> struct chardev_data {
> 	struct device dev;
> 	struct cdev cdev;

Woah, no, that is totally wrong.

> Now you get to have a struct device linked refcount and a free
> function. The file can savely hold onto a chardev_data for its
> lifetime.

You have 2 different reference counts for the same structure.  A bug
that should never be done (yes, it's done a lot in the kernel, it's
wrong.)

And really, let's not abuse cdev anymore than it currently is please.
There's a reason that miscdevice returns just a pointer.  Right now cdev
can be used in 3-4 different ways, let's not come up with another way to
abuse that api :)

> 2) Add locking so the file can exist after the driver is removed:
> 
> struct chardev_data {
> [..]
> 	struct rw_semaphore sem;
> 	struct cros_ec_dev *ec_dev;
> };
> 
> Refcount the chardev_data::dev in the file operations open/release,
> refer to it via the private_data.
> 
> 3) At the start of every fop take the sem and check the ec_dev:
> 
> ACQUIRE(rwsem_read, ecdev_sem)(&data->sem);
> if (ACQUIRE_ERR(ecdev_sem) || !data->ec_dev)
>    return -ENODEV;
> 
> 4) After unregistering the cdev, but before destroying the device take
> the write side of the rwsem and NULL ec_dev.
> 
> 5) Purge all the devm usage from cros_ec_chardev, the only allocation
> is refcounted instead.
> 
> Simple. No need for any synchronize_srcu() for such a simple
> non-performance oriented driver.

There is no performance issue here, but there is lifetime rule logic
here that I really really do not want to have to "open code" for every
user.  revokable gives this to us in a simple way that we can "know" is
being used correctly.

And again, you can't have a single structure with two reference counts,
that's not allowed :)

> Yes, this can be made better, there is a bit too much boilerplate, but
> revocable is not the way for cros_ec.

I strongly disagree, sorry.

greg k-h

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Jason Gunthorpe]

On Mon, Sep 22, 2025 at 08:42:23PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Sep 22, 2025 at 02:40:10PM -0300, Jason Gunthorpe wrote:
> > On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
> > > I fully agree with that, in C there is indeed no value of a revocable type when
> > > subsystems can guarantee "sane unregistration semantics".
> > 
> > Indeed, I agree with your message. If I look at the ec_cros presented
> > here, there is no reason for any revocable. In fact it already seems
> > like an abuse of the idea.
> > 
> > The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev"
> > that is alreay properly lifetime controlled - the platform is already
> > removed during the remove of the cros_ec.c.
> > 
> > So, there is no need for a revocable that spans two drivers like this!
> 
> It's a very common pattern, you have a struct device, and a userspace
> access, and need to "split" them apart, as you say below.  This logic
> here handles that pattern.

This is two struct devices, two device drivers and a fops.  There is
no reason to trigger revoke from the parent driver to a child driver,
that's just mis-layering and obfuscation.

It already subtly relies on the parent not triggering revoke until the
child is removed because it added this to the cros-ec-chardev driver:

@@ -166,7 +183,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
	struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent);

 	if (!priv)
 		return -ENOMEM;
 
-	priv->ec_dev = ec_dev;
+	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);

It would UAF if the revoke is triggered by the parent driver at the
wrong time.

So why is the parent involved at all? Why does it have to violate
modularity?

> See the many talks about this in the past at Plumbers and other
> conferences, this isn't a new thing, and it isn't quite as simple as I
> think you are making it out to be to solve.

There are more complex situations, but this isn't one of them. All
this needs is to fence the file operations of a single cdev. I've
solved it enough times now to know exactly how simple this really is..

 > struct chardev_data {
> > 	struct device dev;
> > 	struct cdev cdev;
> 
> Woah, no, that is totally wrong.

Sigh. I'm sure we've had this exchange before. Please
re-read the documentation for cdev_device_add():

 * This function should be used whenever the struct cdev and the
 * struct device are members of the same structure whose lifetime is
 * managed by the struct device.    ^^^^^^^^^^^^

We created this helper specifically to clean up the refcount bugs
around cdev usage. It supports the above pattern which is the natural
and easy way to use cdev.

> And really, let's not abuse cdev anymore than it currently is please.
> There's a reason that miscdevice returns just a pointer.  Right now cdev
> can be used in 3-4 different ways, let's not come up with another way to
> abuse that api :)

It is true there are many abuses, but converting cdev users to use
cdev_device_add() is the right and best option IMHO.

miscdev is not a good option in cases like this because you don't get
a nice natural kref around the memory, among other limitations.

> There is no performance issue here, but there is lifetime rule logic
> here that I really really do not want to have to "open code" for every
> user.  revokable gives this to us in a simple way that we can "know" is
> being used correctly.

I strongly prefer Laurent's direction to add a helper file_operations
that automatically wraps all ops in a lock.

I think all this series does is create a weirdly named lock that
drivers still have to open code.

The fact the very first proposed user is already abusing it to
needlessly violate driver modularity is really depressing. Let's not
create another devm :\

Jason

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[dan.j.williams]

Laurent Pinchart wrote:
> On Fri, Sep 12, 2025 at 06:22:48PM +0200, Danilo Krummrich wrote:
> > On 9/12/25 4:54 PM, Laurent Pinchart wrote:
> > > On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
> > >> On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
> > >>> Either way, I think this patch series stands on its own, it doesn't
> > >>> require cdev to implement it, drivers can use it to wrap a cdev if they
> > >>> want to.  We have other structures that want to do this type of thing
> > >>> today as is proof with the rust implementation for the devm api.
> > >>
> > >> Yeah, I'm not against this going upstream. If more development is
> > >> needed for this to be usable in other parts of the kernel, that can be
> > >> done gradually. Literally no subsystem ever was perfect on day 1.
> > > 
> > > To be clear, I'm not against the API being merged for the use cases that
> > > would benefit from it, but I don't want to see drivers using it to
> > > protect from the cdev/unregistration race.
> > 
> > I mean, revocable is really a synchronization primitive in the end that
> > "revokes" access to some resource in a race free way.
> > 
> > So, technically, it probably belongs into lib/.
> > 
> > I think the reason it ended up in drivers/base/ is that one common use case is
> > to revoke a device resource from a driver when the device is unbound from this
> > driver; or in other words devres is an obvious user.
> > 
> > So, I think that any other API (cdev, devres, etc.) should  be built on top of it.
> 
> No issue with that. I'm sure there are people who have better knowledge
> than me when it comes to implementing the low-level primitive in the
> most efficient way. What I have lots of experience with is the impact of
> API design on drivers, and the API misuse (including through cargo-cult
> programming) this can generate. Let's design the API towards drivers
> correctly.

Note that I dropped the "managed_fops" [1] effort, targeted for use for
CXL, in favor of simply this in the CXL ioctl device shutdown path:

        cdev_device_del(&cxlmd->cdev, &cxlmd->dev);
        scoped_guard(rwsem_write, &cxl_memdev_rwsem)
		cxlmd->cxlds = NULL;
        put_device(&cxlmd->dev);

Pair that with:

        guard(rwsem_read)(&cxl_memdev_rwsem);
        cxlds = cxlmd->cxlds;
        if (cxlds)
                return __cxl_memdev_ioctl(cxlmd, cmd, arg);
	return -ENXIO;

...on the ioctl invocation side.

This "revocable" mechanism looks useful for other inter-driver resource
sharing, but not for the well known issues with cdev. For cdev and the
design pattern of "shutdown the ioctl path on a core-subsystem device
object that is also a chardev", just use cdev_device_add() with an
rwsem.

[1]: http://lore.kernel.org/all/CAPcyv4h74NjqcuUjv4zFKHAxio_bV0bngLoxP=ACw=JvMfq-UA@mail.gmail.com

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

[Tzung-Bi Shih]

On Sat, Sep 13, 2025 at 07:14:13PM +0300, Laurent Pinchart wrote:
> On Sat, Sep 13, 2025 at 11:55:45PM +0800, Tzung-Bi Shih wrote:
> > - Subsystems (like cdev) should build on this API to provide an easier
> >   interface for their drivers to manage revocable resources.
> > 
> > I'll create a PoC based on this.
> 
> I'm looking forward to that. Please let me know if there's anything you
> would like to discuss. I didn't dive deep in technical details in this
> thread, and I don't expect anyone to guess what I have in mind if I
> failed to express it :-) I'm very confident the cdev race condition can
> be fixed in a neat way, so let's do that.

Even though I think this isn't what you are looking for originally, please
take a look on the PoC attempt (5th - 7th patches) in [1].  Unlike [2], the
PoC allows fops to be exuected and defers to the driver to decide what to do
if the resource is unavailable.

[1] https://lore.kernel.org/chrome-platform/20250923075302.591026-1-tzungbi@kernel.org
[2] https://lore.kernel.org/all/161117153776.2853729.6944617921517514510.stgit@dwillia2-desk3.amr.corp.intel.com/