Re: [syzbot] memory leak in setup_mq_sysctls

[Catalin Marinas]

On Tue, Jun 21, 2022 at 09:30:57AM -0500, Eric W. Biederman wrote:
> Alexey Gladkov <gladkov.alexey@xxxxxxxxx> writes:
> > On Sun, Jun 19, 2022 at 11:52:25PM -0700, syzbot wrote:
> >> syzbot found the following issue on:
> >> 
> >> HEAD commit:    979086f5e006 Merge tag 'fs.fixes.v5.19-rc3' of git://git.k..
> >> git tree:       upstream
> >> console output: https://syzkaller.appspot.com/x/log.txt?x=1284331bf00000
> >> kernel config:  https://syzkaller.appspot.com/x/.config?x=c696a83383a77f81
> >> dashboard link: https://syzkaller.appspot.com/bug?extid=b4b0d1b35442afbf6fd2
> >> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> >> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=163e740ff00000
> >> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=132b758bf00000
> >> 
> >> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> >> Reported-by: syzbot+b4b0d1b35442afbf6fd2@xxxxxxxxxxxxxxxxxxxxxxxxx
> >
> > I'm working on a fix that will remove this memory allocation entirely.
[...]
> Catalin is it possible that the clever use of ctl_table_arg to hold the
> reference to the table before it is freed is confusing the memory leak
> detector?  The idiom is old enough I don't expect so, but I have seen
> bugs lurk for a long time.

As long as the addresses are not obfuscated and can be reached from some
root object (e.g. in the .data/.bss section), there shouldn't be a
problem. There are some occasional brief false positives as kmemleak
doesn't stop the world during scanning but IIRC syszbot does the
scanning twice to reduce them.

Some comments in the traces below:

> >>   backtrace:
> >>     [<ffffffff814b6eb3>] kmemdup+0x23/0x50 mm/util.c:129
> >>     [<ffffffff82219a9b>] kmemdup include/linux/fortify-string.h:456 [inline]
> >>     [<ffffffff82219a9b>] setup_mq_sysctls+0x4b/0x1c0 ipc/mq_sysctl.c:89

This one allocate a struct ctl_table.

> >>   backtrace:
> >>     [<ffffffff816fea1b>] kmalloc include/linux/slab.h:605 [inline]
> >>     [<ffffffff816fea1b>] kzalloc include/linux/slab.h:733 [inline]
> >>     [<ffffffff816fea1b>] __register_sysctl_table+0x7b/0x7f0 fs/proc/proc_sysctl.c:1344
> >>     [<ffffffff82219b7a>] setup_mq_sysctls+0x12a/0x1c0 ipc/mq_sysctl.c:112

This allocates struct ctl_table_header and IIUC, it stores a pointer to
the table allocated above. So if this one leaks, the ctl_table object
would also be reported as a leak.

> >>   backtrace:
> >>     [<ffffffff816fef49>] kmalloc include/linux/slab.h:605 [inline]
> >>     [<ffffffff816fef49>] kzalloc include/linux/slab.h:733 [inline]
> >>     [<ffffffff816fef49>] new_dir fs/proc/proc_sysctl.c:978 [inline]
> >>     [<ffffffff816fef49>] get_subdir fs/proc/proc_sysctl.c:1022 [inline]
> >>     [<ffffffff816fef49>] __register_sysctl_table+0x5a9/0x7f0 fs/proc/proc_sysctl.c:1373
> >>     [<ffffffff82219b7a>] setup_mq_sysctls+0x12a/0x1c0 ipc/mq_sysctl.c:112
[...]
> >>   backtrace:
> >>     [<ffffffff816fef49>] kmalloc include/linux/slab.h:605 [inline]
> >>     [<ffffffff816fef49>] kzalloc include/linux/slab.h:733 [inline]
> >>     [<ffffffff816fef49>] new_dir fs/proc/proc_sysctl.c:978 [inline]
> >>     [<ffffffff816fef49>] get_subdir fs/proc/proc_sysctl.c:1022 [inline]
> >>     [<ffffffff816fef49>] __register_sysctl_table+0x5a9/0x7f0 fs/proc/proc_sysctl.c:1373
> >>     [<ffffffff82219b7a>] setup_mq_sysctls+0x12a/0x1c0 ipc/mq_sysctl.c:112

These two places allocate a struct ctl_dir. Are the ctl_dir objects
supposed to have a pointer to the previously allocated header or the
other way around? At a quick look, I think it's the latter as
insert_header() stores 'dir' into header->parent. Anyway, for some
reason kmemleak cannot reach the ctl_dir or ctl_table_header objects.
If one refers the other, we should focus on tracking down the parent
object.

I'll stare at the code a bit more tomorrow.

-- 
Catalin