* [PATCH v2 1/5] revocable: Revocable resource management
2025-08-20 8:16 [PATCH v2 0/5] platform/chrome: Fix a possible UAF via revocable Tzung-Bi Shih
@ 2025-08-20 8:16 ` Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 2/5] revocable: Add Kunit test cases Tzung-Bi Shih
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2025-08-20 8:16 UTC (permalink / raw)
To: Benson Leung, Greg Kroah-Hartman, Rafael J . Wysocki,
Danilo Krummrich
Cc: Jonathan Corbet, Shuah Khan, Dawid Niedzwiecki, linux-doc,
linux-kernel, chrome-platform, linux-kselftest, tzungbi
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>
---
v2:
- 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 e913c1edd1fd..3bc39685bcf3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21663,6 +21663,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.rc1.167.g924127e9c0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/5] revocable: Add Kunit test cases
2025-08-20 8:16 [PATCH v2 0/5] platform/chrome: Fix a possible UAF via revocable Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 1/5] revocable: Revocable resource management Tzung-Bi Shih
@ 2025-08-20 8:16 ` Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 3/5] selftests: revocable: Add kselftest cases Tzung-Bi Shih
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2025-08-20 8:16 UTC (permalink / raw)
To: Benson Leung, Greg Kroah-Hartman, Rafael J . Wysocki,
Danilo Krummrich
Cc: Jonathan Corbet, Shuah Khan, Dawid Niedzwiecki, linux-doc,
linux-kernel, chrome-platform, linux-kselftest, tzungbi
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>
---
v2:
- 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 3bc39685bcf3..e8a23ba2e2a4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21668,6 +21668,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.rc1.167.g924127e9c0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 3/5] selftests: revocable: Add kselftest cases
2025-08-20 8:16 [PATCH v2 0/5] platform/chrome: Fix a possible UAF via revocable Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 1/5] revocable: Revocable resource management Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 2/5] revocable: Add Kunit test cases Tzung-Bi Shih
@ 2025-08-20 8:16 ` Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 4/5] platform/chrome: Protect cros_ec_device lifecycle with revocable Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 5/5] platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable Tzung-Bi Shih
4 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2025-08-20 8:16 UTC (permalink / raw)
To: Benson Leung, Greg Kroah-Hartman, Rafael J . Wysocki,
Danilo Krummrich
Cc: Jonathan Corbet, Shuah Khan, Dawid Niedzwiecki, linux-doc,
linux-kernel, chrome-platform, linux-kselftest, tzungbi
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>
---
v2:
- 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 e8a23ba2e2a4..81281b828979 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21670,6 +21670,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 babed7b1c2d1..efdc7bacab9e 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.rc1.167.g924127e9c0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 4/5] platform/chrome: Protect cros_ec_device lifecycle with revocable
2025-08-20 8:16 [PATCH v2 0/5] platform/chrome: Fix a possible UAF via revocable Tzung-Bi Shih
` (2 preceding siblings ...)
2025-08-20 8:16 ` [PATCH v2 3/5] selftests: revocable: Add kselftest cases Tzung-Bi Shih
@ 2025-08-20 8:16 ` Tzung-Bi Shih
2025-08-20 8:16 ` [PATCH v2 5/5] platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable Tzung-Bi Shih
4 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2025-08-20 8:16 UTC (permalink / raw)
To: Benson Leung, Greg Kroah-Hartman, Rafael J . Wysocki,
Danilo Krummrich
Cc: Jonathan Corbet, Shuah Khan, Dawid Niedzwiecki, linux-doc,
linux-kernel, chrome-platform, linux-kselftest, tzungbi
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>
---
v2:
- Rename "ref_proxy" -> "revocable".
v1: https://lore.kernel.org/chrome-platform/20250814091020.1302888-3-tzungbi@kernel.org
drivers/platform/chrome/cros_ec_i2c.c | 5 +++++
drivers/platform/chrome/cros_ec_ishtp.c | 5 +++++
drivers/platform/chrome/cros_ec_lpc.c | 5 +++++
drivers/platform/chrome/cros_ec_rpmsg.c | 5 +++++
drivers/platform/chrome/cros_ec_spi.c | 4 ++++
drivers/platform/chrome/cros_ec_uart.c | 5 +++++
include/linux/platform_data/cros_ec_proto.h | 4 ++++
7 files changed, 33 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index 38af97cdaab2..68a8d2c65ca3 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -12,6 +12,7 @@
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
#include <linux/platform_device.h>
+#include <linux/revocable.h>
#include <linux/slab.h>
#include "cros_ec.h"
@@ -296,6 +297,10 @@ static int cros_ec_i2c_probe(struct i2c_client *client)
if (!ec_dev)
return -ENOMEM;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+ if (!ec_dev->revocable_provider)
+ return -ENOMEM;
+
i2c_set_clientdata(client, ec_dev);
ec_dev->dev = dev;
ec_dev->priv = client;
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index 7e7190b30cbb..3467b4e9ceb5 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -12,6 +12,7 @@
#include <linux/pci.h>
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/revocable.h>
#include <linux/intel-ish-client-if.h>
#include "cros_ec.h"
@@ -547,6 +548,10 @@ static int cros_ec_dev_init(struct ishtp_cl_data *client_data)
if (!ec_dev)
return -ENOMEM;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+ if (!ec_dev->revocable_provider)
+ return -ENOMEM;
+
client_data->ec_dev = ec_dev;
dev->driver_data = ec_dev;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 7d9a78289c96..77bdaf430979 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -23,6 +23,7 @@
#include <linux/platform_device.h>
#include <linux/printk.h>
#include <linux/reboot.h>
+#include <linux/revocable.h>
#include <linux/suspend.h>
#include "cros_ec.h"
@@ -641,6 +642,10 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
if (!ec_dev)
return -ENOMEM;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+ if (!ec_dev->revocable_provider)
+ return -ENOMEM;
+
platform_set_drvdata(pdev, ec_dev);
ec_dev->dev = dev;
ec_dev->phys_name = dev_name(dev);
diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
index bc2666491db1..04d886829aa7 100644
--- a/drivers/platform/chrome/cros_ec_rpmsg.c
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -10,6 +10,7 @@
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
#include <linux/platform_device.h>
+#include <linux/revocable.h>
#include <linux/rpmsg.h>
#include <linux/slab.h>
@@ -220,6 +221,10 @@ static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
if (!ec_dev)
return -ENOMEM;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+ if (!ec_dev->revocable_provider)
+ return -ENOMEM;
+
ec_rpmsg = devm_kzalloc(dev, sizeof(*ec_rpmsg), GFP_KERNEL);
if (!ec_rpmsg)
return -ENOMEM;
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8ca0f854e7ac..1b611ec5c9c1 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -10,6 +10,7 @@
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
#include <linux/platform_device.h>
+#include <linux/revocable.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <uapi/linux/sched/types.h>
@@ -752,6 +753,9 @@ static int cros_ec_spi_probe(struct spi_device *spi)
ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
if (!ec_dev)
return -ENOMEM;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+ if (!ec_dev->revocable_provider)
+ return -ENOMEM;
/* Check for any DT properties */
cros_ec_spi_dt_probe(ec_spi, dev);
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 19c179d49c90..b234f13a92a9 100644
--- a/drivers/platform/chrome/cros_ec_uart.c
+++ b/drivers/platform/chrome/cros_ec_uart.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/revocable.h>
#include <linux/serdev.h>
#include <linux/slab.h>
#include <uapi/linux/sched/types.h>
@@ -263,6 +264,10 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
if (!ec_dev)
return -ENOMEM;
+ ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
+ if (!ec_dev->revocable_provider)
+ return -ENOMEM;
+
serdev_device_set_drvdata(serdev, ec_dev);
init_waitqueue_head(&ec_uart->response.wait_queue);
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 3ec24f445c29..0c3df0bb0aa4 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>
@@ -158,6 +159,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 */
@@ -203,6 +205,8 @@ struct cros_ec_device {
struct platform_device *pd;
struct blocking_notifier_head panic_notifier;
+
+ struct revocable_provider *revocable_provider;
};
/**
--
2.51.0.rc1.167.g924127e9c0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 5/5] platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable
2025-08-20 8:16 [PATCH v2 0/5] platform/chrome: Fix a possible UAF via revocable Tzung-Bi Shih
` (3 preceding siblings ...)
2025-08-20 8:16 ` [PATCH v2 4/5] platform/chrome: Protect cros_ec_device lifecycle with revocable Tzung-Bi Shih
@ 2025-08-20 8:16 ` Tzung-Bi Shih
4 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2025-08-20 8:16 UTC (permalink / raw)
To: Benson Leung, Greg Kroah-Hartman, Rafael J . Wysocki,
Danilo Krummrich
Cc: Jonathan Corbet, Shuah Khan, Dawid Niedzwiecki, linux-doc,
linux-kernel, chrome-platform, linux-kselftest, tzungbi
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>
---
v2:
- 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..d41c7f574cf1 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 err;
+ }
+
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 err;
}
+ return 0;
+err:
+ if (priv->ec_dev_rev)
+ revocable_free(priv->ec_dev_rev);
+ 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.rc1.167.g924127e9c0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread