Re: KASAN: use-after-free Write in page_counter_uncharge

Re: KASAN: use-after-free Write in page_counter_uncharge

[Michal Hocko]

On Tue 18-08-20 16:18:56, Andrew Morton wrote:
> On Tue, 18 Aug 2020 07:50:28 -0700 syzbot <syzbot+b305848212deec86eabe@syzkaller.appspotmail.com> wrote:
> 
> > Hello,
> > 
> > syzbot found the following issue on:
> > 
> > HEAD commit:    a1d21081 Merge git://git.kernel.org/pub/scm/linux/kernel/g..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=17ceb0ce900000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=21f0d1d2df6d5fc
> > dashboard link: https://syzkaller.appspot.com/bug?extid=b305848212deec86eabe
> > compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> 
> Presumably this is the same as
> http://lkml.kernel.org/r/00000000000011710f05ad27fe8a@google.com.

Very likely.

> > Unfortunately, I don't have any reproducer for this issue yet.
> > 
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+b305848212deec86eabe@syzkaller.appspotmail.com
> > 
> > ==================================================================
> > BUG: KASAN: use-after-free in instrument_atomic_write include/linux/instrumented.h:71 [inline]
> > BUG: KASAN: use-after-free in atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
> > BUG: KASAN: use-after-free in atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
> > BUG: KASAN: use-after-free in page_counter_cancel mm/page_counter.c:54 [inline]
> > BUG: KASAN: use-after-free in page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
> > Write of size 8 at addr ffff8880371c0148 by task syz-executor.0/9304
> > 
> > CPU: 0 PID: 9304 Comm: syz-executor.0 Not tainted 5.8.0-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > Call Trace:
> >  __dump_stack lib/dump_stack.c:77 [inline]
> >  dump_stack+0x1f0/0x31e lib/dump_stack.c:118
> >  print_address_description+0x66/0x620 mm/kasan/report.c:383
> >  __kasan_report mm/kasan/report.c:513 [inline]
> >  kasan_report+0x132/0x1d0 mm/kasan/report.c:530
> >  check_memory_region_inline mm/kasan/generic.c:183 [inline]
> >  check_memory_region+0x2b5/0x2f0 mm/kasan/generic.c:192
> >  instrument_atomic_write include/linux/instrumented.h:71 [inline]
> >  atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
> >  atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
> >  page_counter_cancel mm/page_counter.c:54 [inline]
> >  page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
> >  uncharge_batch+0x6c/0x350 mm/memcontrol.c:6764
> >  uncharge_page+0x115/0x430 mm/memcontrol.c:6796
> >  uncharge_list mm/memcontrol.c:6835 [inline]
> >  mem_cgroup_uncharge_list+0x70/0xe0 mm/memcontrol.c:6877
> >  release_pages+0x13a2/0x1550 mm/swap.c:911

This looks like a reference count unbalance when memcg is released
early. My first guess would be 1a3e1f40962c ("mm: memcontrol: decouple
reference counting from page accounting").

Unless I am missing something nothing really prevents the memcg for the
current batch to go away. uncharge_page collects all the charges for the
same memcg but it later drops the reference for the current page. Later
on when the memcg changes or when the final clean up is done in uncharge_list
uncharge_batch needs to access memcg but this might be after the last
page dropped the reference and memcg went away. The whole process of
tear down is quite complex and takes some time with all the RCU/WQ
involvement so this is quite unlikely to hit.

That being said the below should cure the reference count but I am not
sure this is a complete fix. If this looks reasonable I will post the
full patch. Johannes?

--- 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b807952b4d43..11b6dd1c4f64 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6753,6 +6753,7 @@ struct uncharge_gather {
 
 static inline void uncharge_gather_clear(struct uncharge_gather *ug)
 {
+	css_put(&ug->memcg->css);
 	memset(ug, 0, sizeof(*ug));
 }
 
@@ -6797,6 +6798,7 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
 			uncharge_gather_clear(ug);
 		}
 		ug->memcg = page->mem_cgroup;
+		css_get(&ug->memcg->css);
 	}
 
 	nr_pages = compound_nr(page);
