| Message ID | 20240915064404.221474-1-danielyangkang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/exfat: resolve memory leak from exfat_create_upcase_table() | expand |
On Sat, Sep 14, 2024 at 11:44:03PM -0700, Daniel Yang wrote: > If exfat_load_upcase_table reaches end and returns -EINVAL, > allocated memory doesn't get freed and while > exfat_load_default_upcase_table allocates more memory, leading to a > memory leak. > > Here's link to syzkaller crash report illustrating this issue: > https://syzkaller.appspot.com/text?tag=CrashReport&x=1406c201980000 > > Signed-off-by: Daniel Yang <danielyangkang@gmail.com> > Reported-by: syzbot+e1c69cadec0f1a078e3d@syzkaller.appspotmail.com > --- > fs/exfat/nls.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c > index afdf13c34..ec69477d0 100644 > --- a/fs/exfat/nls.c > +++ b/fs/exfat/nls.c > @@ -699,6 +699,7 @@ static int exfat_load_upcase_table(struct super_block *sb, > > exfat_err(sb, "failed to load upcase table (idx : 0x%08x, chksum : 0x%08x, utbl_chksum : 0x%08x)", > index, chksum, utbl_checksum); > + exfat_free_upcase_table(sbi); > return -EINVAL; > } Interesting... How does the mainline manage to avoid the call of exfat_kill_sb(), which should call_rcu() delayed_free(), which calls exfat_free_upcase_table()? Could you verify that your reproducer does *NOT* hit that callchain? AFAICS, the only caller of exfat_load_upcase_table() is exfat_create_upcase_table(), called by __exfat_fill_super(), called by exfat_fill_super(), passed as callback to get_tree_bdev(). And if that's the case, ->kill_sb() should be called on failure and with non-NULL ->s_fs_info... Something odd is going on there.
On Sun, Sep 15, 2024 at 08:05:46AM +0100, Al Viro wrote: > Interesting... How does the mainline manage to avoid the > call of exfat_kill_sb(), which should call_rcu() delayed_free(), which > calls exfat_free_upcase_table()? > > Could you verify that your reproducer does *NOT* hit that > callchain? AFAICS, the only caller of exfat_load_upcase_table() > is exfat_create_upcase_table(), called by __exfat_fill_super(), > called by exfat_fill_super(), passed as callback to get_tree_bdev(). > And if that's the case, ->kill_sb() should be called on failure and > with non-NULL ->s_fs_info... > > Something odd is going on there. Yecchh... OK, I see what's happening, and the patch is probably correct, but IMO it's way too subtle. Unless I'm misreading what's going on there, you have the following: exfat_load_upcase_table() have 3 failure exits. One of them is with -ENOMEM; no table allocated and we proceed to exfat_load_default_upcase_table(). Another is with -EIO. In that case the table is left allocated, the caller of exfat_load_upcase_table() returns immediately and the normal logics in ->kill_sb() takes it out. Finally, there's one with -EINVAL. There the caller proceeds to exfat_load_default_upcase_table(), which is where the mainline leaks. That's the case your patch adjusts. Note that resulting rules for exfat_load_upcase_table() * should leave for ->kill_sb() to free if failing with -EIO. * should make sure it's freed on all other failure exits. At the very least that needs to be documented. However, since the problem happens when the caller proceeds to exfat_load_default_upcase_table(), the things would be simpler if you had taken the "need to free what we'd allocated" logics into the place where that logics is visible. I.e. ret = exfat_load_upcase_table(sb, sector, num_sectors, le32_to_cpu(ep->dentry.upcase.checksum)); brelse(bh); if (ret && ret != -EIO) { /* clean after exfat_load_upcase_table() */ exfat_free_upcase_table(sbi); goto load_default; } IMO it would be less brittle that way. And commit message needs the explanation of the leak mechanism - a link to reporter is nice, but it doesn't explain what's going on.
On Sun, Sep 15, 2024 at 08:23:36AM +0100, Al Viro wrote: > IMO it would be less brittle that way. And commit message needs > the explanation of the leak mechanism - a link to reporter is > nice, but it doesn't explain what's going on. Actually, nevermind the part about commit message - what you have there is OK. I still think that the call would be better off in exfat_create_upcase_table(), though - less brittle that way.
diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c index afdf13c34..ec69477d0 100644 --- a/fs/exfat/nls.c +++ b/fs/exfat/nls.c @@ -699,6 +699,7 @@ static int exfat_load_upcase_table(struct super_block *sb, exfat_err(sb, "failed to load upcase table (idx : 0x%08x, chksum : 0x%08x, utbl_chksum : 0x%08x)", index, chksum, utbl_checksum); + exfat_free_upcase_table(sbi); return -EINVAL; }
If exfat_load_upcase_table reaches end and returns -EINVAL, allocated memory doesn't get freed and while exfat_load_default_upcase_table allocates more memory, leading to a memory leak. Here's link to syzkaller crash report illustrating this issue: https://syzkaller.appspot.com/text?tag=CrashReport&x=1406c201980000 Signed-off-by: Daniel Yang <danielyangkang@gmail.com> Reported-by: syzbot+e1c69cadec0f1a078e3d@syzkaller.appspotmail.com --- fs/exfat/nls.c | 1 + 1 file changed, 1 insertion(+)