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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ccfb3a62-f0dd-4bca-0c78-54f78d418815@gmail.com>
Date:   Fri, 6 Mar 2020 11:17:45 +0800
From:   brookxu <brookxu.cn@...il.com>
To:     tytso@....edu, adilger.kernel@...ger.ca,
        linux-kernel@...r.kernel.org
Cc:     linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: mark extents index blocks as dirty to avoid
 information leakage

PING

brookxu wrote on 2020/3/3 16:51:
> From: Chunguang Xu <brookxu@...cent.com>
>
> 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@...cent.com>
> ---
>  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))