linux-ext4 - Re: [PATCH] ext4: fix memory leak in ext4_fill

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]

Date:   Thu, 29 Apr 2021 14:33:54 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     Vegard Nossum <vegard.nossum@...cle.com>,
        akpm@...ux-foundation.org, peterz@...radead.org, axboe@...nel.dk,
        pmladek@...e.com
Cc:     tytso@....edu, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzbot+d9e482e303930fa4f6ff@...kaller.appspotmail.com
Subject: Re: [PATCH] ext4: fix memory leak in ext4_fill_super

On Thu, 29 Apr 2021 12:01:46 +0200
Vegard Nossum <vegard.nossum@...cle.com> wrote:

> 
> On 2021-04-28 19:28, Pavel Skripkin wrote:
> > syzbot reported memory leak in ext4 subsyetem.
> > The problem appears, when thread_stop() call happens
> > before wake_up_process().
> > 
> > Normally, this data will be freed by
> > created thread, but if kthread_stop()
> > returned -EINTR, this data should be freed manually
> > 
> > Reported-by: syzbot+d9e482e303930fa4f6ff@...kaller.appspotmail.com
> > Tested-by: syzbot+d9e482e303930fa4f6ff@...kaller.appspotmail.com
> > Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
> > ---
> >   fs/ext4/super.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index b9693680463a..9c33e97bd5c5 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -5156,8 +5156,10 @@ static int ext4_fill_super(struct
> > super_block *sb, void *data, int silent) failed_mount3:
> >   	flush_work(&sbi->s_error_work);
> >   	del_timer_sync(&sbi->s_err_report);
> > -	if (sbi->s_mmp_tsk)
> > -		kthread_stop(sbi->s_mmp_tsk);
> > +	if (sbi->s_mmp_tsk) {
> > +		if (kthread_stop(sbi->s_mmp_tsk) == -EINTR)
> > +			kfree(kthread_data(sbi->s_mmp_tsk));
> > +	}
> >   failed_mount2:
> >   	rcu_read_lock();
> >   	group_desc = rcu_dereference(sbi->s_group_desc);
> > 
> 
> So I've looked at this, and the puzzling thing is that ext4 uses
> kthread_run() which immediately calls wake_up_process() -- according
> to the kerneldoc for kthread_stop(), it shouldn't return -EINTR in
> this case:
> 
>   * Returns the result of threadfn(), or %-EINTR if wake_up_process()
>   * was never called.
>   */
> int kthread_stop(struct task_struct *k)
> 
> So it really looks like kthread_stop() can return -EINTR even when
> wake_up_process() has been called but the thread hasn't had a chance
> to run yet?
> 
> If this is true, then we either have to fix kthread_create() to make
> sure it respects the behaviour that is claimed by the comment OR we
> have to audit every single kthread_stop() in the kernel which does
> not check for -EINTR.
> 
> 
> Vegard

I am sorry for my complitely broken mail client :(

Me and Vegard found the root case of this bug:

static int kthread(void *_create) 
{
	....
	ret = -EINTR;
	if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
		cgroup_kthread_ready();
		__kthread_parkme(self);
		ret = threadfn(data);
	}
	
	do_exit(ret);
}

There is a chance, that kthread_stop() call will happen before
threadfn call. It means, that kthread_stop() return value must be checked everywhere,
isn't it? Otherwise, there are a lot of potential memory leaks,
because some developers rely on the fact, that data allocated for the thread will
be freed _inside_ thread function.

Vegard wrote the code snippet, which reproduces this behavior:

#include <linux/printk.h>
#include <linux/proc_fs.h>
#include <linux/kthread.h>

static int test_thread(void *data)
{
        printk(KERN_ERR "test_thread()\n");
        return 0;
}

static int test_show(struct seq_file *seq, void *data)
{
        struct task_struct *t = kthread_run(test_thread, NULL, "test");
        if (!IS_ERR(t)) {
                int ret = kthread_stop(t);
                printk(KERN_ERR "kthread_stop() = %d\n", ret);
        }

        return 0;
}

static void __init init_test(void)
{
        proc_create_single("test", 0444, NULL, &test_show);
}

late_initcall(init_test);

So, is this behavior is expected or not? Should maintainers rewrite
code, which doesn't check kthread_stop() return value?


With regards,
Pavel Skripkin