On 24.08.2021 10:51, Dan Carpenter wrote:
> There are three bugs in this code:
> 1) If indx_get_root() fails, then return -EINVAL instead of success.
> 2) On the "/* make root external */" -EOPNOTSUPP; error path it should
> free "re" but it has a memory leak.
> 3) If indx_new() fails then it will lead to an error pointer dereference
> when we call put_indx_node().
>
> I've re-written the error handling to be more clear.
>
> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> fs/ntfs3/index.c | 36 ++++++++++++++++--------------------
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> index 489e0fffbc75..4f2d24010386 100644
> --- a/fs/ntfs3/index.c
> +++ b/fs/ntfs3/index.c
> @@ -1554,12 +1554,12 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> u32 root_size, new_root_size;
> struct ntfs_sb_info *sbi;
> int ds_root;
> - struct INDEX_ROOT *root, *a_root = NULL;
> + struct INDEX_ROOT *root, *a_root;
>
> /* Get the record this root placed in */
> root = indx_get_root(indx, ni, &attr, &mi);
> if (!root)
> - goto out;
> + return -EINVAL;
>
> /*
> * Try easy case:
> @@ -1591,10 +1591,8 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
>
> /* Make a copy of root attribute to restore if error */
> a_root = ntfs_memdup(attr, asize);
> - if (!a_root) {
> - err = -ENOMEM;
> - goto out;
> - }
> + if (!a_root)
> + return -ENOMEM;
>
> /* copy all the non-end entries from the index root to the new buffer.*/
> to_move = 0;
> @@ -1604,7 +1602,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> for (e = e0;; e = hdr_next_de(hdr, e)) {
> if (!e) {
> err = -EINVAL;
> - goto out;
> + goto out_free_root;
> }
>
> if (de_is_last(e))
> @@ -1612,14 +1610,13 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> to_move += le16_to_cpu(e->size);
> }
>
> - n = NULL;
> if (!to_move) {
> re = NULL;
> } else {
> re = ntfs_memdup(e0, to_move);
> if (!re) {
> err = -ENOMEM;
> - goto out;
> + goto out_free_root;
> }
> }
>
> @@ -1636,7 +1633,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> if (ds_root > 0 && used + ds_root > sbi->max_bytes_per_attr) {
> /* make root external */
> err = -EOPNOTSUPP;
> - goto out;
> + goto out_free_re;
> }
>
> if (ds_root)
> @@ -1666,7 +1663,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> /* bug? */
> ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
> err = -EINVAL;
> - goto out1;
> + goto out_free_re;
> }
>
> if (err) {
> @@ -1677,7 +1674,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> /* bug? */
> ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
> }
> - goto out1;
> + goto out_free_re;
> }
>
> e = (struct NTFS_DE *)(root + 1);
> @@ -1688,7 +1685,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> n = indx_new(indx, ni, new_vbn, sub_vbn);
> if (IS_ERR(n)) {
> err = PTR_ERR(n);
> - goto out1;
> + goto out_free_re;
> }
>
> hdr = &n->index->ihdr;
> @@ -1715,7 +1712,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> put_indx_node(n);
> fnd_clear(fnd);
> err = indx_insert_entry(indx, ni, new_de, ctx, fnd);
> - goto out;
> + goto out_free_root;
> }
>
> /*
> @@ -1725,7 +1722,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> e = hdr_insert_de(indx, hdr, new_de, NULL, ctx);
> if (!e) {
> err = -EINVAL;
> - goto out1;
> + goto out_put_n;
> }
> fnd_push(fnd, n, e);
>
> @@ -1734,12 +1731,11 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
>
> n = NULL;
>
> -out1:
> +out_put_n:
> + put_indx_node(n);
> +out_free_re:
> ntfs_free(re);
> - if (n)
> - put_indx_node(n);
> -
> -out:
> +out_free_root:
> ntfs_free(a_root);
> return err;
> }
>
Hi, Dan!
Applied all 3 patches, thanks!
