linux-kernel - Re: [PATCH] vfs:Fix memory leak on error path in get_empty

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-next>] [day] [month] [year] [list]

Message-ID: <20160722040156.GM2356@ZenIV.linux.org.uk>
Date:	Fri, 22 Jul 2016 05:01:56 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Nicholas Krause <xerofoify@...il.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file

On Thu, Jul 21, 2016 at 10:53:37PM -0400, Nicholas Krause wrote:
> This fixes a memory leak on the error path if the call to
> security_file_alloc fails to run successfully as detected
> in this trace by kmemleak:
> [  321.783718] ath9k 0000:03:00.0 eth0: renamed from wlan0
> [  330.960024] atl1c 0000:02:00.0 eth1: renamed from eth126
> [  391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized memory (ffff8800a8ad8a00)
> [  392.678675] 00acada80088fffffeedcafe2800000028000000880000008afa90c800000000
> [  393.568962]  u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
> [  394.461350]  ^
> [  395.305638] RIP: 0010:[<ffffffff811936d0>]  [<ffffffff811936d0>] kmem_cache_alloc+0x70/0x120
> [  396.180025] RSP: 0018:ffff8800a88ebd10  EFLAGS: 00010246
> [  397.037327] RAX: ffff8800a8ad8a00 RBX: 0000000000000000 RCX: 00000000001d90e0
> [  397.889379] RDX: 0000000000057898 RSI: 0000000000057898 RDI: 00000000001d90e0
> [  398.735330] RBP: ffff8800a88ebd38 R08: ffffffffffffffff R09: fffffffffffff580
> [  399.580699] R10: 0000000000000001 R11: ffff8801c294b000 R12: ffff8800a8ad8a00
> [  400.426021] R13: ffffffff8119e308 R14: ffff8801c7003600 R15: 00000000024080c0
> [  401.271494] FS:  00007f6073fc3780(0000) GS:ffff8801c7400000(0000) knlGS:0000000000000000
> [  402.117062] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  402.955591] CR2: ffff8801c7060c90 CR3: 00000000a88f1000 CR4: 00000000000406f0
> [  403.807725]  [<ffffffff8119e308>] get_empty_filp+0x58/0x1b0
> [  404.661980]  [<ffffffff811aa916>] path_openat+0x26/0x9a0
> [  405.514128]  [<ffffffff811ac3a9>] do_filp_open+0x79/0xd0
> [  406.358987]  [<ffffffff8119afd2>] do_sys_open+0x122/0x1f0
> [  407.194074]  [<ffffffff8119b0b9>] SyS_open+0x19/0x20
> [  408.017053]  [<ffffffff819434a5>] entry_SYSCALL_64_fastpath+0x18/0xa8
> [  408.844745]  [<ffffffffffffffff>] 0xffffffffffffffff
> [  417.533148] Adding 1048572k swap on /dev/sda1.  Priority:-1 extents:1 across:1048572k SS
> Fix this memory leak by freeing the previously allocated file
> structure pointer allocated with kmem_cache_alloc from the
> filp_cache by calling kmem_cache_free on this particular
> error path in get_empty_leak in order to avoid leaking
> the memory allocated to this structure pointer.
> 
> Signed-off-by: Nicholas Krause <xerofoify@...il.com>
> ---
>  fs/file_table.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index ad17e05..92eb307 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -128,6 +128,7 @@ struct file *get_empty_filp(void)
>  	error = security_file_alloc(f);
>  	if (unlikely(error)) {
>  		file_free(f);
> +		kmem_cache_free(filp_cache, f);
>  		return ERR_PTR(error);
>  	}

Bloody wonderful.
	a) nothing in quoted trace mentions a leak of any sort.
	b) you have *not* tested your "fix".  At all.
	c) your patch introduces a bug while fixing nothing.  What do you
think a function called "file_free" might be doing?  I realize that reading
the damn thing (all two lines worth of it) is beneath your dignity, but does
its name suggest anything to you?

*plonk*