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;
 }
 
-- 
2.39.2

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.