linux-kernel - Re: [PATCH v2] mm: fix null-ptr-deref in kswapd_is

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]

Date:   Sat, 27 Aug 2022 15:08:55 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Kefeng Wang <wangkefeng.wang@...wei.com>
Cc:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        David Hildenbrand <david@...hat.com>,
        Muchun Song <muchun.song@...ux.dev>
Subject: Re: [PATCH v2] mm: fix null-ptr-deref in kswapd_is_running()

On Sat, 27 Aug 2022 19:19:59 +0800 Kefeng Wang <wangkefeng.wang@...wei.com> wrote:

> The kswapd_run/stop() will set pgdat->kswapd to NULL, which
> could race with kswapd_is_running() in kcompactd(),
> 
> kswapd_run/stop()                       kcompactd()
>                                           kswapd_is_running()
>   pgdat->kswapd // error or nomal ptr
>                                           verify pgdat->kswapd
>                                             // load non-NULL
> pgdat->kswapd
>   pgdat->kswapd = NULL
>                                           task_is_running(pgdat->kswapd)
>                                             // Null pointer derefence
> 
> The KASAN report the null-ptr-deref shown below,
> 
>   vmscan: Failed to start kswapd on node 0
>   ...
>   BUG: KASAN: null-ptr-deref in kcompactd+0x440/0x504
>   Read of size 8 at addr 0000000000000024 by task kcompactd0/37
> 
>   CPU: 0 PID: 37 Comm: kcompactd0 Kdump: loaded Tainted: G           OE     5.10.60 #1
>   Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
>   Call trace:
>    dump_backtrace+0x0/0x394
>    show_stack+0x34/0x4c
>    dump_stack+0x158/0x1e4
>    __kasan_report+0x138/0x140
>    kasan_report+0x44/0xdc
>    __asan_load8+0x94/0xd0
>    kcompactd+0x440/0x504
>    kthread+0x1a4/0x1f0
>    ret_from_fork+0x10/0x18
> 
> For now, kswapd/kcompactd_run() and kswapd/kcompactd_stop() protected
> by mem_hotplug_begin/done(), but without kcompactd(). It is no need to
> involve memory hotplug lock in kcompactd(), so let's add new mutex to
> protect pgdat->kswapd accessed concurrently, also because kcompactd task
> will check the state of kswapd task, it's better to call kcompactd_stop()
> before kswapd_stop() to reduce lock conflicts.
> 

Looks right to me.

I think the below will make the code a little more maintainable?

--- a/include/linux/memory_hotplug.h~mm-fix-null-ptr-deref-in-kswapd_is_running-fix
+++ a/include/linux/memory_hotplug.h
@@ -215,6 +215,7 @@ void put_online_mems(void);
 void mem_hotplug_begin(void);
 void mem_hotplug_done(void);
 
+/* See kswapd_is_running() */
 static inline void pgdat_kswapd_lock(pg_data_t *pgdat)
 {
 	mutex_lock(&pgdat->kswapd_lock);
--- a/mm/compaction.c~mm-fix-null-ptr-deref-in-kswapd_is_running-fix
+++ a/mm/compaction.c
@@ -1980,6 +1980,12 @@ static inline bool is_via_compact_memory
 	return order == -1;
 }
 
+/*
+ * Determine whether kswapd is (or recently was!) running on this node.
+ *
+ * pgdat_kswapd_lock() pins pgdat->kswapd, so a concurrent kswapd_stop() can't
+ * zero it.
+ */
 static bool kswapd_is_running(pg_data_t *pgdat)
 {
 	bool running;
_