| Message ID | 1568962478-126260-1-git-send-email-nixiaoming@huawei.com |
|---|---|
| State | New, archived |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | jffs2:freely allocate memory when parameters are invalid | expand |
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?
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. The modified code is easier to understand. thanks Xiaoming Ni
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.
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...
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
On Fri, Sep 20, 2019 at 4:14 PM Xiaoming Ni <nixiaoming@huawei.com> wrote: > I still think this is easier to understand: > Free the memory allocated by the current function in the failed branch Please note that jffs2 is in "odd fixes only" maintenance mode. Therefore patches like this cannot be processed. On my never ending review queue are some other jffs2 patches which seem to address real problems. These go first. I see that many patches come form Huawai, maybe one of you can help maintaining jffs2? Reviews, tests, etc.. are very welcome!
On Fri, Sep 20, 2019 at 10:13:53PM +0800, Xiaoming Ni wrote: > 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, If any of the three functions _what_? > it will cause problems (such as memory leaks) > I still think this is easier to understand: > Free the memory allocated by the current function in the failed branch No. Again, "failed" branch is going to be practically untested during any later code changes. The more you have to do in those, the faster they rots. And they _do_ rot - we'd seen that happening again and again. As a general rule, the fewer cleanups are required on failure exits, the better off we are. > 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); Not in practice and gone in mainline this cycle. But yes, those initializations should've been done prior to any failure exits - "jffs2: free jffs2_sb_info through jffs2_kill_sb()" ought to have moved them prior to the call of jffs2_parse_options().
Hi Richard, On 2019/9/20 22:38, Richard Weinberger wrote: > On Fri, Sep 20, 2019 at 4:14 PM Xiaoming Ni <nixiaoming@huawei.com> wrote: >> I still think this is easier to understand: >> Free the memory allocated by the current function in the failed branch > > Please note that jffs2 is in "odd fixes only" maintenance mode. > Therefore patches like this cannot be processed. > > On my never ending review queue are some other jffs2 patches which > seem to address > real problems. These go first. > > I see that many patches come form Huawei, maybe one of you can help > maintaining jffs2? > Reviews, tests, etc.. are very welcome! > In Huawei we use jffs2 broadly in our products to support filesystem on raw NOR flash and NAND flash, so fixing the bugs in jffs2 means a lot to us. Although I have not read all of jffs2 code thoroughly, I had find and "fixed" some bugs in jffs2 and I am willing to do any help in the jffs2 community. Maybe we can start by testing and reviewing the pending patches in patch work ? Regards, Tao
Tao, ----- Ursprüngliche Mail ----- > Von: "Hou Tao" <houtao1@huawei.com> > In Huawei we use jffs2 broadly in our products to support filesystem on raw > NOR flash and NAND flash, so fixing the bugs in jffs2 means a lot to us. > > Although I have not read all of jffs2 code thoroughly, I had find and "fixed" > some bugs in jffs2 and I am willing to do any help in the jffs2 community. Maybe > we can start by testing and reviewing the pending patches in patch work ? yes, this is a good idea. In MTD's patchwork the jffs2 queue is in bad shape. I tried to catch up but failed to find enough time. So with more eyeballs I think we can bring it in shape again. Basically we need to classify which patches fix important stuff and which do not. Some time ago I make xfstests work with jffs2, I can share (and upstream) these patches too. One of my goals is making sure that we don't break jffs2. xfstests can help. Are you on the OFTC IRC network? On #mtd you can find us MTD guys. Thanks, //richard
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c index af4aa65..bbdae72 100644 --- a/fs/jffs2/super.c +++ b/fs/jffs2/super.c @@ -280,11 +280,13 @@ static int jffs2_fill_super(struct super_block *sb, void *data, int silent) c->mtd = sb->s_mtd; c->os_priv = sb; - sb->s_fs_info = c; ret = jffs2_parse_options(c, data); - if (ret) + if (ret) { + kfree(c); return -EINVAL; + } + sb->s_fs_info = c; /* Initialize JFFS2 superblock locks, the further initialization will * be done later */
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() Reference: commit 92e2921f7eee6345 ("jffs2: free jffs2_sb_info through jffs2_kill_sb()") This makes the code difficult to understand the code path between memory allocation and free is too long The reason for this problem is: Before the jffs2_parse_options() check, "sb->s_fs_info = c;" has been executed, so jffs2_sb_info has been assigned to super_block. we can move "sb->s_fs_info = c;" to the success branch of the function jffs2_parse_options() and free jffs2_sb_info in the failure branch make the code easier to understand. Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> --- fs/jffs2/super.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)