[PATCH] mm: ksm: fix data-race in __ksm_enter / run_store

[PATCH] mm: ksm: fix data-race in __ksm_enter / run_store

[Kefeng Wang]

Abhishek reported a data-race issue,

BUG: KCSAN: data-race in __ksm_enter / run_store
write to 0xffffffff881edae0 of 8 bytes by task 6542 on cpu 0:
 run_store+0x19a/0x2d0 mm/ksm.c:2897
 kobj_attr_store+0x44/0x60 lib/kobject.c:824
 sysfs_kf_write+0x16f/0x1a0 fs/sysfs/file.c:136
 kernfs_fop_write_iter+0x2ae/0x370 fs/kernfs/file.c:291
 call_write_iter include/linux/fs.h:2050 [inline]
 new_sync_write fs/read_write.c:504 [inline]
 vfs_write+0x779/0x900 fs/read_write.c:591
 ksys_write+0xde/0x190 fs/read_write.c:644
 __do_sys_write fs/read_write.c:656 [inline]
 __se_sys_write fs/read_write.c:653 [inline]
 __x64_sys_write+0x43/0x50 fs/read_write.c:653
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0x90 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

read to 0xffffffff881edae0 of 8 bytes by task 6541 on cpu 1:
 __ksm_enter+0x114/0x260 mm/ksm.c:2501
 ksm_madvise+0x291/0x350 mm/ksm.c:2451
 madvise_vma_behavior mm/madvise.c:1039 [inline]
 madvise_walk_vmas mm/madvise.c:1221 [inline]
 do_madvise+0x656/0xeb0 mm/madvise.c:1399
 __do_sys_madvise mm/madvise.c:1412 [inline]
 __se_sys_madvise mm/madvise.c:1410 [inline]
 __x64_sys_madvise+0x64/0x70 mm/madvise.c:1410
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0x90 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 6541 Comm: syz-executor2-n Not tainted 5.18.0-rc5+ #107
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014

The ksm_run is alread protected by ksm_thread_mutex in run_store, we
could add this lock in __ksm_enter() to avoid the above issue.

Reported-and-tested-by: Abhishek Shah <abhishek.shah@columbia.edu>
Cc: Gabriel Ryan <gabe@cs.columbia.edu>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/ksm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 2f315c69fa2c..3f1908946a6f 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2507,6 +2507,7 @@ int __ksm_enter(struct mm_struct *mm)
 {
 	struct mm_slot *mm_slot;
 	int needs_wakeup;
+	bool ksm_run_unmerge;
 
 	mm_slot = alloc_mm_slot();
 	if (!mm_slot)
@@ -2515,6 +2516,10 @@ int __ksm_enter(struct mm_struct *mm)
 	/* Check ksm_run too?  Would need tighter locking */
 	needs_wakeup = list_empty(&ksm_mm_head.mm_list);
 
+	mutex_lock(&ksm_thread_mutex);
+	ksm_run_unmerge = !!(ksm_run & KSM_RUN_UNMERGE);
+	mutex_unlock(&ksm_thread_mutex);
+
 	spin_lock(&ksm_mmlist_lock);
 	insert_to_mm_slots_hash(mm, mm_slot);
 	/*
@@ -2527,7 +2532,7 @@ int __ksm_enter(struct mm_struct *mm)
 	 * scanning cursor, otherwise KSM pages in newly forked mms will be
 	 * missed: then we might as well insert at the end of the list.
 	 */
-	if (ksm_run & KSM_RUN_UNMERGE)
+	if (ksm_run_unmerge)
 		list_add_tail(&mm_slot->mm_list, &ksm_mm_head.mm_list);
 	else
 		list_add_tail(&mm_slot->mm_list, &ksm_scan.mm_slot->mm_list);
-- 
2.35.3

Re: [PATCH] mm: ksm: fix data-race in __ksm_enter / run_store

[Matthew Wilcox]

On Tue, Aug 02, 2022 at 11:15:50PM +0800, Kefeng Wang wrote:
> The ksm_run is alread protected by ksm_thread_mutex in run_store, we
> could add this lock in __ksm_enter() to avoid the above issue.

I don't think this is a great fix.  Why not protect the store with
ksm_mmlist_lock?  ie:

        mutex_lock(&ksm_thread_mutex);
        wait_while_offlining();
        if (ksm_run != flags) {
+		spin_lock(&ksm_mmlist_lock);
                ksm_run = flags;
+		spin_unlock(&ksm_mmlist_lock);
                if (flags & KSM_RUN_UNMERGE) {
                        set_current_oom_origin();
                        err = unmerge_and_remove_all_rmap_items();
                        clear_current_oom_origin();
                        if (err) {
+				spin_lock(&ksm_mmlist_lock);
				ksm_run = KSM_RUN_STOP;
+				spin_unlock(&ksm_mmlist_lock);
...

(I also don't think this is a real bug, because the call to
unmerge_and_remove_all_rmap_items() will "cure" the misplacement of
items in the list, but there's value in shutting up the tools, I suppose)

Re: [PATCH] mm: ksm: fix data-race in __ksm_enter / run_store

[Gabriel Ryan]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

Hi Matthew,

I don't believe execution of unmerge_and_remove_all_rmap_items() after an
mm is misplaced is guaranteed.

Consider the following interleaving:
Thread A executes *__ksm_enter* with KSM_RUN_MERGE set through the check on
https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2501
Thread B executes *run_store* and sets KSM_RUN_UNMERGE and then also
executes unmerge_and_remove_all_rmap_items() on
https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2900
Thread A completes *__ksm_enter *and misplaces the mm behind the scanning
cursor since it is still on the KSM_RUN_MERGE path on
https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2504

I also noticed through manual inspection another check that appears racy of
the KSM_RUN_UNMERGE flag on
https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2563

Best,

Gabe



On Tue, Aug 2, 2022 at 11:45 AM Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Aug 02, 2022 at 11:15:50PM +0800, Kefeng Wang wrote:
> > The ksm_run is alread protected by ksm_thread_mutex in run_store, we
> > could add this lock in __ksm_enter() to avoid the above issue.
>
> I don't think this is a great fix.  Why not protect the store with
> ksm_mmlist_lock?  ie:
>
>         mutex_lock(&ksm_thread_mutex);
>         wait_while_offlining();
>         if (ksm_run != flags) {
> +               spin_lock(&ksm_mmlist_lock);
>                 ksm_run = flags;
> +               spin_unlock(&ksm_mmlist_lock);
>                 if (flags & KSM_RUN_UNMERGE) {
>                         set_current_oom_origin();
>                         err = unmerge_and_remove_all_rmap_items();
>                         clear_current_oom_origin();
>                         if (err) {
> +                               spin_lock(&ksm_mmlist_lock);
>                                 ksm_run = KSM_RUN_STOP;
> +                               spin_unlock(&ksm_mmlist_lock);
> ...
>
> (I also don't think this is a real bug, because the call to
> unmerge_and_remove_all_rmap_items() will "cure" the misplacement of
> items in the list, but there's value in shutting up the tools, I suppose)
>

[-- Attachment #2: Type: text/html, Size: 3228 bytes --]

Re: [PATCH] mm: ksm: fix data-race in __ksm_enter / run_store

[Andrew Morton]

On Tue, 2 Aug 2022 23:15:50 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Abhishek reported a data-race issue,

OK, but it would be better to perform an analysis of the alleged bug,
describe the potential effects if the race is hit, etc.

> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2507,6 +2507,7 @@ int __ksm_enter(struct mm_struct *mm)
>  {
>  	struct mm_slot *mm_slot;
>  	int needs_wakeup;
> +	bool ksm_run_unmerge;
>  
>  	mm_slot = alloc_mm_slot();
>  	if (!mm_slot)
> @@ -2515,6 +2516,10 @@ int __ksm_enter(struct mm_struct *mm)
>  	/* Check ksm_run too?  Would need tighter locking */
>  	needs_wakeup = list_empty(&ksm_mm_head.mm_list);
>  
> +	mutex_lock(&ksm_thread_mutex);
> +	ksm_run_unmerge = !!(ksm_run & KSM_RUN_UNMERGE);
> +	mutex_unlock(&ksm_thread_mutex);
>
>  	spin_lock(&ksm_mmlist_lock);
>  	insert_to_mm_slots_hash(mm, mm_slot);
>  	/*
> @@ -2527,7 +2532,7 @@ int __ksm_enter(struct mm_struct *mm)
>  	 * scanning cursor, otherwise KSM pages in newly forked mms will be
>  	 * missed: then we might as well insert at the end of the list.
>  	 */
> -	if (ksm_run & KSM_RUN_UNMERGE)
> +	if (ksm_run_unmerge)

run_store() can alter ksm_run right here, so __ksm_enter() is still
acting on the old setting?

>  		list_add_tail(&mm_slot->mm_list, &ksm_mm_head.mm_list);
>  	else
>  		list_add_tail(&mm_slot->mm_list, &ksm_scan.mm_slot->mm_list);

Re: [PATCH] mm: ksm: fix data-race in __ksm_enter / run_store

[Abhishek Shah]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]

Hi all,

 I looked through the vulnerability some more and came up with this so far.
Please let me know what you think

Consider the following interleaving.

   1. In *__ksm_enter*, thread 1 inserts the new mm into the list of mms at
   a position determined by *ksm_run* being set to KSM_RUN_MERGE (see here
   <https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2504>)
   2. In *run_store*, thread 2 changes *ksm_run* to KSM_RUN_UNMERGE here
   <https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2897> and
   executes unmerge_and_remove_all_rmap_items, where it can free the newly
   added mm via *mmdrop* here
   <https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L1004>.
   3. In *__ksm_enter*, thread 2 continues execution and updates the fields
   of the new mm (see here
   <https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2507>),
   although it was freed, resulting in a use-after-free vulnerability.

Thanks!

On Thu, Aug 11, 2022 at 7:00 PM Andrew Morton <akpm@linux-foundation.org>
wrote:

> On Tue, 2 Aug 2022 23:15:50 +0800 Kefeng Wang <wangkefeng.wang@huawei.com>
> wrote:
>
> > Abhishek reported a data-race issue,
>
> OK, but it would be better to perform an analysis of the alleged bug,
> describe the potential effects if the race is hit, etc.
>
> > --- a/mm/ksm.c
> > +++ b/mm/ksm.c
> > @@ -2507,6 +2507,7 @@ int __ksm_enter(struct mm_struct *mm)
> >  {
> >       struct mm_slot *mm_slot;
> >       int needs_wakeup;
> > +     bool ksm_run_unmerge;
> >
> >       mm_slot = alloc_mm_slot();
> >       if (!mm_slot)
> > @@ -2515,6 +2516,10 @@ int __ksm_enter(struct mm_struct *mm)
> >       /* Check ksm_run too?  Would need tighter locking */
> >       needs_wakeup = list_empty(&ksm_mm_head.mm_list);
> >
> > +     mutex_lock(&ksm_thread_mutex);
> > +     ksm_run_unmerge = !!(ksm_run & KSM_RUN_UNMERGE);
> > +     mutex_unlock(&ksm_thread_mutex);
> >
> >       spin_lock(&ksm_mmlist_lock);
> >       insert_to_mm_slots_hash(mm, mm_slot);
> >       /*
> > @@ -2527,7 +2532,7 @@ int __ksm_enter(struct mm_struct *mm)
> >        * scanning cursor, otherwise KSM pages in newly forked mms will be
> >        * missed: then we might as well insert at the end of the list.
> >        */
> > -     if (ksm_run & KSM_RUN_UNMERGE)
> > +     if (ksm_run_unmerge)
>
> run_store() can alter ksm_run right here, so __ksm_enter() is still
> acting on the old setting?
>
> >               list_add_tail(&mm_slot->mm_list, &ksm_mm_head.mm_list);
> >       else
> >               list_add_tail(&mm_slot->mm_list,
> &ksm_scan.mm_slot->mm_list);
>
>

[-- Attachment #2: Type: text/html, Size: 4303 bytes --]