-- 
Michal Hocko
SUSE Labs

Re: KASAN: use-after-free Write in page_counter_uncharge

[Michal Hocko]

On Wed 19-08-20 08:34:22, Michal Hocko wrote:
[...]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b807952b4d43..11b6dd1c4f64 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6753,6 +6753,7 @@ struct uncharge_gather {
>  
>  static inline void uncharge_gather_clear(struct uncharge_gather *ug)
>  {
> +	css_put(&ug->memcg->css);
>  	memset(ug, 0, sizeof(*ug));
>  }
>  
> @@ -6797,6 +6798,7 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
>  			uncharge_gather_clear(ug);
>  		}
>  		ug->memcg = page->mem_cgroup;
> +		css_get(&ug->memcg->css);
>  	}
>  
>  	nr_pages = compound_nr(page);

This is not a proper fix because uncharge_gather_clear is called also to
initialize the initial state so ug->memcg would be a garbage from the
stack. The proper fix with the full changelog should be. Let's add more
people involved in the original commit to the CC. The initial report is
http://lkml.kernel.org/r/00000000000014822b05ad2802a7@google.com resp.
http://lkml.kernel.org/r/00000000000011710f05ad27fe8a@google.com

From 73a40589cab12122170fb9f90222982e81d41423 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 20 Aug 2020 10:44:58 +0200
Subject: [PATCH] memcg: fix use-after-free in uncharge_batch

syzbot has reported an use-after-free in the uncharge_batch path
BUG: KASAN: use-after-free in instrument_atomic_write include/linux/instrumented.h:71 [inline]
BUG: KASAN: use-after-free in atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
BUG: KASAN: use-after-free in atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
BUG: KASAN: use-after-free in page_counter_cancel mm/page_counter.c:54 [inline]
BUG: KASAN: use-after-free in page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
Write of size 8 at addr ffff8880371c0148 by task syz-executor.0/9304

CPU: 0 PID: 9304 Comm: syz-executor.0 Not tainted 5.8.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x1f0/0x31e lib/dump_stack.c:118
 print_address_description+0x66/0x620 mm/kasan/report.c:383
 __kasan_report mm/kasan/report.c:513 [inline]
 kasan_report+0x132/0x1d0 mm/kasan/report.c:530
 check_memory_region_inline mm/kasan/generic.c:183 [inline]
 check_memory_region+0x2b5/0x2f0 mm/kasan/generic.c:192
 instrument_atomic_write include/linux/instrumented.h:71 [inline]
 atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
 atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
 page_counter_cancel mm/page_counter.c:54 [inline]
 page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
 uncharge_batch+0x6c/0x350 mm/memcontrol.c:6764
 uncharge_page+0x115/0x430 mm/memcontrol.c:6796
 uncharge_list mm/memcontrol.c:6835 [inline]
 mem_cgroup_uncharge_list+0x70/0xe0 mm/memcontrol.c:6877
 release_pages+0x13a2/0x1550 mm/swap.c:911
 tlb_batch_pages_flush mm/mmu_gather.c:49 [inline]
 tlb_flush_mmu_free mm/mmu_gather.c:242 [inline]
 tlb_flush_mmu+0x780/0x910 mm/mmu_gather.c:249
 tlb_finish_mmu+0xcb/0x200 mm/mmu_gather.c:328
 exit_mmap+0x296/0x550 mm/mmap.c:3185
 __mmput+0x113/0x370 kernel/fork.c:1076
 exit_mm+0x4cd/0x550 kernel/exit.c:483
 do_exit+0x576/0x1f20 kernel/exit.c:793
 do_group_exit+0x161/0x2d0 kernel/exit.c:903
 get_signal+0x139b/0x1d30 kernel/signal.c:2743
 arch_do_signal+0x33/0x610 arch/x86/kernel/signal.c:811
 exit_to_user_mode_loop kernel/entry/common.c:135 [inline]
 exit_to_user_mode_prepare+0x8d/0x1b0 kernel/entry/common.c:166
 syscall_exit_to_user_mode+0x5e/0x1a0 kernel/entry/common.c:241
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

1a3e1f40962c ("mm: memcontrol: decouple reference counting from page
accounting") has reworked the memcg lifetime to be bound the the struct
page rather than charges. It has also removed the css_put_many from
uncharge_batch and that is causing the above splat. uncharge_batch is
supposed to uncharge accumulated charges for all pages freed from the
same memcg. The queuing is done by uncharge_page which however drops the
memcg reference after it adds charges to the batch. If the current page
happens to be the last one holding the reference for its memcg then the
memcg is OK to go and the next page to be freed will trigger batched
uncharge which needs to access the memcg which is gone already.

Fix the issue by taking a reference for the memcg in the current batch.

Fixes: 1a3e1f40962c ("mm: memcontrol: decouple reference counting from page accounting")
Reported-by: syzbot+b305848212deec86eabe@syzkaller.appspotmail.com
Reported-by: syzbot+b5ea6fb6f139c8b9482b@syzkaller.appspotmail.com
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memcontrol.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b807952b4d43..cfa6cbad21d5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6774,6 +6774,9 @@ static void uncharge_batch(const struct uncharge_gather *ug)
 	__this_cpu_add(ug->memcg->vmstats_percpu->nr_page_events, ug->nr_pages);
 	memcg_check_events(ug->memcg, ug->dummy_page);
 	local_irq_restore(flags);
+
+	/* drop reference from uncharge_page */
+	css_put(&ug->memcg->css);
 }
 
 static void uncharge_page(struct page *page, struct uncharge_gather *ug)
@@ -6797,6 +6800,9 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
 			uncharge_gather_clear(ug);
 		}
 		ug->memcg = page->mem_cgroup;
+
+		/* pairs with css_put in uncharge_batch */
+		css_get(&ug->memcg->css);
 	}
 
 	nr_pages = compound_nr(page);
-- 
2.28.0

-- 
Michal Hocko
SUSE Labs

Re: KASAN: use-after-free Write in page_counter_uncharge

[Shakeel Butt]

