Re: [PATCH 3/5] WIP: fs: ext4: support unlinkat_s() for secure deletion of files

[Lukáš Czerner]

On Mon, 2 Feb 2015, Alexander Holler wrote:

> Date: Mon,  2 Feb 2015 18:05:11 +0100
> From: Alexander Holler <holler@xxxxxxxxxxxxx>
> To: linux-fsdevel@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx, Alexander Holler <holler@xxxxxxxxxxxxx>
> Subject: [PATCH 3/5] WIP: fs: ext4: support unlinkat_s() for secure deletion
>     of files

Hi,

I am missing a description, where you'd describe what is this all
about, why and how.

> 
> Signed-off-by: Alexander Holler <holler@xxxxxxxxxxxxx>
> ---
>  fs/ext4/ext4.h    |  2 ++
>  fs/ext4/mballoc.c | 25 +++++++++++++++++++++++--
>  fs/ext4/super.c   | 12 ++++++++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index c55a1fa..e66507c 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1342,6 +1342,8 @@ struct ext4_sb_info {
>  	struct ratelimit_state s_err_ratelimit_state;
>  	struct ratelimit_state s_warning_ratelimit_state;
>  	struct ratelimit_state s_msg_ratelimit_state;
> +
> +	atomic_t secure_delete;   /* delete blocks securely? */
>  };
>  
>  static inline struct ext4_sb_info *EXT4_SB(struct super_block *sb)
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index dbfe15c..f33416f 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2756,6 +2756,19 @@ static inline int ext4_issue_discard(struct super_block *sb,
>  	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, 0);
>  }
>  
> +static inline int ext4_issue_zeroout(struct super_block *sb,
> +		ext4_group_t block_group, ext4_grpblk_t cluster, int count)
> +{
> +	ext4_fsblk_t discard_block;
> +
> +	discard_block = (EXT4_C2B(EXT4_SB(sb), cluster) +
> +			 ext4_group_first_block_no(sb, block_group));
> +	count = EXT4_C2B(EXT4_SB(sb), count);
> +	//trace_ext4_discard_blocks(sb,
> +	//		(unsigned long long) discard_block, count);
> +	return sb_issue_zeroout(sb, discard_block, count, GFP_NOFS);
> +}
> +
>  /*
>   * This function is called by the jbd2 layer once the commit has finished,
>   * so we know we can free the blocks that were released with that commit.
> @@ -2764,6 +2777,7 @@ static void ext4_free_data_callback(struct super_block *sb,
>  				    struct ext4_journal_cb_entry *jce,
>  				    int rc)
>  {
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct ext4_free_data *entry = (struct ext4_free_data *)jce;
>  	struct ext4_buddy e4b;
>  	struct ext4_group_info *db;
> @@ -2772,6 +2786,11 @@ static void ext4_free_data_callback(struct super_block *sb,
>  	mb_debug(1, "gonna free %u blocks in group %u (0x%p):",
>  		 entry->efd_count, entry->efd_group, entry);
>  
> +
> +	// TODO:
> +	// if (atomic_read(&sbi->secure_delete) && secure_trim_available)
> +	// 	use secure trim
> +	// else

I am missing very important pieces like, what happens if we require
secure delete, but there is no secure trim available and we're still
on the ssd ?

What if the underlying storage is thinly provisioned ?

What if the underlying storage consist of hardware which does
support secure discard and one that does not ? The request crossing
the borders will fail.

What if the underlying hardware does support secure trim, but the
storage under the fs is in raid configuration, which brings  me to
the next question.

Discard/secure discard does have a granularity and alignment, so
what if the extent is smaller than a discard granularity, or it is
not aligned properly ? Such discard requests would be ignored.


>  	if (test_opt(sb, DISCARD)) {
>  		err = ext4_issue_discard(sb, entry->efd_group,
>  					 entry->efd_start_cluster,
> @@ -2782,8 +2801,10 @@ static void ext4_free_data_callback(struct super_block *sb,
>  				 " with %d", entry->efd_group,
>  				 entry->efd_start_cluster,
>  				 entry->efd_count, err);
> -	}
> -
> +	} else if (atomic_read(&sbi->secure_delete))
> +		ext4_issue_zeroout(sb, entry->efd_group,
> +					 entry->efd_start_cluster,
> +					 entry->efd_count);

Error handling is missing here. Also I am not sure that zeroing out
the blocks is really enough. Yes, I've seen the link you've posted,
but I am not convinced.

Did you consider metadata information for the file ? File name,
timestamps, size, data placement ? Is it something you want to
remove as well, or are you going to ignore it ? It can potentially
contain valuable information for the attacker as well. I am just
trying to understand the scope of this thing.

Moreover with inline data you might have the data in the inode
itself, which also means the it will be in the journal as well.

Also with data=journal the data will be in the journal.

With no journal this would not work at all, you have to make this
for nojournal case as well.

What if you do defragmentation in the file, in that case the file data
could be all over the place.

What if you're device is not a real hardware, but just let's say a
loop device ? Talking about the smart phones I had Samsung phone
with that setup (not sure anyone is doing that anymore).


With all that said, the devil is in the details and since it's
security feature the details and corner cases is what you need
to focus on. We have '-o discard' mount option for years now and
we could have made 'secure delete' by simply calling
sb_issue_discard() with BLKDEV_DISCARD_SECURE flag, but that's not
really enough.

Not mentioning the unreliable hardware. And I am not going to rely
on the hardware which was not designed with security in mind for my
security feature, no one should. It's much better, easies and more
feasible just to use disk encryption - it also comes with advantages
that no one can actually read your existing files as opposed to just
deleted files.

>  	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
>  	/* we expect to find existing buddy because it's pinned */
>  	BUG_ON(err != 0);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 2c9e686..f87e3ff 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1100,6 +1100,17 @@ static const struct quotactl_ops ext4_qctl_sysfile_operations = {
>  };
>  #endif
>  
> +static void ext4_set_secure_delete(struct super_block *sb, bool secure)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	// TODO: will overflow with a very large number of
> +	// concurrent calls of unlinkat_s().
> +	if (secure)
> +		atomic_inc(&sbi->secure_delete);
> +	else
> +		atomic_dec(&sbi->secure_delete);
> +}
> +
>  static const struct super_operations ext4_sops = {
>  	.alloc_inode	= ext4_alloc_inode,
>  	.destroy_inode	= ext4_destroy_inode,
> @@ -1119,6 +1130,7 @@ static const struct super_operations ext4_sops = {
>  	.quota_write	= ext4_quota_write,
>  #endif
>  	.bdev_try_to_free_page = bdev_try_to_free_page,
> +	.set_secure_delete = ext4_set_secure_delete,
>  };
>  
>  static const struct export_operations ext4_export_ops = {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/