Re: fs: use after free in /proc/pid/mountinfo

[Heiko Carstens]

[full quote of my mail below]

058504edd026 ("fs/seq_file: fallback to vmalloc allocation") is suspected to
cause a crash. Actually I can't reprocude the crash nor would I be able to
tell how the commit could cause the crash.
Anyway, I'll be offline for the next 2.5 weeks. So if Sasha could confirm
that reverting the patch actually does fix the crash, please revert the
commit, unless somebody else can make sense of the report of course.

I'm still wondering how Sasha could reproduce the crash.

Thanks,
Heiko

On Sun, Jul 06, 2014 at 12:04:20PM +0200, Heiko Carstens wrote:
> On Fri, Jul 04, 2014 at 10:55:13AM -0400, Sasha Levin wrote:
> > On 07/03/2014 05:37 PM, David Rientjes wrote:
> > > On Wed, 2 Jul 2014, Sasha Levin wrote:
> > > 
> > >>> Hi all,
> > >>>
> > >>> While fuzzing with trinity inside a KVM tools guest running the latest -next
> > >>> kernel I've stumbled on the following spew:
> > >>>
> > >>> [ 3569.869749] general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> > >>> [ 3569.869769] Dumping ftrace buffer:
> > >>> [ 3569.869879]    (ftrace buffer empty)
> > >>> [ 3569.869894] Modules linked in:
> > >>> [ 3569.869900] CPU: 7 PID: 10239 Comm: trinity-c86 Tainted: G        W      3.16.0-rc3-next-20140701-sasha-00023-g4eb2544-dirty #759
> > >>> [ 3569.869906] task: ffff88039e873000 ti: ffff880393f8c000 task.ti: ffff880393f8c000
> > >>> [ 3569.869932] RIP: show_mountinfo (fs/proc_namespace.c:127)
> 
> So that would be this line then:
> 
> static int show_mountinfo(struct seq_file *m, struct vfsmount *mnt)
> {
> 	struct proc_mounts *p = proc_mounts(m);
> 	struct mount *r = real_mount(mnt);
> 	struct super_block *sb = mnt->mnt_sb;
> 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt }; <---
> 
> if I understand the output correctly?
> 
> > >>> All code
> > >>> ========
> > >>>    0:	00 00                	add    %al,(%rax)
> > >>>    2:	00 00                	add    %al,(%rax)
> > >>>    4:	66 66 66 66 90       	data32 data32 data32 xchg %ax,%ax
> > >>>    9:	55                   	push   %rbp
> > >>>    a:	48 89 e5             	mov    %rsp,%rbp
> > >>>    d:	48 83 ec 50          	sub    $0x50,%rsp
> > >>>   11:	48 89 5d d8          	mov    %rbx,-0x28(%rbp)
> > >>>   15:	48 89 f3             	mov    %rsi,%rbx
> > >>>   18:	4c 89 65 e0          	mov    %r12,-0x20(%rbp)
> > >>>   1c:	49 89 fc             	mov    %rdi,%r12
> > >>>   1f:	4c 89 6d e8          	mov    %r13,-0x18(%rbp)
> > >>>   23:	4c 89 75 f0          	mov    %r14,-0x10(%rbp)
> > >>>   27:	4c 89 7d f8          	mov    %r15,-0x8(%rbp)
> > >>>   2b:*	48 8b 06             	mov    (%rsi),%rax		<-- trapping instruction
> > >>>   2e:	48 89 75 b0          	mov    %rsi,-0x50(%rbp)
> > >>>   32:	4c 8b 76 08          	mov    0x8(%rsi),%r14
> > >>>   36:	8b 96 ec 00 00 00    	mov    0xec(%rsi),%edx
> > >>>   3c:	48 89 45 b8          	mov    %rax,-0x48(%rbp)
> 
> Does that fit to this asm code? Sorry.. I'm not very familar with x86 asm.
> 
> > > Does this now reproduce on Linus's tree?  If so, does reverting commit
> > > 058504edd026 ("fs/seq_file: fallback to vmalloc allocation") prevent this 
> > > issue?
> > > 
> > > This is a use-after-free since the poison value is 0x6b and I'm presuming 
> > > that your /proc/self/mountinfo may be larger than PAGE_SIZE in your 
> > > testing environment.
> > 
> > Good call, reverting that patch made both issues go away.
> 
> Ok, does that mean you have a test case so you can reproduce the crashes?
> Just wondering, since trinity is usually quite random in what it does.
> 
> I tried to reproduce a crash on one of my systems by enforcing several
> seq_files to grow beyond PAGE_SIZE, but couldn't reproduce anything yet.
> 
> Could you try the patch below please? It basically reverts my patch and
> just leaves the kfree->kvfree conversion in. This is just a shot in the
> dark, since I can't make any sense of this ...yet :)
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3857b720cb1b..c1cf494cc238 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -34,12 +34,7 @@ static void seq_set_overflow(struct seq_file *m)
>  
>  static void *seq_buf_alloc(unsigned long size)
>  {
> -	void *buf;
> -
> -	buf = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
> -	if (!buf && size > PAGE_SIZE)
> -		buf = vmalloc(size);
> -	return buf;
> +	return kmalloc(size, GFP_KERNEL);
>  }
>  
>  /**
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/