From 4d7dc4d1a62dbb22b1178dddeeb7a22d0272df77 Mon Sep 17 00:00:00 2001 From: Tzung-Bi Shih Date: Thu, 29 Jan 2026 14:37:30 +0000 Subject: revocable: Fix races in revocable_alloc() using RCU There are two race conditions when allocating a revocable instance: 1. After a struct revocable_provider is revoked, the caller might still hold a dangling pointer to it. A subsequent call to revocable_alloc() can trigger a use-after-free. 2. If revocable_provider_release() runs concurrently with revocable_alloc(), the memory of struct revocable_provider can be accessed during or after kfree(). To fix these: - Manage the lifetime of struct revocable_provider using RCU. Annotate pointers to it with __rcu and use kfree_rcu() for deallocation. - Update revocable_alloc() to safely acquire a reference using RCU primitives. - Update revocable_provider_revoke() to take a double pointer (`**rp`). It atomically NULLs out the caller's pointer before starting revocation. This prevents the caller from holding a dangling pointer. - Drop devm_revocable_provider_alloc(). The devm-managed model cannot support the required double-pointer semantic for safe pointer nulling. Reported-by: Johan Hovold Closes: https://lore.kernel.org/all/aXdy-b3GOJkzGqYo@hovoldconsulting.com/ Signed-off-by: Tzung-Bi Shih Link: https://patch.msgid.link/20260129143733.45618-2-tzungbi@kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/base/revocable.c | 93 ++++++++++++++++++++++--------------------- drivers/base/revocable_test.c | 20 ++++++---- 2 files changed, 59 insertions(+), 54 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c index b068e18a847d..1bcd1cf54764 100644 --- a/drivers/base/revocable.c +++ b/drivers/base/revocable.c @@ -64,11 +64,13 @@ * @srcu: The SRCU to protect the resource. * @res: The pointer of resource. It can point to anything. * @kref: The refcount for this handle. + * @rcu: The RCU to protect pointer to itself. */ struct revocable_provider { struct srcu_struct srcu; void __rcu *res; struct kref kref; + struct rcu_head rcu; }; /** @@ -88,8 +90,9 @@ struct revocable { * This holds an initial refcount to the struct. * * Return: The pointer of struct revocable_provider. NULL on errors. + * It enforces the caller handles the returned pointer in RCU ways. */ -struct revocable_provider *revocable_provider_alloc(void *res) +struct revocable_provider __rcu *revocable_provider_alloc(void *res) { struct revocable_provider *rp; @@ -98,10 +101,10 @@ struct revocable_provider *revocable_provider_alloc(void *res) return NULL; init_srcu_struct(&rp->srcu); - rcu_assign_pointer(rp->res, res); + RCU_INIT_POINTER(rp->res, res); kref_init(&rp->kref); - return rp; + return (struct revocable_provider __rcu *)rp; } EXPORT_SYMBOL_GPL(revocable_provider_alloc); @@ -111,82 +114,80 @@ static void revocable_provider_release(struct kref *kref) struct revocable_provider, kref); cleanup_srcu_struct(&rp->srcu); - kfree(rp); + kfree_rcu(rp, rcu); } /** * revocable_provider_revoke() - Revoke the managed resource. - * @rp: The pointer of resource provider. + * @rp_ptr: The pointer of 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_revoke(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_revoke); - -static void devm_revocable_provider_revoke(void *data) -{ - struct revocable_provider *rp = data; - - revocable_provider_revoke(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_revoke() 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. + * It enforces the caller to pass a pointer of pointer of resource provider so + * that it sets \*rp_ptr to NULL to prevent from keeping a dangling pointer. */ -struct revocable_provider *devm_revocable_provider_alloc(struct device *dev, - void *res) +void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr) { struct revocable_provider *rp; - rp = revocable_provider_alloc(res); + rp = rcu_replace_pointer(*rp_ptr, NULL, 1); if (!rp) - return NULL; + return; - if (devm_add_action_or_reset(dev, devm_revocable_provider_revoke, rp)) - return NULL; - - return rp; + rcu_assign_pointer(rp->res, NULL); + synchronize_srcu(&rp->srcu); + kref_put(&rp->kref, revocable_provider_release); } -EXPORT_SYMBOL_GPL(devm_revocable_provider_alloc); +EXPORT_SYMBOL_GPL(revocable_provider_revoke); /** * revocable_alloc() - Allocate struct revocable. - * @rp: The pointer of resource provider. + * @_rp: The pointer of resource provider. * * This holds a refcount to the resource provider. * * Return: The pointer of struct revocable. NULL on errors. */ -struct revocable *revocable_alloc(struct revocable_provider *rp) +struct revocable *revocable_alloc(struct revocable_provider __rcu *_rp) { + struct revocable_provider *rp; struct revocable *rev; + if (!_rp) + return NULL; + + /* + * Enter a read-side critical section. + * + * This prevents kfree_rcu() from freeing the struct revocable_provider + * memory, for the duration of this scope. + */ + scoped_guard(rcu) { + rp = rcu_dereference(_rp); + if (!rp) + /* The revocable provider has been revoked. */ + return NULL; + + if (!kref_get_unless_zero(&rp->kref)) + /* + * The revocable provider is releasing (i.e., + * revocable_provider_release() has been called). + */ + return NULL; + } + /* At this point, `rp` is safe to access as holding a kref of it */ + rev = kzalloc(sizeof(*rev), GFP_KERNEL); - if (!rev) + if (!rev) { + kref_put(&rp->kref, revocable_provider_release); return NULL; + } rev->rp = rp; - kref_get(&rp->kref); - return rev; } EXPORT_SYMBOL_GPL(revocable_alloc); diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c index 873a44082b6c..1622aae92fd3 100644 --- a/drivers/base/revocable_test.c +++ b/drivers/base/revocable_test.c @@ -21,7 +21,7 @@ static void revocable_test_basic(struct kunit *test) { - struct revocable_provider *rp; + struct revocable_provider __rcu *rp; struct revocable *rev; void *real_res = (void *)0x12345678, *res; @@ -36,12 +36,13 @@ static void revocable_test_basic(struct kunit *test) revocable_withdraw_access(rev); revocable_free(rev); - revocable_provider_revoke(rp); + revocable_provider_revoke(&rp); + KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL); } static void revocable_test_revocation(struct kunit *test) { - struct revocable_provider *rp; + struct revocable_provider __rcu *rp; struct revocable *rev; void *real_res = (void *)0x12345678, *res; @@ -55,7 +56,8 @@ static void revocable_test_revocation(struct kunit *test) KUNIT_EXPECT_PTR_EQ(test, res, real_res); revocable_withdraw_access(rev); - revocable_provider_revoke(rp); + revocable_provider_revoke(&rp); + KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL); res = revocable_try_access(rev); KUNIT_EXPECT_PTR_EQ(test, res, NULL); @@ -66,7 +68,7 @@ static void revocable_test_revocation(struct kunit *test) static void revocable_test_try_access_macro(struct kunit *test) { - struct revocable_provider *rp; + struct revocable_provider __rcu *rp; struct revocable *rev; void *real_res = (void *)0x12345678, *res; @@ -81,7 +83,8 @@ static void revocable_test_try_access_macro(struct kunit *test) KUNIT_EXPECT_PTR_EQ(test, res, real_res); } - revocable_provider_revoke(rp); + revocable_provider_revoke(&rp); + KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL); { REVOCABLE_TRY_ACCESS_WITH(rev, res); @@ -93,7 +96,7 @@ static void revocable_test_try_access_macro(struct kunit *test) static void revocable_test_try_access_macro2(struct kunit *test) { - struct revocable_provider *rp; + struct revocable_provider __rcu *rp; struct revocable *rev; void *real_res = (void *)0x12345678, *res; bool accessed; @@ -111,7 +114,8 @@ static void revocable_test_try_access_macro2(struct kunit *test) } KUNIT_EXPECT_TRUE(test, accessed); - revocable_provider_revoke(rp); + revocable_provider_revoke(&rp); + KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL); accessed = false; REVOCABLE_TRY_ACCESS_SCOPED(rev, res) { -- cgit v1.2.3