linux-kernel - Re: [PATCH] debugobject: Prevent init race with static objects

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZE/BQ5yUQZUwhlTu@shredder>
Date:   Mon, 1 May 2023 16:40:19 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Schspa Shi <schspa@...il.com>, longman@...hat.com,
        swboyd@...omium.org, linux@...ck-us.net, wuchi.zero@...il.com,
        linux-kernel@...r.kernel.org,
        syzbot+5093ba19745994288b53@...kaller.appspotmail.com,
        danieller@...dia.com, petrm@...dia.com
Subject: Re: [PATCH] debugobject: Prevent init race with static objects

On Wed, Apr 12, 2023 at 09:54:39AM +0200, Thomas Gleixner wrote:
> Statically initialized objects are usually not initialized via the init()
> function of the subsystem. They are special cased and the subsystem
> provides a function to validate whether an object which is not yet tracked
> by debugobjects is statically initialized. This means the object is started
> to be tracked on first use, e.g. activation.
> 
> This works perfectly fine, unless there are two concurrent operations on
> that object. Schspa decoded the problem:

[...]

> This race exists forever, but was never observed until mod_timer() got a
> debug_object_assert_init() added which is outside of the timer base lock
> held section right at the beginning of the function to cover the lockless
> early exit points too.
> 
> Rework the code so that the lookup, the static object check and the
> tracking object association happens atomically under the hash bucket
> lock. This prevents the issue completely as all callers are serialized on
> the hash bucket lock and therefore cannot observe inconsistent state.
> 
> Fixes: 3ac7fe5a4aab ("infrastructure to debug (dynamic) objects")
> Reported-by: syzbot+5093ba19745994288b53@...kaller.appspotmail.com
> Debugged-by: Schspa Shi <schspa@...il.com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
> Link: https://lore.kernel.org/lkml/20230303161906.831686-1-schspa@gmail.com

Thomas,

With this patch we see the following warning in the kernel log during
boot:

"ODEBUG: Out of memory. ODEBUG disabled"

In which case, the stats are:

# cat /sys/kernel/debug/debug_objects/stats
max_chain     :24
max_checked   :37
warnings      :0
fixups        :0
pool_free     :4297
pool_pcp_free :84
pool_min_free :0
pool_used     :0
pool_max_used :6615
on_free_list  :0
objs_allocated:15616
objs_freed    :11319


If I revert this patch, the warning disappears and I see the following
stats:

# cat /sys/kernel/debug/debug_objects/stats
max_chain     :25
max_checked   :40
warnings      :0
fixups        :0
pool_free     :1219
pool_pcp_free :209
pool_min_free :289
pool_used     :1578
pool_max_used :8026
on_free_list  :0
objs_allocated:32304
objs_freed    :29507

The following diff seems to solve the problem for me:

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index b796799fadb2..af4bd66c571c 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -21,7 +21,7 @@
 #define ODEBUG_HASH_BITS       14
 #define ODEBUG_HASH_SIZE       (1 << ODEBUG_HASH_BITS)
 
-#define ODEBUG_POOL_SIZE       1024
+#define ODEBUG_POOL_SIZE       (16 * 1024)
 #define ODEBUG_POOL_MIN_LEVEL  256
 #define ODEBUG_POOL_PERCPU_SIZE        64
 #define ODEBUG_BATCH_SIZE      16

In which case, the stats are:

# cat /sys/kernel/debug/debug_objects/stats 
max_chain     :28
max_checked   :64
warnings      :0
fixups        :0
pool_free     :14789
pool_pcp_free :192
pool_min_free :8120
pool_used     :1595
pool_max_used :8264
on_free_list  :0
objs_allocated:16384
objs_freed    :0

I'm not familiar with the debugobjects code, but maybe it makes sense to
make "ODEBUG_POOL_SIZE" configurable via Kconfig in a similar fashion to
"CONFIG_DEBUG_KMEMLEAK_MEM_POOL_SIZE"?

Please let me know if more information is required or if you want me to
test a patch.

Thanks