[PATCH v2 0/2] ubifs: Fix two kmemleaks in error path

[PATCH v2 0/2] ubifs: Fix two kmemleaks in error path

[Zhihao Cheng]

First memleak is found by mounting corrupted UBIFS image with chk_index
enabled.
Second memleak is found by powercut testing for encryption scenario.

v1->v2:
  ubifs_symlink: Call fscrypt_free_inode() directly in error handling
  path.
  Add 'Cc: stable@vger.kernel.org' and 'Suggested-by' tags in patch 2.

Zhihao Cheng (2):
  ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed
  ubifs: ubifs_symlink: Fix memleak of inode->i_link in error path

 fs/ubifs/dir.c   | 2 ++
 fs/ubifs/super.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH v2 1/2] ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed

[Zhihao Cheng]

If function dbg_check_idx_size() failed by loading znode in mounting
process, there are two problems:
  1. Allocated znodes won't be freed, which causes kmemleak in kernel:
     ubifs_mount
      dbg_check_idx_size
       dbg_walk_index
        c->zroot.znode = ubifs_load_znode
	child = ubifs_load_znode // failed
	// Loaded znodes won't be freed in error handling path.
  2. Global variable ubifs_clean_zn_cnt is not decreased, because
     ubifs_tnc_close() is not invoked in error handling path, which
     triggers a warning in ubifs_exit():
      WARNING: CPU: 1 PID: 1576 at fs/ubifs/super.c:2486 ubifs_exit
      Modules linked in: zstd ubifs(-) ubi nandsim
      CPU: 1 PID: 1576 Comm: rmmod Not tainted 6.7.0-rc6
      Call Trace:
	ubifs_exit+0xca/0xc70 [ubifs]
	__do_sys_delete_module+0x29a/0x4a0
	do_syscall_64+0x6f/0x140

Fix it by invoking destroy_journal() if dbg_check_idx_size() failed.

Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 09e270d6ed02..eabb0f44ea3e 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1449,7 +1449,7 @@ static int mount_ubifs(struct ubifs_info *c)
 
 	err = dbg_check_idx_size(c, c->bi.old_idx_sz);
 	if (err)
-		goto out_lpt;
+		goto out_journal;
 
 	err = ubifs_replay_journal(c);
 	if (err)
-- 
2.31.1

[PATCH v2 2/2] ubifs: ubifs_symlink: Fix memleak of inode->i_link in error path

[Zhihao Cheng]

For error handling path in ubifs_symlink(), inode will be marked as
bad first, then iput() is invoked. If inode->i_link is initialized by
fscrypt_encrypt_symlink() in encryption scenario, inode->i_link won't
be freed by callchain ubifs_free_inode -> fscrypt_free_inode in error
handling path, because make_bad_inode() has changed 'inode->i_mode' as
'S_IFREG'.
Following kmemleak is easy to be reproduced by injecting error in
ubifs_jnl_update() when doing symlink in encryption scenario:
 unreferenced object 0xffff888103da3d98 (size 8):
  comm "ln", pid 1692, jiffies 4294914701 (age 12.045s)
  backtrace:
   kmemdup+0x32/0x70
   __fscrypt_encrypt_symlink+0xed/0x1c0
   ubifs_symlink+0x210/0x300 [ubifs]
   vfs_symlink+0x216/0x360
   do_symlinkat+0x11a/0x190
   do_syscall_64+0x3b/0xe0
There are two ways fixing it:
 1. Remove make_bad_inode() in error handling path. We can do that
    because ubifs_evict_inode() will do same processes for good
    symlink inode and bad symlink inode, for inode->i_nlink checking
    is before is_bad_inode().
 2. Free inode->i_link before marking inode bad.
Method 2 is picked, it has less influence, personally, I think.

Cc: stable@vger.kernel.org
Fixes: 2c58d548f570 ("fscrypt: cache decrypted symlink target in ->i_link")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Suggested-by: Eric Biggers <ebiggers@kernel.org>
---
 fs/ubifs/dir.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 3b13c648d490..e413a9cf8ee3 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1234,6 +1234,8 @@ static int ubifs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	dir_ui->ui_size = dir->i_size;
 	mutex_unlock(&dir_ui->ui_mutex);
 out_inode:
+	/* Free inode->i_link before inode is marked as bad. */
+	fscrypt_free_inode(inode);
 	make_bad_inode(inode);
 	iput(inode);
 out_fname:
-- 
2.31.1

Re: [PATCH v2 2/2] ubifs: ubifs_symlink: Fix memleak of inode->i_link in error path

[Eric Biggers]

On Fri, Dec 22, 2023 at 04:54:46PM +0800, Zhihao Cheng wrote:
> For error handling path in ubifs_symlink(), inode will be marked as
> bad first, then iput() is invoked. If inode->i_link is initialized by
> fscrypt_encrypt_symlink() in encryption scenario, inode->i_link won't
> be freed by callchain ubifs_free_inode -> fscrypt_free_inode in error
> handling path, because make_bad_inode() has changed 'inode->i_mode' as
> 'S_IFREG'.
> Following kmemleak is easy to be reproduced by injecting error in
> ubifs_jnl_update() when doing symlink in encryption scenario:
>  unreferenced object 0xffff888103da3d98 (size 8):
>   comm "ln", pid 1692, jiffies 4294914701 (age 12.045s)
>   backtrace:
>    kmemdup+0x32/0x70
>    __fscrypt_encrypt_symlink+0xed/0x1c0
>    ubifs_symlink+0x210/0x300 [ubifs]
>    vfs_symlink+0x216/0x360
>    do_symlinkat+0x11a/0x190
>    do_syscall_64+0x3b/0xe0
> There are two ways fixing it:
>  1. Remove make_bad_inode() in error handling path. We can do that
>     because ubifs_evict_inode() will do same processes for good
>     symlink inode and bad symlink inode, for inode->i_nlink checking
>     is before is_bad_inode().
>  2. Free inode->i_link before marking inode bad.
> Method 2 is picked, it has less influence, personally, I think.
> 
> Cc: stable@vger.kernel.org
> Fixes: 2c58d548f570 ("fscrypt: cache decrypted symlink target in ->i_link")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> Suggested-by: Eric Biggers <ebiggers@kernel.org>
> ---
>  fs/ubifs/dir.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 3b13c648d490..e413a9cf8ee3 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -1234,6 +1234,8 @@ static int ubifs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	dir_ui->ui_size = dir->i_size;
>  	mutex_unlock(&dir_ui->ui_mutex);
>  out_inode:
> +	/* Free inode->i_link before inode is marked as bad. */
> +	fscrypt_free_inode(inode);
>  	make_bad_inode(inode);
>  	iput(inode);
>  out_fname:

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric

Re: [PATCH v2 1/2] ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
> An: "richard" <richard@nod.at>, terrelln@fb.com, "Eric Biggers" <ebiggers@google.com>
> CC: "linux-fscrypt" <linux-fscrypt@vger.kernel.org>, "linux-mtd" <linux-mtd@lists.infradead.org>
> Gesendet: Freitag, 22. Dezember 2023 09:54:45
> Betreff: [PATCH v2 1/2] ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed

> If function dbg_check_idx_size() failed by loading znode in mounting
> process, there are two problems:
>  1. Allocated znodes won't be freed, which causes kmemleak in kernel:
>     ubifs_mount
>      dbg_check_idx_size
>       dbg_walk_index
>        c->zroot.znode = ubifs_load_znode
>	child = ubifs_load_znode // failed
>	// Loaded znodes won't be freed in error handling path.
>  2. Global variable ubifs_clean_zn_cnt is not decreased, because
>     ubifs_tnc_close() is not invoked in error handling path, which
>     triggers a warning in ubifs_exit():
>      WARNING: CPU: 1 PID: 1576 at fs/ubifs/super.c:2486 ubifs_exit
>      Modules linked in: zstd ubifs(-) ubi nandsim
>      CPU: 1 PID: 1576 Comm: rmmod Not tainted 6.7.0-rc6
>      Call Trace:
>	ubifs_exit+0xca/0xc70 [ubifs]
>	__do_sys_delete_module+0x29a/0x4a0
>	do_syscall_64+0x6f/0x140
> 
> Fix it by invoking destroy_journal() if dbg_check_idx_size() failed.
> 
> Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
> fs/ubifs/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 09e270d6ed02..eabb0f44ea3e 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1449,7 +1449,7 @@ static int mount_ubifs(struct ubifs_info *c)
> 
> 	err = dbg_check_idx_size(c, c->bi.old_idx_sz);
> 	if (err)
> -		goto out_lpt;
> +		goto out_journal;

While it is technically not wrong, calling destroy_journal() before ubifs_replay_journal() is highly confusing
to future readers of the source code.
It seem to work because ubifs_replay_journal() can deal with the fact the journal is not yet initialized.
I'd rather suggest to add a distinct function which undoes what dbg_check_idx_size() did.
Alternatively, dbg_check_idx_size() could also cleanup itself after failure.

Thanks,
//richard