linux-kernel - Re: [PATCH] vfs:Fix kmemleak in get_empty_filp on error path if security_file

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

Message-ID: <20160717033203.GL14480@ZenIV.linux.org.uk>
Date:	Sun, 17 Jul 2016 04:32:13 +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 kmemleak in get_empty_filp on error path if
 security_file_alloc fails

On Sat, Jul 16, 2016 at 11:00:03PM -0400, Nicholas Krause wrote:
> This fixes the following kmemleak memory report spat:
> [  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
> This is easily fixed by moving the call to setting the file structure
> pointer's file count to 1 before the error check if security_file_alloc
> fails with atomic_long_set before this call in order to avoid the
> above spat. In addition this is required in order to avoid us
> freeing a file structure pointer with no reference i.e. has zero
> users in file_free on this zero path in order to avoid the kmemleak
> spat complaining about reading from uninitiliazied memory.
> 
> Signed-off-by: Nicholas Krause <xerofoify@...il.com>
> ---
>  fs/file_table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index ad17e05..cbc6c37 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -125,13 +125,13 @@ struct file *get_empty_filp(void)
>  
>  	percpu_counter_inc(&nr_files);
>  	f->f_cred = get_cred(cred);
> +	atomic_long_set(&f->f_count, 1);
>  	error = security_file_alloc(f);
>  	if (unlikely(error)) {
>  		file_free(f);
>  		return ERR_PTR(error);
>  	}
>  
> -	atomic_long_set(&f->f_count, 1);
>  	rwlock_init(&f->f_owner.lock);
>  	spin_lock_init(&f->f_lock);
>  	mutex_init(&f->f_pos_lock);

NAK.  Analysis is complete garbage, and so's the patch; to start with, f
comes from kmem_cache_zalloc(), which *does* initialize the entire object
returned.  Moreover, neither file_free() nor security_file_alloc() are
accessing ->f_count anyway, so moving that assignment up doesn't change
anything whatsoever.  So if this changes behaviour, your reproducer is
unreliable.  OR, considering the very special origin of that patch, it hadn't
been tested at all.

For those who are not familiar with Nick - this is really a very special case,
with long history of posting utterly BS patches, nodding politely when people
explain what is wrong and proceeding to spew the same kind of garbage again
and again.  Not only clue-resistant, but has exhausted the patience even of
normally quite polite people (which I do not pretend to be)

Nick, if you are, for once, interested in something other than "participation,
no matter how useless", post the reproducer.