linux-kernel - Re: memory leak in generic_parse

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

Message-ID: <1c752ffe-8118-f9ea-e928-d92783a5c516@infradead.org>
Date:   Tue, 8 Dec 2020 08:41:50 -0800
From:   Randy Dunlap <rdunlap@...radead.org>
To:     David Howells <dhowells@...hat.com>
Cc:     syzbot <syzbot+86dc6632faaca40133ab@...kaller.appspotmail.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com, viro@...iv.linux.org.uk
Subject: Re: memory leak in generic_parse_monolithic [+PATCH]

On 12/8/20 12:36 AM, David Howells wrote:
> Randy Dunlap <rdunlap@...radead.org> wrote:
> 
>> Otherwise please look at the patch below.
> 
> The patch won't help, since it's not going through sys_fsconfig() - worse, it
> introduces two new errors.
> 
>>  		fc->source = param->string;
>> -		param->string = NULL;
> 
> This will cause the string now attached to fc->source to be freed by the
> caller.  No, the original is doing the correct thing here.  The point is to
> steal the string.
> 
>> @@ -262,7 +262,9 @@ static int vfs_fsconfig_locked(struct fs
>>
>> -		return vfs_parse_fs_param(fc, param);
>> +		ret = vfs_parse_fs_param(fc, param);
>> +		kfree(param->string);
>> +		return ret;
> 
> But your stack trace shows you aren't going through sys_fsconfig(), so this
> function isn't involved.  Further, this introduces a double free, since
> sys_fsconfig() frees param.string after it drops uapi_mutex.
> 
> Looking at the backtrace:
> 
>>      kmemdup_nul+0x2d/0x70 mm/util.c:151
>>      vfs_parse_fs_string+0x6e/0xd0 fs/fs_context.c:155
>>      generic_parse_monolithic+0xe0/0x130 fs/fs_context.c:201
>>      do_new_mount fs/namespace.c:2871 [inline]
>>      path_mount+0xbbb/0x1170 fs/namespace.c:3205
>>      do_mount fs/namespace.c:3218 [inline]
>>      __do_sys_mount fs/namespace.c:3426 [inline]
>>      __se_sys_mount fs/namespace.c:3403 [inline]
>>      __x64_sys_mount+0x18e/0x1d0 fs/namespace.c:3403
> 
> A couple of possibilities spring to mind from that: maybe
> vfs_parse_fs_string() is not releasing the param.string - but that's not the
> problem since we stole the string and the free is definitely there at the
> bottom of the function:
> 
> 	int vfs_parse_fs_string(struct fs_context *fc, const char *key,
> 				const char *value, size_t v_size)
> 	{
> 	...
> 		kfree(param.string);
> 		return ret;
> 	}
> 
> or fc->source is not being cleaned up in vfs_clean_context() - but that's
> there as well:
> 
> 	void vfs_clean_context(struct fs_context *fc)
> 	{
> 	...
> 		kfree(fc->source);
> 		fc->source = NULL;
> 
> In either of these cases, I would expect this to have already become evident
> from other filesystem mounts as there would be a lot of leaking going on,
> particularly with the first.
> 
> Now the backtrace only shows what the state was when the string was allocated;
> it doesn't show what happened to it after that, so another possibility is that
> the filesystem being mounted nicked what vfs_parse_fs_param() had rightfully
> stolen, transferring fc->source somewhere else and then failed to release it -
> most likely on mount failure (ie. it's an error handling bug in the
> filesystem).
> 
> Do we know what filesystem it was?

Yes, it's call AFS (or kAFS).

Thanks for your comments & help.

-- 
~Randy