| 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
| ||
|
Message-ID: <m28rfomyoi.fsf@gmail.com>
Date: Thu, 23 Mar 2023 02:18:10 +0800
From: Schspa Shi <schspa@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: longman@...hat.com, swboyd@...omium.org, linux@...ck-us.net,
wuchi.zero@...il.com, linux-kernel@...r.kernel.org,
syzbot+5093ba19745994288b53@...kaller.appspotmail.com
Subject: Re: [PATCH 1/2] debugobject: fix concurrency issues with
is_static_object
Thomas Gleixner <tglx@...utronix.de> writes:
> On Wed, Mar 22 2023 at 18:46, Thomas Gleixner wrote:
>> On Wed, Mar 22 2023 at 23:40, Schspa Shi wrote:
>>> I think we should introduce lookup_object_or_alloc and is_static at the
>>> same time.
>>
>> What for?
>
> The below has the NONE/INIT issue addressed and plugs that race
> completely, so no further action required.
>
> Thanks,
>
> tglx
> ---
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -216,10 +216,6 @@ static struct debug_obj *__alloc_object(
> return obj;
> }
>
> -/*
> - * Allocate a new object. If the pool is empty, switch off the debugger.
> - * Must be called with interrupts disabled.
> - */
> static struct debug_obj *
> alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *descr)
> {
> @@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi
> WARN_ON(1);
> }
>
> +static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b,
> + const struct debug_obj_descr *descr,
> + bool onstack, bool alloc_ifstatic)
> +{
> + struct debug_obj *obj = lookup_object(addr, b);
> + enum debug_obj_state state = ODEBUG_STATE_NONE;
> +
> + if (likely(obj))
> + return obj;
> +
> + /*
> + * debug_object_init() unconditionally allocates untracked
> + * objects. It does not matter whether it is a static object or
> + * not.
> + *
> + * debug_object_assert_init() and debug_object_activate() allow
> + * allocation only if the descriptor callback confirms that the
> + * object is static and considered initialized. For non-static
> + * objects the allocation needs to be done from the fixup callback.
> + */
> + if (unlikely(alloc_ifstatic)) {
> + if (!descr->is_static_object || !descr->is_static_object(addr))
> + return ERR_PTR(-ENOENT);
> + /* Statically allocated objects are considered initialized */
> + state = ODEBUG_STATE_INIT;
> + }
> +
> + obj = alloc_object(addr, b, descr);
> + if (likely(obj)) {
> + obj->state = state;
> + debug_object_is_on_stack(addr, onstack);
> + return obj;
> + }
> +
> + /* Out of memory. Do the cleanup outside of the locked region */
> + debug_objects_enabled = 0;
> + return NULL;
> +}
> +
> static void
> __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
> {
> enum debug_obj_state state;
> - bool check_stack = false;
> struct debug_bucket *db;
> struct debug_obj *obj;
> unsigned long flags;
> @@ -572,16 +606,11 @@ static void
>
> raw_spin_lock_irqsave(&db->lock, flags);
>
> - obj = lookup_object(addr, db);
> - if (!obj) {
> - obj = alloc_object(addr, db, descr);
> - if (!obj) {
> - debug_objects_enabled = 0;
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_objects_oom();
> - return;
> - }
> - check_stack = true;
> + obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
> + if (unlikely(!obj)) {
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + debug_objects_oom();
> + return;
> }
>
> switch (obj->state) {
> @@ -607,8 +636,6 @@ static void
> }
>
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - if (check_stack)
> - debug_object_is_on_stack(addr, onstack);
> }
>
> /**
> @@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s
> */
> int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
> {
> + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
> enum debug_obj_state state;
> struct debug_bucket *db;
> struct debug_obj *obj;
> unsigned long flags;
> int ret;
> - struct debug_obj o = { .object = addr,
> - .state = ODEBUG_STATE_NOTAVAILABLE,
> - .descr = descr };
>
> if (!debug_objects_enabled)
> return 0;
> @@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co
>
> raw_spin_lock_irqsave(&db->lock, flags);
>
> - obj = lookup_object(addr, db);
> - if (obj) {
> + obj = lookup_object_or_alloc(addr, db, descr, false, true);
> + if (likely(!IS_ERR_OR_NULL(obj))) {
> bool print_object = false;
>
> switch (obj->state) {
> @@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co
>
> raw_spin_unlock_irqrestore(&db->lock, flags);
>
> - /*
> - * We are here when a static object is activated. We
> - * let the type specific code confirm whether this is
> - * true or not. if true, we just make sure that the
> - * 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)) {
> - /* track this static object */
> - debug_object_init(addr, descr);
> - debug_object_activate(addr, descr);
> - } else {
> - debug_print_object(&o, "activate");
> - ret = debug_object_fixup(descr->fixup_activate, addr,
> - ODEBUG_STATE_NOTAVAILABLE);
> - return ret ? 0 : -EINVAL;
> + /* If NULL the allocaction has hit OOM */
> + if (!obj) {
> + debug_objects_oom();
> + return 0;
> }
> - return 0;
> +
> + /* Object is neither static nor tracked. It's not initialized */
> + debug_print_object(&o, "activate");
> + ret = debug_object_fixup(descr->fixup_activate, addr, ODEBUG_STATE_NOTAVAILABLE);
> + return ret ? 0 : -EINVAL;
> }
> EXPORT_SYMBOL_GPL(debug_object_activate);
>
> @@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free);
> */
> void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
> {
> + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
> struct debug_bucket *db;
> struct debug_obj *obj;
> unsigned long flags;
> @@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr
> db = get_bucket((unsigned long) addr);
>
> raw_spin_lock_irqsave(&db->lock, flags);
> + obj = lookup_object_or_alloc(addr, db, descr, false, true);
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + if (likely(!IS_ERR_OR_NULL(obj)))
> + return;
>
> - obj = lookup_object(addr, db);
> + /* If NULL the allocaction has hit OOM */
> if (!obj) {
> - struct debug_obj o = { .object = addr,
> - .state = ODEBUG_STATE_NOTAVAILABLE,
> - .descr = 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)) {
> - /* Track this static object */
> - debug_object_init(addr, descr);
> - } else {
> - debug_print_object(&o, "assert_init");
> - debug_object_fixup(descr->fixup_assert_init, addr,
> - ODEBUG_STATE_NOTAVAILABLE);
> - }
> + debug_objects_oom();
> return;
> }
>
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> + /* Object is neither tracked nor static. It's not initialized. */
> + debug_print_object(&o, "assert_init");
> + debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE);
> }
> EXPORT_SYMBOL_GPL(debug_object_assert_init);
>
It's OK if you don't think debug_object_free() should report a error when
the object is static. But I still think we should report an error and
modify the autotest case in debug_objects_selftest() to remove
debug_object_free() call on the static object.
--
BRs
Schspa Shi
Powered by blists - more mailing lists