| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Mar 2023 12:00:02 -0500
From: Waiman Long <longman@...hat.com>
To: Schspa Shi <schspa@...il.com>, tglx@...utronix.de,
swboyd@...omium.org, linux@...ck-us.net, wuchi.zero@...il.com
Cc: linux-kernel@...r.kernel.org,
syzbot+5093ba19745994288b53@...kaller.appspotmail.com
Subject: Re: [PATCH 1/2] debugobject: fix concurrency issues with
is_static_object
On 3/3/23 11:19, Schspa Shi wrote:
> The is_static_object implementation relay on the initial state of the
> object. If multiple places are accessed concurrently, there is a
> probability that the debug object has been registered in the system, which
> will invalidate the judgment of is_static_object.
>
> The following is the scenario where the problem occurs:
>
> T0 T1
> =========================================================================
> mod_timer();
> debug_object_assert_init
> db = get_bucket((unsigned long) addr);
> raw_spin_lock_irqsave(&db->lock, flags);
> obj = lookup_object(addr, db);
> if (!obj) {
> raw_spin_unlock_irqrestore(&db->lock, flags);
> << Context switch >>
> mod_timer();
> debug_object_assert_init
> ...
> enqueue_timer();
> /*
> * The initial state changed a static timer object, and
> * is_static_object will return false
> */
>
> if (descr->is_static_object &&
> descr->is_static_object(addr)) {
> debug_object_init();
> } else {
> << Hit here for a static object >>
> debug_print_object(&o, "assert_init");
> debug_object_fixup(descr->fixup_assert_init, addr,
> ODEBUG_STATE_NOTAVAILABLE);
> }
> }
>
> To fix it, we got the is_static_object called within db->lock, and save
> it's state to struct debug_obj. This will ensure we won't hit the code
> branch not belong to the static object.
>
> For the same static object, debug_object_init may enter multiple times, but
> there is a lock in debug_object_init to ensure that there is no problem.
>
> Reported-by: syzbot+5093ba19745994288b53@...kaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
> Signed-off-by: Schspa Shi <schspa@...il.com>
> ---
> include/linux/debugobjects.h | 1 +
> lib/debugobjects.c | 71 ++++++++++++++++++++++++++++--------
> 2 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
> index 32444686b6ff4..544a6111b97f6 100644
> --- a/include/linux/debugobjects.h
> +++ b/include/linux/debugobjects.h
> @@ -30,6 +30,7 @@ struct debug_obj {
> enum debug_obj_state state;
> unsigned int astate;
> void *object;
> + bool is_static;
> const struct debug_obj_descr *descr;
> };
The patch looks reasonable. My main concern is the increase in size of
the debug_obj structure. It is an additional 8 bytes on 64-bit arches.
How much will we save performance-wise by caching it in the debug_obj.
Alternatively, you may pack it within the current size by, maybe,
reducing the size of state.
Cheers,
Longman
>
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index df86e649d8be0..d1be18158a1f7 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -275,6 +275,8 @@ alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *d
> obj->descr = descr;
> obj->state = ODEBUG_STATE_NONE;
> obj->astate = 0;
> + obj->is_static = descr->is_static_object &&
> + descr->is_static_object(addr);
> hlist_add_head(&obj->node, &b->list);
> }
> return obj;
> @@ -581,7 +583,16 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
> debug_objects_oom();
> return;
> }
> +
> check_stack = true;
> + } else {
> + /*
> + * The debug object is inited, and we should check this again
> + */
> + if (obj->is_static) {
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + return;
> + }
> }
>
> switch (obj->state) {
> @@ -640,6 +651,29 @@ void debug_object_init_on_stack(void *addr, const struct debug_obj_descr *descr)
> }
> EXPORT_SYMBOL_GPL(debug_object_init_on_stack);
>
> +/*
> + * Check static object.
> + */
> +static bool debug_object_check_static(struct debug_bucket *db, void *addr,
> + const struct debug_obj_descr *descr)
> +{
> + struct debug_obj *obj;
> +
> + /*
> + * The is_static_object implementation relay on the initial state of the
> + * object. If multiple places are accessed concurrently, there is a
> + * probability that the debug object has been registered in the system,
> + * which will invalidate the judgment of is_static_object.
> + */
> + lockdep_assert_held(&db->lock);
> +
> + obj = lookup_object(addr, db);
> + if (likely(obj))
> + return obj->is_static;
> +
> + return descr->is_static_object && descr->is_static_object(addr);
> +}
> +
> /**
> * debug_object_activate - debug checks when an object is activated
> * @addr: address of the object
> @@ -656,6 +690,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
> struct debug_obj o = { .object = addr,
> .state = ODEBUG_STATE_NOTAVAILABLE,
> .descr = descr };
> + bool is_static;
>
> if (!debug_objects_enabled)
> return 0;
> @@ -696,6 +731,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
> return ret;
> }
>
> + is_static = debug_object_check_static(db, addr, descr);
> raw_spin_unlock_irqrestore(&db->lock, flags);
>
> /*
> @@ -705,7 +741,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
> * static object is tracked in the object tracker. If
> * not, this must be a bug, so we try to fix it up.
> */
> - if (descr->is_static_object && descr->is_static_object(addr)) {
> + if (is_static) {
> /* track this static object */
> debug_object_init(addr, descr);
> debug_object_activate(addr, descr);
> @@ -872,6 +908,7 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
> struct debug_bucket *db;
> struct debug_obj *obj;
> unsigned long flags;
> + bool is_static;
>
> if (!debug_objects_enabled)
> return;
> @@ -886,13 +923,14 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
> .state = ODEBUG_STATE_NOTAVAILABLE,
> .descr = descr };
>
> + is_static = debug_object_check_static(db, addr, descr);
> raw_spin_unlock_irqrestore(&db->lock, flags);
> /*
> * Maybe the object is static, and we let the type specific
> * code confirm. Track this static object if true, else invoke
> * fixup.
> */
> - if (descr->is_static_object && descr->is_static_object(addr)) {
> + if (is_static) {
> /* Track this static object */
> debug_object_init(addr, descr);
> } else {
> @@ -1215,7 +1253,8 @@ static __initconst const struct debug_obj_descr descr_type_test = {
> .fixup_free = fixup_free,
> };
>
> -static __initdata struct self_test obj = { .static_init = 0 };
> +static struct self_test obj __initdata = { .static_init = 0 };
> +static struct self_test sobj __initdata = { .static_init = 1 };
>
> static void __init debug_objects_selftest(void)
> {
> @@ -1256,26 +1295,26 @@ static void __init debug_objects_selftest(void)
> if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
> goto out;
>
> - obj.static_init = 1;
> - debug_object_activate(&obj, &descr_type_test);
> - if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> + debug_object_init(&sobj, &descr_type_test);
> + debug_object_activate(&sobj, &descr_type_test);
> + if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> goto out;
> - debug_object_init(&obj, &descr_type_test);
> - if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
> + debug_object_init(&sobj, &descr_type_test);
> + if (check_results(&sobj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
> goto out;
> - debug_object_free(&obj, &descr_type_test);
> - if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
> + debug_object_free(&sobj, &descr_type_test);
> + if (check_results(&sobj, ODEBUG_STATE_NONE, fixups, warnings))
> goto out;
>
> #ifdef CONFIG_DEBUG_OBJECTS_FREE
> - debug_object_init(&obj, &descr_type_test);
> - if (check_results(&obj, ODEBUG_STATE_INIT, fixups, warnings))
> + debug_object_init(&sobj, &descr_type_test);
> + if (check_results(&sobj, ODEBUG_STATE_INIT, fixups, warnings))
> goto out;
> - debug_object_activate(&obj, &descr_type_test);
> - if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> + debug_object_activate(&sobj, &descr_type_test);
> + if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> goto out;
> - __debug_check_no_obj_freed(&obj, sizeof(obj));
> - if (check_results(&obj, ODEBUG_STATE_NONE, ++fixups, ++warnings))
> + __debug_check_no_obj_freed(&sobj, sizeof(sobj));
> + if (check_results(&sobj, ODEBUG_STATE_NONE, ++fixups, ++warnings))
> goto out;
> #endif
> pr_info("selftest passed\n");
Powered by blists - more mailing lists