| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220113110042.ahquxopitdwug7xo@quack3.lan>
Date: Thu, 13 Jan 2022 12:00:42 +0100
From: Jan Kara <jack@...e.cz>
To: Ritesh Harjani <riteshh@...ux.ibm.com>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, Jan Kara <jack@...e.com>,
Andreas Dilger <adilger.kernel@...ger.ca>, tytso@....edu,
Eric Whitney <enwlinux@...il.com>,
luo penghao <luo.penghao@....com.cn>,
Lukas Czerner <lczerner@...hat.com>
Subject: Re: [PATCH 3/6] ext4: Fix error handling in
ext4_fc_record_modified_inode()
On Thu 13-01-22 08:56:26, Ritesh Harjani wrote:
> Current code does not fully takes care of krealloc() error case,
> which could lead to silent memory corruption or a kernel bug.
> This patch fixes that.
>
> Also it cleans up some duplicated error handling logic from various functions
> in fast_commit.c file.
>
> Reported-by: luo penghao <luo.penghao@....com.cn>
> Suggested-by: Lukas Czerner <lczerner@...hat.com>
> Signed-off-by: Ritesh Harjani <riteshh@...ux.ibm.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@...e.cz>
Honza
> ---
> fs/ext4/fast_commit.c | 64 ++++++++++++++++++++-----------------------
> 1 file changed, 29 insertions(+), 35 deletions(-)
>
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 5ae8026a0c56..4541c8468c01 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1392,14 +1392,15 @@ static int ext4_fc_record_modified_inode(struct super_block *sb, int ino)
> if (state->fc_modified_inodes[i] == ino)
> return 0;
> if (state->fc_modified_inodes_used == state->fc_modified_inodes_size) {
> - state->fc_modified_inodes_size +=
> - EXT4_FC_REPLAY_REALLOC_INCREMENT;
> state->fc_modified_inodes = krealloc(
> - state->fc_modified_inodes, sizeof(int) *
> - state->fc_modified_inodes_size,
> - GFP_KERNEL);
> + state->fc_modified_inodes,
> + sizeof(int) * (state->fc_modified_inodes_size +
> + EXT4_FC_REPLAY_REALLOC_INCREMENT),
> + GFP_KERNEL);
> if (!state->fc_modified_inodes)
> return -ENOMEM;
> + state->fc_modified_inodes_size +=
> + EXT4_FC_REPLAY_REALLOC_INCREMENT;
> }
> state->fc_modified_inodes[state->fc_modified_inodes_used++] = ino;
> return 0;
> @@ -1431,7 +1432,9 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl,
> }
> inode = NULL;
>
> - ext4_fc_record_modified_inode(sb, ino);
> + ret = ext4_fc_record_modified_inode(sb, ino);
> + if (ret)
> + goto out;
>
> raw_fc_inode = (struct ext4_inode *)
> (val + offsetof(struct ext4_fc_inode, fc_raw_inode));
> @@ -1621,6 +1624,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> }
>
> ret = ext4_fc_record_modified_inode(sb, inode->i_ino);
> + if (ret)
> + goto out;
>
> start = le32_to_cpu(ex->ee_block);
> start_pblk = ext4_ext_pblock(ex);
> @@ -1638,18 +1643,14 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> map.m_pblk = 0;
> ret = ext4_map_blocks(NULL, inode, &map, 0);
>
> - if (ret < 0) {
> - iput(inode);
> - return 0;
> - }
> + if (ret < 0)
> + goto out;
>
> if (ret == 0) {
> /* Range is not mapped */
> path = ext4_find_extent(inode, cur, NULL, 0);
> - if (IS_ERR(path)) {
> - iput(inode);
> - return 0;
> - }
> + if (IS_ERR(path))
> + goto out;
> memset(&newex, 0, sizeof(newex));
> newex.ee_block = cpu_to_le32(cur);
> ext4_ext_store_pblock(
> @@ -1663,10 +1664,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> up_write((&EXT4_I(inode)->i_data_sem));
> ext4_ext_drop_refs(path);
> kfree(path);
> - if (ret) {
> - iput(inode);
> - return 0;
> - }
> + if (ret)
> + goto out;
> goto next;
> }
>
> @@ -1679,10 +1678,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> ret = ext4_ext_replay_update_ex(inode, cur, map.m_len,
> ext4_ext_is_unwritten(ex),
> start_pblk + cur - start);
> - if (ret) {
> - iput(inode);
> - return 0;
> - }
> + if (ret)
> + goto out;
> /*
> * Mark the old blocks as free since they aren't used
> * anymore. We maintain an array of all the modified
> @@ -1702,10 +1699,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> ext4_ext_is_unwritten(ex), map.m_pblk);
> ret = ext4_ext_replay_update_ex(inode, cur, map.m_len,
> ext4_ext_is_unwritten(ex), map.m_pblk);
> - if (ret) {
> - iput(inode);
> - return 0;
> - }
> + if (ret)
> + goto out;
> /*
> * We may have split the extent tree while toggling the state.
> * Try to shrink the extent tree now.
> @@ -1717,6 +1712,7 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> }
> ext4_ext_replay_shrink_inode(inode, i_size_read(inode) >>
> sb->s_blocksize_bits);
> +out:
> iput(inode);
> return 0;
> }
> @@ -1746,6 +1742,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl,
> }
>
> ret = ext4_fc_record_modified_inode(sb, inode->i_ino);
> + if (ret)
> + goto out;
>
> jbd_debug(1, "DEL_RANGE, inode %ld, lblk %d, len %d\n",
> inode->i_ino, le32_to_cpu(lrange.fc_lblk),
> @@ -1755,10 +1753,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl,
> map.m_len = remaining;
>
> ret = ext4_map_blocks(NULL, inode, &map, 0);
> - if (ret < 0) {
> - iput(inode);
> - return 0;
> - }
> + if (ret < 0)
> + goto out;
> if (ret > 0) {
> remaining -= ret;
> cur += ret;
> @@ -1773,15 +1769,13 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl,
> ret = ext4_ext_remove_space(inode, lrange.fc_lblk,
> lrange.fc_lblk + lrange.fc_len - 1);
> up_write(&EXT4_I(inode)->i_data_sem);
> - if (ret) {
> - iput(inode);
> - return 0;
> - }
> + if (ret)
> + goto out;
> ext4_ext_replay_shrink_inode(inode,
> i_size_read(inode) >> sb->s_blocksize_bits);
> ext4_mark_inode_dirty(NULL, inode);
> +out:
> iput(inode);
> -
> return 0;
> }
>
> --
> 2.31.1
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists