linux-kernel - Re: [PATCH] jffs2:freely allocate memory when parameters are invalid

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: <eb679ad2-4020-951c-e4d1-60cb059a5ca8@huawei.com>
Date:   Fri, 20 Sep 2019 22:13:53 +0800
From:   Xiaoming Ni <nixiaoming@...wei.com>
To:     Al Viro <viro@...iv.linux.org.uk>
CC:     <dwmw2@...radead.org>, <dilinger@...ued.net>, <richard@....at>,
        <houtao1@...wei.com>, <bbrezillon@...nel.org>,
        <daniel.santos@...ox.com>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] jffs2:freely allocate memory when parameters are invalid

On 2019/9/20 20:54, Al Viro wrote:
> On Fri, Sep 20, 2019 at 01:45:33PM +0100, Al Viro wrote:
>> On Fri, Sep 20, 2019 at 08:21:53PM +0800, Xiaoming Ni wrote:
>>>
>>>
>>> On 2019/9/20 19:43, Al Viro wrote:
>>>> On Fri, Sep 20, 2019 at 02:54:38PM +0800, Xiaoming Ni wrote:
>>>>> Use kzalloc() to allocate memory in jffs2_fill_super().
>>>>> Freeing memory when jffs2_parse_options() fails will cause
>>>>> use-after-free and double-free in jffs2_kill_sb()
>>>>
>>>> ... so we are not freeing it there.  What's the problem?
>>>
>>> No code logic issues, no memory leaks
>>>
>>> But there is too much code logic between memory allocation and free,
>>> which is difficult to understand.
>>
>> Er?  An instance of jffs2 superblock might have a related object
>> attached to it; it is created in jffs2 superblock constructor and
>> freed in destructor.
>>
>>> The modified code is easier to understand.
>>
>> You are making the cleanup logics harder to follow.
> 
> PS: the whole point of ->kill_sb() is that it's always called on
> superblock destruction, whether that instance had been fully set
> up of failed halfway through.
> 
> In particular, anything like foofs_fill_super() *will* be followed
> by ->kill_sb().  Always.  Which allows for simpler logics in
> failure exits.  And the main thing about those is that they are
> always the bitrot hot spots - they are systematically undertested,
> so that's the last place where you want something non-trivial.
> 
> As for "too much code between"...  Huh?  We fail jffs2_fill_super()
> immediately, which has get_tree_mtd() (or mount_mtd() in slightly
> earlier kernels) destroy the superblock there and then...
> 

Currently releasing jffs2_sb_info in jffs2_kill_sb(),
Then the current code path is
1. drivers/mtd/mtdsuper.c
mount_mtd_aux() {
....
   /* jffs2_sb_info is allocated in jffs2_fill_super, */
    ret = fill_super(sb, data, flags & SB_SILENT ? 1 : 0);
    if (ret < 0) {
        deactivate_locked_super(sb); /* If the parameter is wrong, release it here*/
        return ERR_PTR(ret);
    }
...
}

2. fs/super.c
deactivate_locked_super()
---> fs->kill_sb(s);

3. fs/jffs2/super.c
 jffs2_kill_sb()
    kfree(c); /*release jffs2_sb_info allocated by jffs2_fill_super here

Here memory allocation and release,
experienced the function of mount_mtd_aux/deactivate_locked_super/jffs2_kill_sb three different files,
the path is relatively long,
if any of the three functions between the errors,
it will cause problems (such as memory leaks)

Analyze the code of jffs2_kill_sb:
static void jffs2_kill_sb(struct super_block *sb)
{
    struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
    if (c && !sb_rdonly(sb))
		/* If sb is not read only,
		 * then jffs2_stop_garbage_collect_thread() will be executed
		 * when the jffs2_fill_super parameter is invalid.
		 */
        jffs2_stop_garbage_collect_thread(c);
    kill_mtd_super(sb);
    kfree(c);
}

void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c)
{
    int wait = 0;
	/* When the jffs2_fill_super parameter is invalid,
	 * this lock is not initialized.
	 * Is this a code problem ?
	 */
    spin_lock(&c->erase_completion_lock);
.....

I still think this is easier to understand:
 Free the memory allocated by the current function in the failed branch

thanks
Xiaoming Ni