[PATCH] fs/ntfs3: Fix memory leak if ntfs_read_mft failed

[PATCH] fs/ntfs3: Fix memory leak if ntfs_read_mft failed

[Chen Zhongjin]

Label ATTR_ROOT in ntfs_read_mft() sets is_root = true and
ni->ni_flags |= NI_FLAG_DIR, then next attr will goto label ATTR_ALLOC
and alloc ni->dir.alloc_run. However two states are not always
consistent and can make memory leak.

 1) attr_name in ATTR_ROOT does not fit the condition it will set
 is_root = true but NI_FLAG_DIR is not set.
 2) next attr_name in ATTR_ALLOC fits the condition and alloc
 ni->dir.alloc_run
 3) in cleanup function ni_clear(), when NI_FLAG_DIR is set, it frees
 ni->dir.alloc_run, otherwise it frees ni->file.run
 4) because NI_FLAG_DIR is not set in this case, ni->dir.alloc_run is
 leaked as kmemleak reported:

unreferenced object 0xffff888003bc5480 (size 64):
  backtrace:
    [<000000003d42e6b0>] __kmalloc_node+0x4e/0x1c0
    [<00000000d8e19b8a>] kvmalloc_node+0x39/0x1f0
    [<00000000fc3eb5b8>] run_add_entry+0x18a/0xa40 [ntfs3]
    [<0000000011c9f978>] run_unpack+0x75d/0x8e0 [ntfs3]
    [<00000000e7cf1819>] run_unpack_ex+0xbc/0x500 [ntfs3]
    [<00000000bbf0a43d>] ntfs_iget5+0xb25/0x2dd0 [ntfs3]
    [<00000000a6e50693>] ntfs_fill_super+0x218d/0x3580 [ntfs3]
    [<00000000b9170608>] get_tree_bdev+0x3fb/0x710
    [<000000004833798a>] vfs_get_tree+0x8e/0x280
    [<000000006e20b8e6>] path_mount+0xf3c/0x1930
    [<000000007bf15a5f>] do_mount+0xf3/0x110
    ...

Fix this by always setting is_root and NI_FLAG_DIR together.

Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 fs/ntfs3/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index d5a3afbbbfd8..b7f60624b044 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -247,7 +247,6 @@ static struct inode *ntfs_read_mft(struct inode *inode,
 			goto out;
 
 		root = Add2Ptr(attr, roff);
-		is_root = true;
 
 		if (attr->name_len != ARRAY_SIZE(I30_NAME) ||
 		    memcmp(attr_name(attr), I30_NAME, sizeof(I30_NAME)))
@@ -260,6 +259,7 @@ static struct inode *ntfs_read_mft(struct inode *inode,
 		if (!is_dir)
 			goto next_attr;
 
+		is_root = true;
 		ni->ni_flags |= NI_FLAG_DIR;
 
 		err = indx_init(&ni->dir, sbi, attr, INDEX_MUTEX_I30);
-- 
2.17.1

Re: [PATCH] fs/ntfs3: Fix memory leak if ntfs_read_mft failed

[Konstantin Komarov]

On 22.11.2022 13:24, Chen Zhongjin wrote:
> Label ATTR_ROOT in ntfs_read_mft() sets is_root = true and
> ni->ni_flags |= NI_FLAG_DIR, then next attr will goto label ATTR_ALLOC
> and alloc ni->dir.alloc_run. However two states are not always
> consistent and can make memory leak.
>
>   1) attr_name in ATTR_ROOT does not fit the condition it will set
>   is_root = true but NI_FLAG_DIR is not set.
>   2) next attr_name in ATTR_ALLOC fits the condition and alloc
>   ni->dir.alloc_run
>   3) in cleanup function ni_clear(), when NI_FLAG_DIR is set, it frees
>   ni->dir.alloc_run, otherwise it frees ni->file.run
>   4) because NI_FLAG_DIR is not set in this case, ni->dir.alloc_run is
>   leaked as kmemleak reported:
>
> unreferenced object 0xffff888003bc5480 (size 64):
>    backtrace:
>      [<000000003d42e6b0>] __kmalloc_node+0x4e/0x1c0
>      [<00000000d8e19b8a>] kvmalloc_node+0x39/0x1f0
>      [<00000000fc3eb5b8>] run_add_entry+0x18a/0xa40 [ntfs3]
>      [<0000000011c9f978>] run_unpack+0x75d/0x8e0 [ntfs3]
>      [<00000000e7cf1819>] run_unpack_ex+0xbc/0x500 [ntfs3]
>      [<00000000bbf0a43d>] ntfs_iget5+0xb25/0x2dd0 [ntfs3]
>      [<00000000a6e50693>] ntfs_fill_super+0x218d/0x3580 [ntfs3]
>      [<00000000b9170608>] get_tree_bdev+0x3fb/0x710
>      [<000000004833798a>] vfs_get_tree+0x8e/0x280
>      [<000000006e20b8e6>] path_mount+0xf3c/0x1930
>      [<000000007bf15a5f>] do_mount+0xf3/0x110
>      ...
>
> Fix this by always setting is_root and NI_FLAG_DIR together.
>
> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>   fs/ntfs3/inode.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index d5a3afbbbfd8..b7f60624b044 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -247,7 +247,6 @@ static struct inode *ntfs_read_mft(struct inode *inode,
>   			goto out;
>   
>   		root = Add2Ptr(attr, roff);
> -		is_root = true;
>   
>   		if (attr->name_len != ARRAY_SIZE(I30_NAME) ||
>   		    memcmp(attr_name(attr), I30_NAME, sizeof(I30_NAME)))
> @@ -260,6 +259,7 @@ static struct inode *ntfs_read_mft(struct inode *inode,
>   		if (!is_dir)
>   			goto next_attr;
>   
> +		is_root = true;
>   		ni->ni_flags |= NI_FLAG_DIR;
>   
>   		err = indx_init(&ni->dir, sbi, attr, INDEX_MUTEX_I30);
Thanks for patch, applied!