On Thu, Aug 20, 2020 at 2:03 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 19-08-20 08:34:22, Michal Hocko wrote:
> [...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index b807952b4d43..11b6dd1c4f64 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -6753,6 +6753,7 @@ struct uncharge_gather {
> >
> >  static inline void uncharge_gather_clear(struct uncharge_gather *ug)
> >  {
> > +     css_put(&ug->memcg->css);
> >       memset(ug, 0, sizeof(*ug));
> >  }
> >
> > @@ -6797,6 +6798,7 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
> >                       uncharge_gather_clear(ug);
> >               }
> >               ug->memcg = page->mem_cgroup;
> > +             css_get(&ug->memcg->css);
> >       }
> >
> >       nr_pages = compound_nr(page);
>
> This is not a proper fix because uncharge_gather_clear is called also to
> initialize the initial state so ug->memcg would be a garbage from the
> stack. The proper fix with the full changelog should be. Let's add more
> people involved in the original commit to the CC. The initial report is
> http://lkml.kernel.org/r/00000000000014822b05ad2802a7@google.com resp.
> http://lkml.kernel.org/r/00000000000011710f05ad27fe8a@google.com
>
> From 73a40589cab12122170fb9f90222982e81d41423 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 20 Aug 2020 10:44:58 +0200
> Subject: [PATCH] memcg: fix use-after-free in uncharge_batch
>
> syzbot has reported an use-after-free in the uncharge_batch path
> BUG: KASAN: use-after-free in instrument_atomic_write include/linux/instrumented.h:71 [inline]
> BUG: KASAN: use-after-free in atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
> BUG: KASAN: use-after-free in atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
> BUG: KASAN: use-after-free in page_counter_cancel mm/page_counter.c:54 [inline]
> BUG: KASAN: use-after-free in page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
> Write of size 8 at addr ffff8880371c0148 by task syz-executor.0/9304
>
> CPU: 0 PID: 9304 Comm: syz-executor.0 Not tainted 5.8.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1f0/0x31e lib/dump_stack.c:118
>  print_address_description+0x66/0x620 mm/kasan/report.c:383
>  __kasan_report mm/kasan/report.c:513 [inline]
>  kasan_report+0x132/0x1d0 mm/kasan/report.c:530
>  check_memory_region_inline mm/kasan/generic.c:183 [inline]
>  check_memory_region+0x2b5/0x2f0 mm/kasan/generic.c:192
>  instrument_atomic_write include/linux/instrumented.h:71 [inline]
>  atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
>  atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
>  page_counter_cancel mm/page_counter.c:54 [inline]
>  page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
>  uncharge_batch+0x6c/0x350 mm/memcontrol.c:6764
>  uncharge_page+0x115/0x430 mm/memcontrol.c:6796
>  uncharge_list mm/memcontrol.c:6835 [inline]
>  mem_cgroup_uncharge_list+0x70/0xe0 mm/memcontrol.c:6877
>  release_pages+0x13a2/0x1550 mm/swap.c:911
>  tlb_batch_pages_flush mm/mmu_gather.c:49 [inline]
>  tlb_flush_mmu_free mm/mmu_gather.c:242 [inline]
>  tlb_flush_mmu+0x780/0x910 mm/mmu_gather.c:249
>  tlb_finish_mmu+0xcb/0x200 mm/mmu_gather.c:328
>  exit_mmap+0x296/0x550 mm/mmap.c:3185
>  __mmput+0x113/0x370 kernel/fork.c:1076
>  exit_mm+0x4cd/0x550 kernel/exit.c:483
>  do_exit+0x576/0x1f20 kernel/exit.c:793
>  do_group_exit+0x161/0x2d0 kernel/exit.c:903
>  get_signal+0x139b/0x1d30 kernel/signal.c:2743
>  arch_do_signal+0x33/0x610 arch/x86/kernel/signal.c:811
>  exit_to_user_mode_loop kernel/entry/common.c:135 [inline]
>  exit_to_user_mode_prepare+0x8d/0x1b0 kernel/entry/common.c:166
>  syscall_exit_to_user_mode+0x5e/0x1a0 kernel/entry/common.c:241
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> 1a3e1f40962c ("mm: memcontrol: decouple reference counting from page
> accounting") has reworked the memcg lifetime to be bound the the struct
> page rather than charges. It has also removed the css_put_many from
> uncharge_batch and that is causing the above splat. uncharge_batch is
> supposed to uncharge accumulated charges for all pages freed from the
> same memcg. The queuing is done by uncharge_page which however drops the
> memcg reference after it adds charges to the batch. If the current page
> happens to be the last one holding the reference for its memcg then the
> memcg is OK to go and the next page to be freed will trigger batched
> uncharge which needs to access the memcg which is gone already.
>
> Fix the issue by taking a reference for the memcg in the current batch.
>
> Fixes: 1a3e1f40962c ("mm: memcontrol: decouple reference counting from page accounting")
> Reported-by: syzbot+b305848212deec86eabe@syzkaller.appspotmail.com
> Reported-by: syzbot+b5ea6fb6f139c8b9482b@syzkaller.appspotmail.com
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Seems correct to me.

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: KASAN: use-after-free Write in page_counter_uncharge

[Johannes Weiner]

On Thu, Aug 20, 2020 at 11:03:41AM +0200, Michal Hocko wrote:
> From 73a40589cab12122170fb9f90222982e81d41423 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 20 Aug 2020 10:44:58 +0200
> Subject: [PATCH] memcg: fix use-after-free in uncharge_batch
> 
> syzbot has reported an use-after-free in the uncharge_batch path
> BUG: KASAN: use-after-free in instrument_atomic_write include/linux/instrumented.h:71 [inline]
> BUG: KASAN: use-after-free in atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
> BUG: KASAN: use-after-free in atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
> BUG: KASAN: use-after-free in page_counter_cancel mm/page_counter.c:54 [inline]
> BUG: KASAN: use-after-free in page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
> Write of size 8 at addr ffff8880371c0148 by task syz-executor.0/9304
> 
> CPU: 0 PID: 9304 Comm: syz-executor.0 Not tainted 5.8.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1f0/0x31e lib/dump_stack.c:118
>  print_address_description+0x66/0x620 mm/kasan/report.c:383
>  __kasan_report mm/kasan/report.c:513 [inline]
>  kasan_report+0x132/0x1d0 mm/kasan/report.c:530
>  check_memory_region_inline mm/kasan/generic.c:183 [inline]
>  check_memory_region+0x2b5/0x2f0 mm/kasan/generic.c:192
>  instrument_atomic_write include/linux/instrumented.h:71 [inline]
>  atomic64_sub_return include/asm-generic/atomic-instrumented.h:970 [inline]
>  atomic_long_sub_return include/asm-generic/atomic-long.h:113 [inline]
>  page_counter_cancel mm/page_counter.c:54 [inline]
>  page_counter_uncharge+0x3d/0xc0 mm/page_counter.c:155
>  uncharge_batch+0x6c/0x350 mm/memcontrol.c:6764
>  uncharge_page+0x115/0x430 mm/memcontrol.c:6796
>  uncharge_list mm/memcontrol.c:6835 [inline]
>  mem_cgroup_uncharge_list+0x70/0xe0 mm/memcontrol.c:6877
>  release_pages+0x13a2/0x1550 mm/swap.c:911
>  tlb_batch_pages_flush mm/mmu_gather.c:49 [inline]
>  tlb_flush_mmu_free mm/mmu_gather.c:242 [inline]
>  tlb_flush_mmu+0x780/0x910 mm/mmu_gather.c:249
>  tlb_finish_mmu+0xcb/0x200 mm/mmu_gather.c:328
>  exit_mmap+0x296/0x550 mm/mmap.c:3185
>  __mmput+0x113/0x370 kernel/fork.c:1076
>  exit_mm+0x4cd/0x550 kernel/exit.c:483
>  do_exit+0x576/0x1f20 kernel/exit.c:793
>  do_group_exit+0x161/0x2d0 kernel/exit.c:903
>  get_signal+0x139b/0x1d30 kernel/signal.c:2743
>  arch_do_signal+0x33/0x610 arch/x86/kernel/signal.c:811
>  exit_to_user_mode_loop kernel/entry/common.c:135 [inline]
>  exit_to_user_mode_prepare+0x8d/0x1b0 kernel/entry/common.c:166
>  syscall_exit_to_user_mode+0x5e/0x1a0 kernel/entry/common.c:241
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> 1a3e1f40962c ("mm: memcontrol: decouple reference counting from page
> accounting") has reworked the memcg lifetime to be bound the the struct
> page rather than charges. It has also removed the css_put_many from
> uncharge_batch and that is causing the above splat. uncharge_batch is
> supposed to uncharge accumulated charges for all pages freed from the
> same memcg. The queuing is done by uncharge_page which however drops the
> memcg reference after it adds charges to the batch. If the current page
> happens to be the last one holding the reference for its memcg then the
> memcg is OK to go and the next page to be freed will trigger batched
> uncharge which needs to access the memcg which is gone already.
> 
> Fix the issue by taking a reference for the memcg in the current batch.
> 
> Fixes: 1a3e1f40962c ("mm: memcontrol: decouple reference counting from page accounting")
> Reported-by: syzbot+b305848212deec86eabe@syzkaller.appspotmail.com
> Reported-by: syzbot+b5ea6fb6f139c8b9482b@syzkaller.appspotmail.com
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Nice catch! The fix looks correct - ug now holds a reference count for
its ug->memcg pointer.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>