Re: [PATCH] ext4: mark extents index blocks as dirty to avoid information leakage

[brookxu]

PING

brookxu wrote on 2020/3/3 16:51:
> From: Chunguang Xu <brookxu@xxxxxxxxxxx>
>
> In the scene of deleting a file, the physical block information in the
> extent will be cleared to 0, the buffer_head contains these extents is
> marked as dirty, and then managed by jbd2, which will clear the
> buffer_head's dirty flag by clear_buffer_dirty. However, when the entire
> extent block is deleted, it is revoked from the jbd2, but the extents
> block is not redirtied.
>
> Not quite reasonable here, for the following concerns:
>
> 1. This has the risk of information leakage and leads to an interesting
> phenomenon that deleting the entire file is no more secure than truncate
> to 1 byte, because the whole extents physical block clear to zero in cache
> will never written back as the page is not redirtied.
>
> 2. For large files, the number of index block is usually very small.
> Ignoring index pages not get much more benefit in IO performance. But if
> we remark the page as dirty, the page is then written back by the system
> writeback mechanism asynchronously with little performance impact. As a
> result, the risk of information leakage can be avoided. At least not wrose
> than truncate file length to 1 byte
>
> Therefore, when the index block is released, we need to remark its page
> as dirty, so that the index block on the disk will be updated and the
> data is more security.
>
> Signed-off-by: Chunguang Xu <brookxu@xxxxxxxxxxx>
> ---
> Âfs/ext4/ext4.hÂÂÂÂÂ | 1 +
> Âfs/ext4/ext4_jbd2.c | 8 ++++++++
> Âfs/ext4/extents.cÂÂ | 3 ++-
> Â3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 61b37a0..f9a4d97 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -644,6 +644,7 @@ enum {
> Â#define EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTERÂÂÂ 0x0010
> Â#define EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTERÂÂÂ 0x0020
> Â#define EXT4_FREE_BLOCKS_RERESERVE_CLUSTERÂÂÂÂÂ 0x0040
> +#define EXT4_FREE_BLOCKS_METADATA_INDEXÂÂÂ ÂÂÂ 0x0080
> Â
> Â/*
> Â * ioctl commands
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index 1f53d64..7974c62 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -275,7 +275,15 @@ int __ext4_forget(const char *where, unsigned int line, handle_t *handle,
> ÂÂÂÂ ÂÂÂ ext4_set_errno(inode->i_sb, -err);
> ÂÂÂÂ ÂÂÂ __ext4_abort(inode->i_sb, where, line,
> ÂÂÂÂ ÂÂÂ ÂÂÂ ÂÂ "error %d when attempting revoke", err);
> + } else {
> +ÂÂÂ ÂÂÂ /*
> +ÂÂÂ ÂÂÂ Â* we dirtied index block to ensure that related changes to
> +ÂÂÂ ÂÂÂ Â* the index block will be stored to disk.
> +ÂÂÂ ÂÂÂ Â*/
> +ÂÂÂ ÂÂÂ if (is_metadata & EXT4_FREE_BLOCKS_METADATA_INDEX)
> +ÂÂÂ ÂÂÂ ÂÂÂ mark_buffer_dirty(bh);
> ÂÂÂÂ }
> +
> ÂÂÂÂ BUFFER_TRACE(bh, "exit");
> ÂÂÂÂ return err;
> Â}
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 954013d..2ee0df0 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2431,7 +2431,8 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
> ÂÂÂÂ trace_ext4_ext_rm_idx(inode, leaf);
> Â
> ÂÂÂÂ ext4_free_blocks(handle, inode, NULL, leaf, 1,
> -ÂÂÂ ÂÂÂ ÂÂÂ ÂEXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
> +ÂÂÂ ÂÂÂ ÂÂÂ ÂEXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET |
> +ÂÂÂ ÂÂÂ ÂÂÂ ÂEXT4_FREE_BLOCKS_METADATA_INDEX);
> Â
> ÂÂÂÂ while (--depth >= 0) {
> ÂÂÂÂ ÂÂÂ if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))