| 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: <2f72313b-2fb4-4f62-a9d7-3fe05f1051c4@intel.com>
Date: Wed, 15 Nov 2023 13:33:17 +0100
From: Andrzej Hajda <andrzej.hajda@...el.com>
To: linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
linux-mm@...ck.org, Thomas Gleixner <tglx@...utronix.de>
Cc: Nirmoy Das <nirmoy.das@...el.com>
Subject: Re: [Intel-gfx] [PATCH v3] debugobjects: stop accessing objects after
releasing spinlock
On 25.10.2023 23:39, Andrzej Hajda wrote:
> After spinlock release object can be modified/freed by concurrent thread.
> Using it in such case is error prone, even for printing object state.
> To avoid such situation local copy of the object is created if necessary.
>
> Sample buggy scenario:
> 1. Thread tries to deactivate destroyed object, debugobjects detects it,
> spin lock is released, thread is preempted.
> 2. Other thread frees debugobject, then allocates new one on the same memory
> location, ie 'obj' variable from 1st thread point to it - it is possible
> because there is no locking.
> 3. Then preemption occurs, and 1st thread reports error for wrong object.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@...el.com>
> ---
> v2: add missing switch breaks
> v3: abandon single-point-of-unlock approach
Gently ping.
Regards
Andrzej
> ---
> lib/debugobjects.c | 196 +++++++++++++++++++++--------------------------------
> 1 file changed, 77 insertions(+), 119 deletions(-)
>
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index a517256a270b71..c074dbbec084a6 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void)
> static void
> __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
> {
> - enum debug_obj_state state;
> struct debug_bucket *db;
> - struct debug_obj *obj;
> + struct debug_obj *obj, o;
> unsigned long flags;
>
> debug_objects_fill_pool();
> @@ -643,24 +642,18 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
> case ODEBUG_STATE_INIT:
> case ODEBUG_STATE_INACTIVE:
> obj->state = ODEBUG_STATE_INIT;
> - break;
> -
> - case ODEBUG_STATE_ACTIVE:
> - state = obj->state;
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "init");
> - debug_object_fixup(descr->fixup_init, addr, state);
> - return;
> -
> - case ODEBUG_STATE_DESTROYED:
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "init");
> return;
> default:
> break;
> }
>
> + o = *obj;
> raw_spin_unlock_irqrestore(&db->lock, flags);
> + debug_print_object(&o, "init");
> +
> + if (o.state == ODEBUG_STATE_ACTIVE)
> + debug_object_fixup(descr->fixup_init, addr, o.state);
> }
>
> /**
> @@ -701,11 +694,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack);
> 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;
>
> if (!debug_objects_enabled)
> return 0;
> @@ -717,49 +708,38 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
> raw_spin_lock_irqsave(&db->lock, flags);
>
> obj = lookup_object_or_alloc(addr, db, descr, false, true);
> - if (likely(!IS_ERR_OR_NULL(obj))) {
> - bool print_object = false;
> -
> + if (unlikely(!obj)) {
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + debug_objects_oom();
> + return 0;
> + } else if (likely(!IS_ERR(obj))) {
> switch (obj->state) {
> - case ODEBUG_STATE_INIT:
> - case ODEBUG_STATE_INACTIVE:
> - obj->state = ODEBUG_STATE_ACTIVE;
> - ret = 0;
> - break;
> -
> case ODEBUG_STATE_ACTIVE:
> - state = obj->state;
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "activate");
> - ret = debug_object_fixup(descr->fixup_activate, addr, state);
> - return ret ? 0 : -EINVAL;
> -
> case ODEBUG_STATE_DESTROYED:
> - print_object = true;
> - ret = -EINVAL;
> break;
> + case ODEBUG_STATE_INIT:
> + case ODEBUG_STATE_INACTIVE:
> + obj->state = ODEBUG_STATE_ACTIVE;
> + fallthrough;
> default:
> - ret = 0;
> - break;
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + return 0;
> }
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - if (print_object)
> - debug_print_object(obj, "activate");
> - return ret;
> }
>
> + o = *obj;
> raw_spin_unlock_irqrestore(&db->lock, flags);
> + debug_print_object(&o, "activate");
>
> - /* If NULL the allocation has hit OOM */
> - if (!obj) {
> - debug_objects_oom();
> - return 0;
> + switch (o.state) {
> + case ODEBUG_STATE_ACTIVE:
> + case ODEBUG_STATE_NOTAVAILABLE:
> + if (debug_object_fixup(descr->fixup_activate, addr, o.state))
> + return 0;
> + fallthrough;
> + default:
> + return -EINVAL;
> }
> -
> - /* 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);
>
> @@ -770,10 +750,10 @@ EXPORT_SYMBOL_GPL(debug_object_activate);
> */
> void debug_object_deactivate(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;
> - bool print_object = false;
>
> if (!debug_objects_enabled)
> return;
> @@ -785,33 +765,24 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr)
> obj = lookup_object(addr, db);
> if (obj) {
> switch (obj->state) {
> + case ODEBUG_STATE_DESTROYED:
> + break;
> case ODEBUG_STATE_INIT:
> case ODEBUG_STATE_INACTIVE:
> case ODEBUG_STATE_ACTIVE:
> - if (!obj->astate)
> - obj->state = ODEBUG_STATE_INACTIVE;
> - else
> - print_object = true;
> - break;
> -
> - case ODEBUG_STATE_DESTROYED:
> - print_object = true;
> - break;
> + if (obj->astate)
> + break;
> + obj->state = ODEBUG_STATE_INACTIVE;
> + fallthrough;
> default:
> - break;
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + return;
> }
> + o = *obj;
> }
>
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - if (!obj) {
> - struct debug_obj o = { .object = addr,
> - .state = ODEBUG_STATE_NOTAVAILABLE,
> - .descr = descr };
> -
> - debug_print_object(&o, "deactivate");
> - } else if (print_object) {
> - debug_print_object(obj, "deactivate");
> - }
> + debug_print_object(&o, "deactivate");
> }
> EXPORT_SYMBOL_GPL(debug_object_deactivate);
>
> @@ -822,11 +793,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate);
> */
> void debug_object_destroy(void *addr, const struct debug_obj_descr *descr)
> {
> - enum debug_obj_state state;
> struct debug_bucket *db;
> - struct debug_obj *obj;
> + struct debug_obj *obj, o;
> unsigned long flags;
> - bool print_object = false;
>
> if (!debug_objects_enabled)
> return;
> @@ -836,32 +805,31 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr)
> raw_spin_lock_irqsave(&db->lock, flags);
>
> obj = lookup_object(addr, db);
> - if (!obj)
> - goto out_unlock;
> + if (!obj) {
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + return;
> + }
>
> switch (obj->state) {
> + case ODEBUG_STATE_ACTIVE:
> + case ODEBUG_STATE_DESTROYED:
> + break;
> case ODEBUG_STATE_NONE:
> case ODEBUG_STATE_INIT:
> case ODEBUG_STATE_INACTIVE:
> obj->state = ODEBUG_STATE_DESTROYED;
> - break;
> - case ODEBUG_STATE_ACTIVE:
> - state = obj->state;
> + fallthrough;
> + default:
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "destroy");
> - debug_object_fixup(descr->fixup_destroy, addr, state);
> return;
> -
> - case ODEBUG_STATE_DESTROYED:
> - print_object = true;
> - break;
> - default:
> - break;
> }
> -out_unlock:
> +
> + o = *obj;
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - if (print_object)
> - debug_print_object(obj, "destroy");
> + debug_print_object(&o, "destroy");
> +
> + if (o.state == ODEBUG_STATE_ACTIVE)
> + debug_object_fixup(descr->fixup_destroy, addr, o.state);
> }
> EXPORT_SYMBOL_GPL(debug_object_destroy);
>
> @@ -872,9 +840,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy);
> */
> void debug_object_free(void *addr, const struct debug_obj_descr *descr)
> {
> - enum debug_obj_state state;
> struct debug_bucket *db;
> - struct debug_obj *obj;
> + struct debug_obj *obj, o;
> unsigned long flags;
>
> if (!debug_objects_enabled)
> @@ -885,24 +852,26 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr)
> raw_spin_lock_irqsave(&db->lock, flags);
>
> obj = lookup_object(addr, db);
> - if (!obj)
> - goto out_unlock;
> + if (!obj) {
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + return;
> + }
>
> switch (obj->state) {
> case ODEBUG_STATE_ACTIVE:
> - state = obj->state;
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "free");
> - debug_object_fixup(descr->fixup_free, addr, state);
> - return;
> + break;
> default:
> hlist_del(&obj->node);
> raw_spin_unlock_irqrestore(&db->lock, flags);
> free_object(obj);
> return;
> }
> -out_unlock:
> +
> + o = *obj;
> raw_spin_unlock_irqrestore(&db->lock, flags);
> + debug_print_object(&o, "free");
> +
> + debug_object_fixup(descr->fixup_free, addr, o.state);
> }
> EXPORT_SYMBOL_GPL(debug_object_free);
>
> @@ -954,10 +923,10 @@ void
> debug_object_active_state(void *addr, const struct debug_obj_descr *descr,
> unsigned int expect, unsigned int next)
> {
> + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
> struct debug_bucket *db;
> struct debug_obj *obj;
> unsigned long flags;
> - bool print_object = false;
>
> if (!debug_objects_enabled)
> return;
> @@ -970,28 +939,20 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr,
> if (obj) {
> switch (obj->state) {
> case ODEBUG_STATE_ACTIVE:
> - if (obj->astate == expect)
> + if (obj->astate == expect) {
> obj->astate = next;
> - else
> - print_object = true;
> + raw_spin_unlock_irqrestore(&db->lock, flags);
> + return;
> + }
> break;
> -
> default:
> - print_object = true;
> break;
> }
> + o = *obj;
> }
>
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - if (!obj) {
> - struct debug_obj o = { .object = addr,
> - .state = ODEBUG_STATE_NOTAVAILABLE,
> - .descr = descr };
> -
> - debug_print_object(&o, "active_state");
> - } else if (print_object) {
> - debug_print_object(obj, "active_state");
> - }
> + debug_print_object(&o, "active_state");
> }
> EXPORT_SYMBOL_GPL(debug_object_active_state);
>
> @@ -999,11 +960,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state);
> static void __debug_check_no_obj_freed(const void *address, unsigned long size)
> {
> unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
> - const struct debug_obj_descr *descr;
> - enum debug_obj_state state;
> struct debug_bucket *db;
> struct hlist_node *tmp;
> - struct debug_obj *obj;
> + struct debug_obj *obj, o;
> int cnt, objs_checked = 0;
>
> saddr = (unsigned long) address;
> @@ -1026,12 +985,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>
> switch (obj->state) {
> case ODEBUG_STATE_ACTIVE:
> - descr = obj->descr;
> - state = obj->state;
> + o = *obj;
> raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "free");
> - debug_object_fixup(descr->fixup_free,
> - (void *) oaddr, state);
> + debug_print_object(&o, "free");
> + debug_object_fixup(o.descr->fixup_free,
> + (void *) oaddr, o.state);
> goto repeat;
> default:
> hlist_del(&obj->node);
>
> ---
> base-commit: 201c8a7bd1f3f415920a2df4b8a8817e973f42fe
> change-id: 20231025-debugobjects_fix-66e5292557c4
>
> Best regards,
Powered by blists - more mailing lists