Re: memory leak in generic_parse_monolithic [+PATCH]

[Randy Dunlap]

On 11/13/20 9:17 AM, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    af5043c8 Merge tag 'acpi-5.10-rc4' of git://git.kernel.org..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13e8c906500000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=a3f13716fa0212fd
> dashboard link: https://syzkaller.appspot.com/bug?extid=86dc6632faaca40133ab
> compiler:       gcc (GCC) 10.1.0-syz 20200507
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=102a57dc500000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=129ca3d6500000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+86dc6632faaca40133ab@xxxxxxxxxxxxxxxxxxxxxxxxx
> 
> Warning: Permanently added '10.128.0.84' (ECDSA) to the list of known hosts.
> executing program
> executing program
> BUG: memory leak
> unreferenced object 0xffff888111f15a80 (size 32):
>   comm "syz-executor841", pid 8507, jiffies 4294942125 (age 14.070s)
>   hex dump (first 32 bytes):
>     25 5e 5d 24 5b 2b 25 5d 28 24 7b 3a 0f 6b 5b 29  %^]$[+%](${:.k[)
>     2d 3a 00 00 00 00 00 00 00 00 00 00 00 00 00 00  -:..............
>   backtrace:
>     [<000000005c6f565d>] kmemdup_nul+0x2d/0x70 mm/util.c:151
>     [<0000000054985c27>] vfs_parse_fs_string+0x6e/0xd0 fs/fs_context.c:155
>     [<0000000077ef66e4>] generic_parse_monolithic+0xe0/0x130 fs/fs_context.c:201
>     [<00000000d4d4a652>] do_new_mount fs/namespace.c:2871 [inline]
>     [<00000000d4d4a652>] path_mount+0xbbb/0x1170 fs/namespace.c:3205
>     [<00000000f43f0071>] do_mount fs/namespace.c:3218 [inline]
>     [<00000000f43f0071>] __do_sys_mount fs/namespace.c:3426 [inline]
>     [<00000000f43f0071>] __se_sys_mount fs/namespace.c:3403 [inline]
>     [<00000000f43f0071>] __x64_sys_mount+0x18e/0x1d0 fs/namespace.c:3403
>     [<00000000dc5fffd5>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>     [<000000004e665669>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> 

Hi David,
Is this a false positive, maybe having to do with this comment from
fs/fsopen.c: ?

/*
 * Check the state and apply the configuration.  Note that this function is
 * allowed to 'steal' the value by setting param->xxx to NULL before returning.
 */
static int vfs_fsconfig_locked(struct fs_context *fc, int cmd,
			       struct fs_parameter *param)
{


Otherwise please look at the patch below.
Thanks.

> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@xxxxxxxxxxxxxxxx.
> 
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this issue, for details see:
> https://goo.gl/tpsmEJ#testing-patches


---
From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>

Callers to vfs_parse_fs_param() should be responsible for freeing
param.string.

Fixes: ecdab150fddb ("vfs: syscall: Add fsconfig() for configuring and managing a context")
Signed-off-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
Reported-by: syzbot+86dc6632faaca40133ab@xxxxxxxxxxxxxxxxxxxxxxxxx
Cc: David Howells <dhowells@xxxxxxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
---
This looks promising to me but I haven't fully tested it yet
because my build/test machine just started acting flaky,
like it is having memory or disk errors.
OTOH, it could have ramifications in other places.

 fs/fs_context.c |    1 -
 fs/fsopen.c     |    4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)

--- linux-next-20201204.orig/fs/fs_context.c
+++ linux-next-20201204/fs/fs_context.c
@@ -128,7 +128,6 @@ int vfs_parse_fs_param(struct fs_context
 		if (fc->source)
 			return invalf(fc, "VFS: Multiple sources");
 		fc->source = param->string;
-		param->string = NULL;
 		return 0;
 	}
 
--- linux-next-20201204.orig/fs/fsopen.c
+++ linux-next-20201204/fs/fsopen.c
@@ -262,7 +262,9 @@ static int vfs_fsconfig_locked(struct fs
 		    fc->phase != FS_CONTEXT_RECONF_PARAMS)
 			return -EBUSY;
 
-		return vfs_parse_fs_param(fc, param);
+		ret = vfs_parse_fs_param(fc, param);
+		kfree(param->string);
+		return ret;
 	}
 	fc->phase = FS_CONTEXT_FAILED;
 	return ret;