On 2022/7/21 23:58, Abhishek Shah wrote:
> Dear Kernel Maintainers,
>
> We found a race in mm/ksm.c. During the execution of the function 
> /__ksm_run/ which uses variable /ksm_run/ to decide the list insertion 
> point, the variable /ksm_run/ can be concurrently modified in the 
> function /run_store/, which we thought could be undesirable since “KSM 
> pages in newly forked mms can be missed” (See comment here: 
> https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2498 
> <https://elixir.bootlin.com/linux/v5.18-rc5/source/mm/ksm.c#L2498>). 
> We would also like your thoughts on the security impact given it is a 
> TOCTOU bug.
>
> We provide more details below including the trace and reproducing 
> test cases.


Hello， could the following changes to avoid the data-race issue？

diff --git a/mm/ksm.c b/mm/ksm.c
index 54f78c9eecae..f072753cbb3a 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2497,6 +2497,7 @@ int __ksm_enter(struct mm_struct *mm)
  {
         struct mm_slot *mm_slot;
         int needs_wakeup;
+       bool ksm_run_merge;

         mm_slot = alloc_mm_slot();
         if (!mm_slot)
@@ -2505,6 +2506,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);
         /*
@@ -2517,7 +2522,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);


>
>
> *Trace*
> 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
> ---------------------
> *Inputs *
> Input CPU 0:
> r0 = openat$sysctl(0xffffff9c, 
> &(0x7f0000000100)='/sys/kernel/mm/ksm/run\x00', 0x1, 0x0)
> write$sysctl(r0, &(0x7f0000000000)='2\x00', 0x2)
>
> Input CPU 1:
> madvise(&(0x7f0000ffc000/0x4000)=nil, 0x4000, 0xc)
> mlock2(&(0x7f0000ffe000/0x2000)=nil, 0x2000, 0x0)
> madvise(&(0x7f0000ffd000/0x3000)=nil, 0x3000, 0x12)
> clone(0x0, 0x0, 0x0, 0x0, 0x0)