memory leak in generic_parse_monolithic

[syzbot]

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+86dc66...@syzkaller.appspotmail.com

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



---
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 syzk...@googlegroups.com.

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

[Randy Dunlap]

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 syzk...@googlegroups.com.
>
> 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 <rdu...@infradead.org>

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 <rdu...@infradead.org>
Reported-by: syzbot+86dc66...@syzkaller.appspotmail.com
Cc: David Howells <dhow...@redhat.com>
Cc: Al Viro <vi...@zeniv.linux.org.uk>
---
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;

[David Howells]

Randy Dunlap <rdu...@infradead.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?

David

[Randy Dunlap]

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

Thanks for your comments & help.

--
~Randy

[David Howells]

Randy Dunlap <rdu...@infradead.org> wrote:

> > 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).

Hmmm... afs parses the string in afs_parse_source() without modifying it,
then moves the pointer to fc->source (parallelling vfs_parse_fs_param()) and
doesn't touch it again. fc->source should be cleaned up by do_new_mount()
calling put_fs_context() at the end of the function.

As far as I can tell with the attached print-insertion patch, it works, called
by the following commands, some of which are correct and some which aren't:

# mount -t afs none /xfstest.test/ -o dyn
# umount /xfstest.test
# mount -t afs "" /xfstest.test/ -o foo
mount: /xfstest.test: bad option; for several filesystems (e.g. nfs, cifs) you might need a /sbin/mount.<type> helper program.
# umount /xfstest.test
umount: /xfstest.test: not mounted.
# mount -t afs %xfstest.test20 /xfstest.test/ -o foo
mount: /xfstest.test: bad option; for several filesystems (e.g. nfs, cifs) you might need a /sbin/mount.<type> helper program.
# umount /xfstest.test
umount: /xfstest.test: not mounted.
# mount -t afs %xfstest.test20 /xfstest.test/
# umount /xfstest.test

Do you know if the mount was successful and what the mount parameters were?

David
---
diff --git a/fs/afs/super.c b/fs/afs/super.c
index 6c5900df6aa5..4c44ec0196c9 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -299,7 +299,7 @@ static int afs_parse_source(struct fs_context *fc, struct fs_parameter *param)
ctx->cell = cell;
}

- _debug("CELL:%s [%p] VOLUME:%*.*s SUFFIX:%s TYPE:%d%s",
+ kdebug("CELL:%s [%p] VOLUME:%*.*s SUFFIX:%s TYPE:%d%s",
ctx->cell->name, ctx->cell,
ctx->volnamesz, ctx->volnamesz, ctx->volname,
suffix ?: "-", ctx->type, ctx->force ? " FORCE" : "");
@@ -318,6 +318,8 @@ static int afs_parse_param(struct fs_context *fc, struct fs_parameter *param)
struct afs_fs_context *ctx = fc->fs_private;
int opt;

+ kenter("%s,%p '%s'", param->key, param->string, param->string);
+
opt = fs_parse(fc, afs_fs_parameters, param, &result);
if (opt < 0)
return opt;
diff --git a/fs/fs_context.c b/fs/fs_context.c
index 2834d1afa6e8..f530a33876ce 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -450,6 +450,8 @@ void put_fs_context(struct fs_context *fc)
put_user_ns(fc->user_ns);
put_cred(fc->cred);
put_fc_log(fc);
+ if (strcmp(fc->fs_type->name, "afs") == 0)
+ printk("PUT %p '%s'\n", fc->source, fc->source);
put_filesystem(fc->fs_type);
kfree(fc->source);
kfree(fc);
@@ -671,6 +673,8 @@ void vfs_clean_context(struct fs_context *fc)
fc->s_fs_info = NULL;
fc->sb_flags = 0;
security_free_mnt_opts(&fc->security);
+ if (strcmp(fc->fs_type->name, "afs") == 0)
+ printk("CLEAN %p '%s'\n", fc->source, fc->source);

[Randy Dunlap]

Here's the syzbot reproducer:
https://syzkaller.appspot.com/x/repro.c?x=129ca3d6500000

The "interesting" mount params are:
source=%^]$[+%](${:\017k[)-:,source=%^]$[+.](%{:\017\200[)-:,\000

There is no other AFS activity: nothing mounted, no cells known (or
whatever that is), etc.

I don't recall if the mount was successful and I can't test it just now.
My laptop is mucked up.


Be aware that this report could just be a false positive: it waits
for 5 seconds then looks for a memleak. AFAIK, it's possible that the "leaked"
memory is still in valid use and will be freed some day.

I'll check more after my test machine is working again.

thanks.
--
~Randy

[David Howells]

Randy Dunlap <rdu...@infradead.org> wrote:

> Here's the syzbot reproducer:
> https://syzkaller.appspot.com/x/repro.c?x=129ca3d6500000
>
> The "interesting" mount params are:
> source=%^]$[+%](${:\017k[)-:,source=%^]$[+.](%{:\017\200[)-:,\000
>
> There is no other AFS activity: nothing mounted, no cells known (or
> whatever that is), etc.
>
> I don't recall if the mount was successful and I can't test it just now.
> My laptop is mucked up.
>
>
> Be aware that this report could just be a false positive: it waits
> for 5 seconds then looks for a memleak. AFAIK, it's possible that the "leaked"
> memory is still in valid use and will be freed some day.

Bah. Multiple source= parameters. I don't reject the second one, but just
overwrite fc->source.

David

[Dmitry Vyukov]

FWIW KMEMLEAK scans memory for pointers. If it claims a memory leak,
it means the heap object is not referenced anywhere anymore. There are
no live pointers to it to call kfree or anything else.
Some false positives are theoretically possible, but so I don't
remember any, all reported ones were true leaks:
https://syzkaller.appspot.com/upstream/fixed?manager=ci-upstream-gce-leak

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/e6d9fd7e-ea43-25a6-9f1e-16a605de0f2d%40infradead.org.

[Randy Dunlap]

OK, great, thanks for the info.

--
~Randy