Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Eric Sandeen]

On 6/12/14, 9:14 PM, JP Abgrall wrote:
> This provides an interface for issuing secure trims to parallel
> the existing interface for non-secure trim.

Which does what?  A bit of digging through git history tells
me, but an idiot reader like myself doesn't know what a
"secure trim" is.  ;)  Would be nice to have that info,
and the reason for elevating it from a block-level ioctl
to an fs-level ioctl in the commit log.

IOWS: your commit log says what, but not why.

Also:

You're adding a new high-level IOCTL, so let's get a bit more
visibility than just linux-ext4; linux-fsdevel cc'd.

one other ext4-specific note below.

> Signed-off-by: Geremy Condra <gcondra@google.com>
> Signed-off-by: JP Abgrall <jpa@google.com>
> ---
>  fs/ext4/ext4.h          |  3 ++-
>  fs/ext4/ioctl.c         | 14 +++++++++++++-
>  fs/ext4/mballoc.c       | 29 +++++++++++++++++++----------
>  include/uapi/linux/fs.h |  1 +
>  4 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 7cc5a0e..cf2ddad 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2082,7 +2082,8 @@ extern int ext4_mb_add_groupinfo(struct super_block *sb,
>  		ext4_group_t i, struct ext4_group_desc *desc);
>  extern int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>  				ext4_fsblk_t block, unsigned long count);
> -extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);
> +extern int ext4_trim_fs(struct super_block *, struct fstrim_range *,
> +				bool secure);
>  
>  /* inode.c */
>  struct buffer_head *ext4_getblk(handle_t *, struct inode *,
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 0f2252e..0a0b483 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -580,11 +580,13 @@ resizefs_out:
>  		return err;
>  	}
>  
> +	case SFITRIM:
>  	case FITRIM:
>  	{
>  		struct request_queue *q = bdev_get_queue(sb->s_bdev);
>  		struct fstrim_range range;
>  		int ret = 0;
> +		bool secure_trim = cmd == SFITRIM;
>  
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
> @@ -592,13 +594,23 @@ resizefs_out:
>  		if (!blk_queue_discard(q))
>  			return -EOPNOTSUPP;
>  
> +		if (secure_trim && !blk_queue_secdiscard(q))
> +			return -EOPNOTSUPP;
> +
> +		if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> +			       EXT4_FEATURE_RO_COMPAT_BIGALLOC)) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "FITRIM not supported with bigalloc");
> +			return -EOPNOTSUPP;
> +		}
> +

This last conditional is unrelated to the patch; if BIGALLOC has another
incomplete part of its implementation, please send it as a standalone patch,
not buried in this one.

Thanks,
-Eric

>  		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
>  		    sizeof(range)))
>  			return -EFAULT;
>  
>  		range.minlen = max((unsigned int)range.minlen,
>  				   q->limits.discard_granularity);
> -		ret = ext4_trim_fs(sb, &range);
> +		ret = ext4_trim_fs(sb, &range, secure_trim);
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 59e3162..b470efe 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2734,16 +2734,18 @@ int ext4_mb_release(struct super_block *sb)
>  }
>  
>  static inline int ext4_issue_discard(struct super_block *sb,
> -		ext4_group_t block_group, ext4_grpblk_t cluster, int count)
> +		ext4_group_t block_group, ext4_grpblk_t cluster, int count,
> +		bool secure)
>  {
>  	ext4_fsblk_t discard_block;
> +	unsigned long flags = secure ? BLKDEV_DISCARD_SECURE : 0;
>  
>  	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_discard(sb, discard_block, count, GFP_NOFS, 0);
> +	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, flags);
>  }
>  
>  /*
> @@ -2765,7 +2767,7 @@ static void ext4_free_data_callback(struct super_block *sb,
>  	if (test_opt(sb, DISCARD)) {
>  		err = ext4_issue_discard(sb, entry->efd_group,
>  					 entry->efd_start_cluster,
> -					 entry->efd_count);
> +					 entry->efd_count, 0);
>  		if (err && err != -EOPNOTSUPP)
>  			ext4_msg(sb, KERN_WARNING, "discard request in"
>  				 " group:%d block:%d count:%d failed"
> @@ -4804,7 +4806,8 @@ do_more:
>  		 * them with group lock_held
>  		 */
>  		if (test_opt(sb, DISCARD)) {
> -			err = ext4_issue_discard(sb, block_group, bit, count);
> +			err = ext4_issue_discard(sb, block_group, bit, count,
> +						 0);
>  			if (err && err != -EOPNOTSUPP)
>  				ext4_msg(sb, KERN_WARNING, "discard request in"
>  					 " group:%d block:%d count:%lu failed"
> @@ -5011,13 +5014,15 @@ error_return:
>   * @count:	number of blocks to TRIM
>   * @group:	alloc. group we are working with
>   * @e4b:	ext4 buddy for the group
> + * @secure:	false to issue a standard discard, true for secure discard
>   *
>   * Trim "count" blocks starting at "start" in the "group". To assure that no
>   * one will allocate those blocks, mark it as used in buddy bitmap. This must
>   * be called with under the group lock.
>   */
>  static int ext4_trim_extent(struct super_block *sb, int start, int count,
> -			     ext4_group_t group, struct ext4_buddy *e4b)
> +			    ext4_group_t group, struct ext4_buddy *e4b,
> +			    bool secure)
>  __releases(bitlock)
>  __acquires(bitlock)
>  {
> @@ -5038,7 +5043,7 @@ __acquires(bitlock)
>  	 */
>  	mb_mark_used(e4b, &ex);
>  	ext4_unlock_group(sb, group);
> -	ret = ext4_issue_discard(sb, group, start, count);
> +	ret = ext4_issue_discard(sb, group, start, count, secure);
>  	ext4_lock_group(sb, group);
>  	mb_free_blocks(NULL, e4b, start, ex.fe_len);
>  	return ret;
> @@ -5051,6 +5056,7 @@ __acquires(bitlock)
>   * @start:		first group block to examine
>   * @max:		last group block to examine
>   * @minblocks:		minimum extent block count
> + * @secure:		false for standard discard, true for secure discard
>   *
>   * ext4_trim_all_free walks through group's buddy bitmap searching for free
>   * extents. When the free block is found, ext4_trim_extent is called to TRIM
> @@ -5065,7 +5071,7 @@ __acquires(bitlock)
>  static ext4_grpblk_t
>  ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  		   ext4_grpblk_t start, ext4_grpblk_t max,
> -		   ext4_grpblk_t minblocks)
> +		   ext4_grpblk_t minblocks, bool secure)
>  {
>  	void *bitmap;
>  	ext4_grpblk_t next, count = 0, free_count = 0;
> @@ -5098,7 +5104,8 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  
>  		if ((next - start) >= minblocks) {
>  			ret = ext4_trim_extent(sb, start,
> -					       next - start, group, &e4b);
> +					       next - start, group, &e4b,
> +					       secure);
>  			if (ret && ret != -EOPNOTSUPP)
>  				break;
>  			ret = 0;
> @@ -5140,6 +5147,7 @@ out:
>   * ext4_trim_fs() -- trim ioctl handle function
>   * @sb:			superblock for filesystem
>   * @range:		fstrim_range structure
> + * @secure:		false for standard discard, true for secure discard
>   *
>   * start:	First Byte to trim
>   * len:		number of Bytes to trim from start
> @@ -5148,7 +5156,8 @@ out:
>   * start to start+len. For each such a group ext4_trim_all_free function
>   * is invoked to trim all free space.
>   */
> -int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> +int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range,
> +			bool secure)
>  {
>  	struct ext4_group_info *grp;
>  	ext4_group_t group, first_group, last_group;
> @@ -5204,7 +5213,7 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
>  
>  		if (grp->bb_free >= minlen) {
>  			cnt = ext4_trim_all_free(sb, group, first_cluster,
> -						end, minlen);
> +						end, minlen, secure);
>  			if (cnt < 0) {
>  				ret = cnt;
>  				break;
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index ca1a11b..efbd8f8 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -156,6 +156,7 @@ struct inodes_stat_t {
>  #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
>  #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
>  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
> +#define SFITRIM	_IOWR('X', 122, struct fstrim_range)	/* Secure trim */
>  
>  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
>  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
> 

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Thu, Jun 12, 2014 at 7:36 PM, Eric Sandeen <sandeen@redhat.com> wrote:

> IOWS: your commit log says what, but not why.

Will upload a version with some more "why".


> You're adding a new high-level IOCTL, so let's get a bit more
> visibility than just linux-ext4; linux-fsdevel cc'd.

Wasn't too sure about that one, because I didn't feel like committing
to other FSes for now even if they do have FITRIM.
Will remember to include them.

>> +             if (secure_trim && !blk_queue_secdiscard(q))
>> +                     return -EOPNOTSUPP;
>> +
>> +             if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
>> +                            EXT4_FEATURE_RO_COMPAT_BIGALLOC)) {
>> +                     ext4_msg(sb, KERN_ERR,
>> +                              "FITRIM not supported with bigalloc");
>> +                     return -EOPNOTSUPP;
>> +             }
>> +
>
> This last conditional is unrelated to the patch; if BIGALLOC has another
> incomplete part of its implementation, please send it as a standalone patch,
> not buried in this one.

Yup. My bad.

--

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Eric Sandeen]

On 6/12/14, 10:02 PM, JP Abgrall wrote:
> On Thu, Jun 12, 2014 at 7:36 PM, Eric Sandeen <sandeen@redhat.com> wrote:
> 
>> IOWS: your commit log says what, but not why.
> 
> Will upload a version with some more "why".
> 
> 
>> You're adding a new high-level IOCTL, so let's get a bit more
>> visibility than just linux-ext4; linux-fsdevel cc'd.
> 
> Wasn't too sure about that one, because I didn't feel like committing
> to other FSes for now even if they do have FITRIM.
> Will remember to include them.

You don't need to implement it for other filesystems, but whether or
not this should be a new fs-level ioctl is something worth airing
out on linux-fsdevel.

Thanks,
-Eric

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Thu, Jun 12, 2014 at 8:12 PM, Eric Sandeen <sandeen@redhat.com> wrote:
> On 6/12/14, 10:02 PM, JP Abgrall wrote:
>> On Thu, Jun 12, 2014 at 7:36 PM, Eric Sandeen <sandeen@redhat.com> wrote:
>>> You're adding a new high-level IOCTL, so let's get a bit more
>>> visibility than just linux-ext4; linux-fsdevel cc'd.
>>
>> Wasn't too sure about that one, because I didn't feel like committing
>> to other FSes for now even if they do have FITRIM.
>> Will remember to include them.
>
> You don't need to implement it for other filesystems, but whether or
> not this should be a new fs-level ioctl is something worth airing
> out on linux-fsdevel.

 FITRIM/SFITRIM
vs
 FITRIM/EXT4_IOC_SFITRIM ?

I don't mind either way.

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Eric Sandeen]

On 6/12/14, 10:19 PM, JP Abgrall wrote:
> On Thu, Jun 12, 2014 at 8:12 PM, Eric Sandeen <sandeen@redhat.com> wrote:
>> On 6/12/14, 10:02 PM, JP Abgrall wrote:
>>> On Thu, Jun 12, 2014 at 7:36 PM, Eric Sandeen <sandeen@redhat.com> wrote:
>>>> You're adding a new high-level IOCTL, so let's get a bit more
>>>> visibility than just linux-ext4; linux-fsdevel cc'd.
>>>
>>> Wasn't too sure about that one, because I didn't feel like committing
>>> to other FSes for now even if they do have FITRIM.
>>> Will remember to include them.
>>
>> You don't need to implement it for other filesystems, but whether or
>> not this should be a new fs-level ioctl is something worth airing
>> out on linux-fsdevel.
> 
>  FITRIM/SFITRIM
> vs
>  FITRIM/EXT4_IOC_SFITRIM ?
> 
> I don't mind either way.

Well, no, I'm not talking about making it another ext4-only implementation
- I'm talking about whether or not it's a good idea at all;
see for example Dave's reply.

If it's deemed a good idea, then yes, define SFITRIM, and implement
it for ext4, and let others follow.

If it's not a good idea then... well, perhaps don't do it at all.

This gets back to the "why" - what do you want to accomplish?
When would a user use this?  What if they have mounted -o discard,
or already issued an FITRIM?

What's the ultimate goal, here?

-Eric

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Thu, Jun 12, 2014 at 8:24 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> - I'm talking about whether or not it's a good idea at all;
> see for example Dave's reply.

I see. I'll reply to in his comments.

> This gets back to the "why" - what do you want to accomplish?
> When would a user use this?
> What if they have mounted -o discard,
> or already issued an FITRIM?

Android. No -o discard. A user should not be able to FITRIM.
The system would do the SFITRIM.

> What's the ultimate goal, here?
It was to make sure that data was not left behind after some steps were taken.
And one of the steps seemed to be to replace the FITRIM used in
Android with something better knowing that their was no -o discard
(see my reply to Dave Chinner).

--

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Dave Chinner]

On Thu, Jun 12, 2014 at 09:36:49PM -0500, Eric Sandeen wrote:
> On 6/12/14, 9:14 PM, JP Abgrall wrote:
> > This provides an interface for issuing secure trims to parallel
> > the existing interface for non-secure trim.
> 
> Which does what?  A bit of digging through git history tells
> me, but an idiot reader like myself doesn't know what a
> "secure trim" is.  ;)  Would be nice to have that info,
> and the reason for elevating it from a block-level ioctl
> to an fs-level ioctl in the commit log.
> 
> IOWS: your commit log says what, but not why.

"Secure discard" is only "secure" if every discard that is issued is
secure. Filesystems optimise away repeated discards for freed
regions, so FITRIM followed by SFITRIM means the regions trimmed by
the initial FITRIM are not securely erased from the underlying
media, and you can't get then to be securely erased until
<hand-wave> stuff happens.

Indeed, mixing -o discard and SFITRIM is a recipe for
confusion and leakage - "but I used secure trim on the device!" -
and so all discards either have to be secure or not.

Further, some filesystems may other copies of stale data in the
discarded ranges (e.g. data from unlinked files in the journal or
even in unused portions of metadata blocks), so you can't really say
that secure TRIM provides any sort of security against leakage at
the filesystem layer...

Hence I think secure discard should be a block layer property and
controlled at that layer rather than giving an incorrect impression
that FITRIM provides protection against unintentional data leakage
from the filesystem....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Dave Chinner]

On Fri, Jun 13, 2014 at 01:15:38PM +1000, Dave Chinner wrote:
> On Thu, Jun 12, 2014 at 09:36:49PM -0500, Eric Sandeen wrote:
> > On 6/12/14, 9:14 PM, JP Abgrall wrote:
> > > This provides an interface for issuing secure trims to parallel
> > > the existing interface for non-secure trim.
> > 
> > Which does what?  A bit of digging through git history tells
> > me, but an idiot reader like myself doesn't know what a
> > "secure trim" is.  ;)  Would be nice to have that info,
> > and the reason for elevating it from a block-level ioctl
> > to an fs-level ioctl in the commit log.
> > 
> > IOWS: your commit log says what, but not why.
> 
> "Secure discard" is only "secure" if every discard that is issued is
> secure. Filesystems optimise away repeated discards for freed
> regions, so FITRIM followed by SFITRIM means the regions trimmed by
> the initial FITRIM are not securely erased from the underlying
> media, and you can't get then to be securely erased until
> <hand-wave> stuff happens.
> 
> Indeed, mixing -o discard and SFITRIM is a recipe for
> confusion and leakage - "but I used secure trim on the device!" -
> and so all discards either have to be secure or not.
> 
> Further, some filesystems may other copies of stale data in the
> discarded ranges (e.g. data from unlinked files in the journal or
> even in unused portions of metadata blocks), so you can't really say
> that secure TRIM provides any sort of security against leakage at
> the filesystem layer...

Oh, and while I think of it secure discard at the filesystem level
isn't even a guarantee that you'll get rid of all stale references
to a sector - if the filesystem has freed and then re-allocated a
block without having gone through a discard cycle on that block,
then the underlying device may have old copies of the block that it
hasn't garbage collected and SFITRIM won't clean those up because it
won't ask to trim in-use blocks....

So, really, secure trim from a filesystem perspective can leak stale
data at multiple layers....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Thu, Jun 12, 2014 at 8:30 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Jun 13, 2014 at 01:15:38PM +1000, Dave Chinner wrote:
>> Indeed, mixing -o discard and SFITRIM is a recipe for
>> confusion and leakage - "but I used secure trim on the device!" -
>> and so all discards either have to be secure or not.

The idea was to keep on not using -o discard. And move from FITRIM to SFITRIM.

> Oh, and while I think of it secure discard at the filesystem level
> isn't even a guarantee that you'll get rid of all stale references
> to a sector - if the filesystem has freed and then re-allocated a
> block without having gone through a discard cycle on that block,
> then the underlying device may have old copies of the block that it
> hasn't garbage collected and SFITRIM won't clean those up because it
> won't ask to trim in-use blocks....

Arg. So, if understand this correctly, if the eMMC chip won't get a
secure discard/trim of a block that gets reassigned to the FS, then
data duplicates within the eMMC related to that block are not cleared,
and the next SFITRIM won't even reach that block or the duplicates as
the FS says they are in use.


> So, really, secure trim from a filesystem perspective can leak stale
> data at multiple layers....

.. back to the drawing board to evaluate how much leakage we can live
with or maybe go down a path of fibmap + some secure form of erase
(eMMC level secure trim).

Thanks for now.
--

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Dave Chinner]

On Thu, Jun 12, 2014 at 09:37:58PM -0700, JP Abgrall wrote:
> On Thu, Jun 12, 2014 at 8:30 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Fri, Jun 13, 2014 at 01:15:38PM +1000, Dave Chinner wrote:
> >> Indeed, mixing -o discard and SFITRIM is a recipe for
> >> confusion and leakage - "but I used secure trim on the device!" -
> >> and so all discards either have to be secure or not.
> 
> The idea was to keep on not using -o discard. And move from FITRIM to SFITRIM.

IOWs, you want either normal discard or secure discard, and not a
mix of both. IOWs, you don't need SFITRIM, you need a "block device
does secure discard only" configuration flag...

> > Oh, and while I think of it secure discard at the filesystem level
> > isn't even a guarantee that you'll get rid of all stale references
> > to a sector - if the filesystem has freed and then re-allocated a
> > block without having gone through a discard cycle on that block,
> > then the underlying device may have old copies of the block that it
> > hasn't garbage collected and SFITRIM won't clean those up because it
> > won't ask to trim in-use blocks....
> 
> Arg. So, if understand this correctly, if the eMMC chip won't get a
> secure discard/trim of a block that gets reassigned to the FS, then
						      ^^ within

> data duplicates within the eMMC related to that block are not cleared,
> and the next SFITRIM won't even reach that block or the duplicates as
> the FS says they are in use.

Pretty much.

And even using -o discard is no guarantee that the filesystem will
issue a discard between freeing and re-using a block e.g.  XFS
explicitly avoids issuing discards for blocks it re-uses immediately
because they are always considered "in-use" from a transactional
POV. Hence there is no place where the block is considered free, and
hence there isn't a point in time where a discard can be safely
issued on that block.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Fri, Jun 13, 2014 at 03:07:03PM +1000, Dave Chinner wrote:
> > 
> > Arg. So, if understand this correctly, if the eMMC chip won't get a
> > secure discard/trim of a block that gets reassigned to the FS, then
> 						      ^^ within
> 
> > data duplicates within the eMMC related to that block are not cleared,
> > and the next SFITRIM won't even reach that block or the duplicates as
> > the FS says they are in use.
> 
> Pretty much.

If you really want this to work, and be 100% secure, you really need
to do the secure discard at the file system layer.  The file system
could make sure that every single block gets a secure discard before
it gets reused.  Now as long as you can be sure the flash controller
won't discard certain trims because the range is too small or the
flash controller is too busy (DISCARD is "advisory" which means the
flash controller is free to drop any discard request on the floor; you
should check and see if secure discard is a similarly advisory
request.  Yes, that would be broken, but the flash controller
developers leaned on the standards bodies to make discard_zeros_data
to be similarly useless.)

One question --- do you really need to use secure discard on all
blocks, or just on certain critical bits of data?  If so, there may be
other ways of meeting your security requirements.  One of the things
which Michael Halcrow has been implementing for filesystem level
encryption for ChromeOS, so that selecting ext4 files can be encrypted
using per-user keys.  He recently has gotten his proof of concept
working with a global fixed key, so he's pretty far along.  (Although
I'm sure we'll need to do quite a bit of cleanup before it will be
ready for upstream submission.)  Depending on what your security
requirements are, perhaps this might help meet your security
requirements.

Feel free to contact Michael and I at our google.com e-mails and
perhaps schedule a meeting if that would be helpful.

Cheers,

						- Ted

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Fri, Jun 13, 2014 at 10:20:54AM -0400, Theodore Ts'o wrote:
> 
> If you really want this to work, and be 100% secure, you really need
> to do the secure discard at the file system layer.  The file system
> could make sure that every single block gets a secure discard before
> it gets reused.

BTW, one major downside of doing a secure trim after every time that a
block has been released is that it will massively increase the flash
wear, since if you do a secure trim on a single 4k block in 512k erase
block, assuming that secure trim has been implemented properly from a
security perspective, it will need to copy out all of the used portion
of the 512k erase block, and then erase it.

This is one of the reasons why I asked if you really need to worry
about securely discarding all of the blocks on the file system, or
just blocks containing specific really security-sensitive information
(i.e., for Google Wallet, etc.)

If so, you might be better off either doing per-file encryption, or
per-file secure discard.

Cheers,

					- Ted

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Fri, Jun 13, 2014 at 7:31 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Fri, Jun 13, 2014 at 10:20:54AM -0400, Theodore Ts'o wrote:
>>
>> If you really want this to work, and be 100% secure, you really need
>> to do the secure discard at the file system layer.  The file system
>> could make sure that every single block gets a secure discard before
>> it gets reused.
>
> BTW, one major downside of doing a secure trim after every time that a
> block has been released is that it will massively increase the flash
> wear, since if you do a secure trim on a single 4k block in 512k erase
> block, assuming that secure trim has been implemented properly from a
> security perspective, it will need to copy out all of the used portion
> of the 512k erase block, and then erase it.

We did not plan on doing always-on secure-discard or always
secure-trim for those reasons.


> This is one of the reasons why I asked if you really need to worry
> about securely discarding all of the blocks on the file system, or
> just blocks containing specific really security-sensitive information
> (i.e., for Google Wallet, etc.)

Part of the "why" for this SFITRIM patch:
At some point we need to delete a bunch of files and packages and we
want to reduce the volume of recoverable data (file content, file
names, file names within databases or other files,...).
These are not security-critical files.
We understand that not all of the data can be purged, but covering
most of it would be nice.
We currently only care about ext4.
The current expected leftovers from a cleanup would be:
  - data blocks that were modified during the life-time of the files.
This includes directories containing those filenames.
  - file names in top level directories for directories that are not
getting deleted.
  - databases that are not set for deletion which referenced the files
being deleted.


> If so, you might be better off either doing per-file encryption, or
> per-file secure discard.

The per-file secure discard seems to be the way to go, as there are
only a few places in Android where this needs to be turned on.
The  current idletime-fstrim would  switch from FITRIM to SFITRIM to
reduce the leftovers.

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Eric Sandeen]

On 6/13/14, 2:44 PM, JP Abgrall wrote:
> On Fri, Jun 13, 2014 at 7:31 AM, Theodore Ts'o <tytso@mit.edu> wrote:

<snip>

> 
>> If so, you might be better off either doing per-file encryption, or
>> per-file secure discard.
> 
> The per-file secure discard seems to be the way to go, as there are
> only a few places in Android where this needs to be turned on.
> The  current idletime-fstrim would  switch from FITRIM to SFITRIM to
> reduce the leftovers.

Apologies if this is a dumb thing to point out, but...
mmc is the only in-kernel driver (aside from xen) which can
even set the flags necessary to enable secure discard; and then
only if mmc_can_secure_erase_trim() is true, so it depends on
the card, I guess.

I don't know what device you're running on, but figured it might
be worth pointing out that not all hardware even supports this
capability.

-Eric

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Fri, Jun 13, 2014 at 12:57 PM, Eric Sandeen <sandeen@redhat.com> wrote:
> I don't know what device you're running on,

At least Nexus devices that will get updates. :)

> but figured it might
> be worth pointing out that not all hardware even supports this
> capability.

True.
For future devices, the feature will be for certain eMMC devices,
certain kernels, certain FSes only. All stuff that we don't control,
but we can give advice on.

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Fri, Jun 13, 2014 at 12:44:34PM -0700, JP Abgrall wrote:
> The per-file secure discard seems to be the way to go, as there are
> only a few places in Android where this needs to be turned on.
> The  current idletime-fstrim would  switch from FITRIM to SFITRIM to
> reduce the leftovers.

OK, how about this?  The following patch is in the Google data center
kernel, but I never got around to get it upstream (oops, was on my
todo list, but it never happened).

If you want to adopt this for usptream, and add support for
BLKSECDISCARD as well as BLKDISCARD, then you could for each file that
you want to do the per-file secure discard, you would just have to
open the file, call the BLKSECDISCARD ioctl, and then delete the file.

Cheers,

					- Ted

commit 16ff6352b123aa134417793d636f05cd4e240eaa
Author: Theodore Ts'o <tytso@google.com>
Date:   Fri Dec 20 12:48:26 2013 -0500

    ext4: add support for the BLKDISCARD ioctl
    
    The blkdicard ioctl previously only worked on block devices.  Allow
    this ioctl to work on ext4 files.
    
    This commit is intended to be sent upstream.
    
    Google-Bug-Id: 11517631
    Tested: fs smoke test; manual check of the blkdiscard ioctl
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
    Effort: fs/ext4
    Origin-3.3-SHA1: 85ef322daf0da0000759b1fe3a1e1bd3da3b906a
    Change-Id: If031d1b8e796ebcb0afcb8a5259c760441bc6b2e

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b3a0f21..0203d9c 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2695,6 +2695,8 @@ extern int ext4_check_blockref(const char *, unsigned int,
 /* extents.c */
 struct ext4_ext_path;
 struct ext4_extent;
+typedef int (*extent_iterator_t)(struct inode *inode, struct extent_status *es,
+				 unsigned int flags, void *private);
 
 extern int ext4_ext_tree_init(handle_t *handle, struct inode *);
 extern int ext4_ext_writepage_trans_blocks(struct inode *, int);
@@ -2733,6 +2735,9 @@ extern int ext4_find_delalloc_range(struct inode *inode,
 				    ext4_lblk_t lblk_start,
 				    ext4_lblk_t lblk_end);
 extern int ext4_find_delalloc_cluster(struct inode *inode, ext4_lblk_t lblk);
+extern int ext4_extent_iterator(struct inode *inode,
+				ext4_lblk_t block, ext4_lblk_t num,
+				extent_iterator_t callback, void *private);
 extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			__u64 start, __u64 len);
 extern int ext4_ext_precache(struct inode *inode);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 2c84bc0..6118a1d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2118,9 +2118,13 @@ cleanup:
 	return err;
 }
 
-static int ext4_fill_fiemap_extents(struct inode *inode,
-				    ext4_lblk_t block, ext4_lblk_t num,
-				    struct fiemap_extent_info *fieinfo)
+
+typedef int (*extent_iterator_t)(struct inode *inode, struct extent_status *es,
+				 unsigned int flags, void *private);
+
+int ext4_extent_iterator(struct inode *inode,
+			 ext4_lblk_t block, ext4_lblk_t num,
+			 extent_iterator_t callback, void *private)
 {
 	struct ext4_ext_path *path = NULL;
 	struct ext4_extent *ex;
@@ -2129,7 +2133,6 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
 	ext4_lblk_t last = block + num;
 	int exists, depth = 0, err = 0;
 	unsigned int flags = 0;
-	unsigned char blksize_bits = inode->i_sb->s_blocksize_bits;
 
 	while (block < last && block != EXT_MAX_BLOCKS) {
 		num = last - block;
@@ -2253,11 +2256,7 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
 		}
 
 		if (exists) {
-			err = fiemap_fill_next_extent(fieinfo,
-				(__u64)es.es_lblk << blksize_bits,
-				(__u64)es.es_pblk << blksize_bits,
-				(__u64)es.es_len << blksize_bits,
-				flags);
+			err = callback(inode, &es, flags, private);
 			if (err < 0)
 				break;
 			if (err == 1) {
@@ -2277,6 +2276,27 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
 	return err;
 }
 
+static int call_fill_fiemap(struct inode *inode, struct extent_status *es,
+			    unsigned int flags, void *private)
+{
+	unsigned char blksize_bits = inode->i_sb->s_blocksize_bits;
+
+	return fiemap_fill_next_extent(private,
+				       (__u64)es->es_lblk << blksize_bits,
+				       (__u64)es->es_pblk << blksize_bits,
+				       (__u64)es->es_len << blksize_bits,
+				       flags);
+}
+
+static int ext4_fill_fiemap_extents(struct inode *inode,
+				    ext4_lblk_t block, ext4_lblk_t num,
+				    struct fiemap_extent_info *fieinfo)
+{
+	return ext4_extent_iterator(inode, block, num,
+				    call_fill_fiemap, fieinfo);
+}
+
+
 /*
  * ext4_ext_put_gap_in_cache:
  * calculate boundaries of the gap that the requested block fits into
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 5498f75..da6eac0 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -214,6 +214,132 @@ swap_boot_out:
 	return err;
 }
 
+static int discard_callback(struct inode *inode, struct extent_status *es,
+			    unsigned int flags, void *private)
+{
+	struct ext4_map_blocks *map = private;
+	ext4_lblk_t es_lblk = es->es_lblk;
+	ext4_lblk_t es_len = es->es_len;
+	ext4_fsblk_t es_pblk = es->es_pblk;
+
+	if (flags & (FIEMAP_EXTENT_UNKNOWN |
+		     FIEMAP_EXTENT_ENCODED |
+		     FIEMAP_EXTENT_DATA_ENCRYPTED |
+		     FIEMAP_EXTENT_DELALLOC |
+		     FIEMAP_EXTENT_DATA_TAIL |
+		     FIEMAP_EXTENT_DATA_INLINE |
+		     FIEMAP_EXTENT_NOT_ALIGNED |
+		     FIEMAP_EXTENT_SHARED))
+		return 0;
+
+	if (es_lblk < map->m_lblk) {
+		ext4_lblk_t d = map->m_lblk - es_lblk;
+		if (d > es_len)
+			return 0;
+		es_lblk += d;
+		es_pblk += d;
+		es_len -= d;
+	}
+
+	if (es_lblk + es_len > map->m_lblk + map->m_len)
+		es_len -= es_lblk + es_len - (map->m_lblk + map->m_len);
+#ifdef BLKDISCARD_DEBUG
+	ext4_msg(inode->i_sb, KERN_NOTICE, "discard: %llu len %lu",
+		 (unsigned long long) es_pblk, (unsigned long) es_len);
+	return 0;
+#else
+	return sb_issue_discard(inode->i_sb, es_pblk, es_len, GFP_KERNEL, 0);
+#endif
+}
+
+static int blkdiscard_inode(struct inode *inode, u64 start_offset, u64 len)
+{
+	struct super_block *sb = inode->i_sb;
+	struct ext4_map_blocks map;
+	unsigned int num;
+
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;
+
+	if (!blk_queue_discard(bdev_get_queue(sb->s_bdev)))
+		return -EOPNOTSUPP;
+
+	if (!bdev_discard_zeroes_data(sb->s_bdev) && !capable(CAP_SYS_ADMIN))
+		return -EOPNOTSUPP;
+
+	num = start_offset & (sb->s_blocksize - 1);
+	if (num) {
+		num = sb->s_blocksize - num;
+		start_offset += num;
+		len = (len > num) ? len - num : 0;
+	}
+	if (len == 0)
+		return 0;
+	if (start_offset > sb->s_maxbytes)
+		return -EFBIG;
+	if (len > sb->s_maxbytes || (sb->s_maxbytes - len) < start_offset)
+		len = sb->s_maxbytes - start_offset;
+
+	map.m_lblk = start_offset >> sb->s_blocksize_bits;
+	map.m_len = len >> sb->s_blocksize_bits;
+
+#ifdef BLKDISCARD_DEBUG
+	ext4_msg(sb, KERN_NOTICE, "blkdiscard range: %lu len %lu",
+		 (unsigned long) map.m_lblk, (unsigned long) map.m_len);
+#endif
+
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
+		return ext4_extent_iterator(inode, map.m_lblk, map.m_len,
+					    discard_callback, &map);
+
+	num = map.m_len;
+	while (num) {
+		int ret = ext4_map_blocks(NULL, inode, &map, 0);
+
+		if (ret < 0)
+			return ret;
+
+		if (ret == 0) {
+#ifdef BLKDISCARD_DEBUG
+			ext4_msg(sb, KERN_NOTICE,
+				 "skip: lblk %lu len %lu ret %lu num %lu",
+				 (unsigned long) map.m_lblk,
+				 (unsigned long) map.m_len,
+				 (unsigned long) ret,
+				 (unsigned long) num);
+#endif
+			map.m_lblk++;
+			num--;
+			continue;
+		}
+#ifdef BLKDISCARD_DEBUG
+		ext4_msg(sb, KERN_NOTICE,
+			 "walk: lblk %lu pblk %llu len %lu ret %lu num %lu",
+			 (unsigned long) map.m_lblk,
+			 (unsigned long long) map.m_pblk,
+			 (unsigned long) map.m_len,
+			 (unsigned long) ret,
+			 (unsigned long) num);
+#endif
+		if (ret > num)
+			ret = num;
+		map.m_lblk += ret;
+		num -= ret;
+		map.m_len = num;
+
+#ifdef BLKDISCARD_DEBUG
+		ext4_msg(sb, KERN_NOTICE, "discard: %llu len %lu",
+			 (unsigned long long) map.m_pblk, (unsigned long) ret);
+#else
+		ret = sb_issue_discard(sb, map.m_pblk, ret,
+				       GFP_KERNEL, 0);
+		if (ret)
+			return ret;
+#endif
+	}
+	return 0;
+}
+
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -627,6 +753,18 @@ resizefs_out:
 	case EXT4_IOC_PRECACHE_EXTENTS:
 		return ext4_ext_precache(inode);
 
+	case BLKDISCARD: {
+		uint64_t range[2];
+
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+
+		if (copy_from_user(range, (void __user *)arg, sizeof(range)))
+			return -EFAULT;
+
+		return blkdiscard_inode(file_inode(filp), range[0], range[1]);
+	}
+
 	default:
 		return -ENOTTY;
 	}
@@ -691,6 +829,7 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case FITRIM:
 	case EXT4_IOC_RESIZE_FS:
 	case EXT4_IOC_PRECACHE_EXTENTS:
+	case BLKDISCARD:
 		break;
 	default:
 		return -ENOIOCTLCMD;

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Fri, Jun 13, 2014 at 4:41 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> If you want to adopt this for usptream, and add support for
> BLKSECDISCARD as well as BLKDISCARD, then you could for each file that
> you want to do the per-file secure discard, you would just have to
> open the file, call the BLKSECDISCARD ioctl, and then delete the file.

That would work also. Thanks.

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Dave Chinner]

On Fri, Jun 13, 2014 at 07:41:34PM -0400, Theodore Ts'o wrote:
> On Fri, Jun 13, 2014 at 12:44:34PM -0700, JP Abgrall wrote:
> > The per-file secure discard seems to be the way to go, as there are
> > only a few places in Android where this needs to be turned on.
> > The  current idletime-fstrim would  switch from FITRIM to SFITRIM to
> > reduce the leftovers.
> 
> OK, how about this?  The following patch is in the Google data center
> kernel, but I never got around to get it upstream (oops, was on my
> todo list, but it never happened).
> 
> If you want to adopt this for usptream, and add support for
> BLKSECDISCARD as well as BLKDISCARD, then you could for each file that
> you want to do the per-file secure discard, you would just have to
> open the file, call the BLKSECDISCARD ioctl, and then delete the file.
> 
> Cheers,
> 
> 					- Ted
> 
> commit 16ff6352b123aa134417793d636f05cd4e240eaa
> Author: Theodore Ts'o <tytso@google.com>
> Date:   Fri Dec 20 12:48:26 2013 -0500
> 
>     ext4: add support for the BLKDISCARD ioctl
>     
>     The blkdicard ioctl previously only worked on block devices.  Allow
>     this ioctl to work on ext4 files.
>     
>     This commit is intended to be sent upstream.

Not in that form - it's an ugly API hack.

This is really just an extension of hole punching (if the blocks in
the file are being removed) or zeroing (if the blocks are being
retained by the file). Either way, fallocate() is the interface
used for per-file block level manipulations, and either of these
operations could issue a discard (secure or not) during the
punch/zero operation....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Tue, Jun 17, 2014 at 12:49:53PM +1000, Dave Chinner wrote:
> >     The blkdicard ioctl previously only worked on block devices.  Allow
> >     this ioctl to work on ext4 files.
> >     
> >     This commit is intended to be sent upstream.
> 
> Not in that form - it's an ugly API hack.

The whole *point* was that it's an API hack.  We had a userspace
program that wanted to use the same code regardless of whether it was
accessing a block device or a file, and we didn't want to spin up a
KVM just to simulate a block device (think the whole countainers
vs. virtualization argument).

> This is really just an extension of hole punching (if the blocks in
> the file are being removed) or zeroing (if the blocks are being
> retained by the file). Either way, fallocate() is the interface
> used for per-file block level manipulations, and either of these
> operations could issue a discard (secure or not) during the
> punch/zero operation....

Except that discard is not an exact equivalent of zeroing, since even
in the case of discard zeros data, the flash device is free to
disregard the discard request if it feels like it.  But if you have a
userspace application which is trying to manage the flash to optimize
write endurance, that's different from either hole punching or
zeroing.

Secure discard maps a bit more like a zero'ing or hole punching, since
hopefully the standard for secure discard was as incompetently
authored as the discard/trim specification.  So that might be a
different approach if this is an approach that we want to get adoption
across multiple file systems.

						- Ted

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Lukáš Czerner]

On Tue, 17 Jun 2014, Dave Chinner wrote:

> Date: Tue, 17 Jun 2014 12:49:53 +1000
> From: Dave Chinner <david@fromorbit.com>
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: JP Abgrall <jpa@google.com>, Eric Sandeen <sandeen@redhat.com>,
>     linux-ext4@vger.kernel.org, Geremy Condra <gcondra@google.com>,
>     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH] ext4: Add support for SFITRIM,
>     an ioctl for secure FITRIM.
> 
> On Fri, Jun 13, 2014 at 07:41:34PM -0400, Theodore Ts'o wrote:
> > On Fri, Jun 13, 2014 at 12:44:34PM -0700, JP Abgrall wrote:
> > > The per-file secure discard seems to be the way to go, as there are
> > > only a few places in Android where this needs to be turned on.
> > > The  current idletime-fstrim would  switch from FITRIM to SFITRIM to
> > > reduce the leftovers.
> > 
> > OK, how about this?  The following patch is in the Google data center
> > kernel, but I never got around to get it upstream (oops, was on my
> > todo list, but it never happened).
> > 
> > If you want to adopt this for usptream, and add support for
> > BLKSECDISCARD as well as BLKDISCARD, then you could for each file that
> > you want to do the per-file secure discard, you would just have to
> > open the file, call the BLKSECDISCARD ioctl, and then delete the file.
> > 
> > Cheers,
> > 
> > 					- Ted
> > 
> > commit 16ff6352b123aa134417793d636f05cd4e240eaa
> > Author: Theodore Ts'o <tytso@google.com>
> > Date:   Fri Dec 20 12:48:26 2013 -0500
> > 
> >     ext4: add support for the BLKDISCARD ioctl
> >     
> >     The blkdicard ioctl previously only worked on block devices.  Allow
> >     this ioctl to work on ext4 files.
> >     
> >     This commit is intended to be sent upstream.
> 
> Not in that form - it's an ugly API hack.
> 
> This is really just an extension of hole punching (if the blocks in
> the file are being removed) or zeroing (if the blocks are being
> retained by the file). Either way, fallocate() is the interface
> used for per-file block level manipulations, and either of these
> operations could issue a discard (secure or not) during the
> punch/zero operation....

I definitely agree with Dave here it is an ugly API hack. Fallocate
seems much more suitable for this.

New flag FALLOC_FL_ISSUE_DISCARD which would work with
FALLOC_FL_PUNCH_HOLE, FALLOC_FL_ZERO_RANGE and possibly
FALLOC_FL_COLLAPSE_RANGE might actually be useful.

-Lukas

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Tue, Jun 17, 2014 at 01:55:26PM +0200, Lukáš Czerner wrote:
> 
> I definitely agree with Dave here it is an ugly API hack. Fallocate
> seems much more suitable for this.
> 
> New flag FALLOC_FL_ISSUE_DISCARD which would work with
> FALLOC_FL_PUNCH_HOLE, FALLOC_FL_ZERO_RANGE and possibly
> FALLOC_FL_COLLAPSE_RANGE might actually be useful.

I agree it would be useful to have an FL_ISSUE_DISCARD (and while
we're at it, FL_ISSUE_SECDISCARD) as an fallocate flag.  That doesn't
obviate the usefulness of a BLKDISCARD ioctl for ext4 files, though.

Something else that might be useful, and perhaps more appropriate for
the Android use case, is to add a SECDISCARD flag to the unlinkat(2)
system call.  That way, people who want to do a "discard and then
unlink" don't have to be forced to do an open(2), fallocate(2),
close(2), and only *then* the unlink(2) system call.

Cheers,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2345 bytes --]

On Tue, 17 Jun 2014, Theodore Ts'o wrote:

> Date: Tue, 17 Jun 2014 08:46:29 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Dave Chinner <david@fromorbit.com>, JP Abgrall <jpa@google.com>,
>     Eric Sandeen <sandeen@redhat.com>, linux-ext4@vger.kernel.org,
>     Geremy Condra <gcondra@google.com>,
>     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH] ext4: Add support for SFITRIM,
>     an ioctl for secure FITRIM.
> 
> On Tue, Jun 17, 2014 at 01:55:26PM +0200, Lukáš Czerner wrote:
> > 
> > I definitely agree with Dave here it is an ugly API hack. Fallocate
> > seems much more suitable for this.
> > 
> > New flag FALLOC_FL_ISSUE_DISCARD which would work with
> > FALLOC_FL_PUNCH_HOLE, FALLOC_FL_ZERO_RANGE and possibly
> > FALLOC_FL_COLLAPSE_RANGE might actually be useful.
> 
> I agree it would be useful to have an FL_ISSUE_DISCARD (and while
> we're at it, FL_ISSUE_SECDISCARD) as an fallocate flag.  That doesn't
> obviate the usefulness of a BLKDISCARD ioctl for ext4 files, though.
> 
> Something else that might be useful, and perhaps more appropriate for
> the Android use case, is to add a SECDISCARD flag to the unlinkat(2)
> system call.  That way, people who want to do a "discard and then
> unlink" don't have to be forced to do an open(2), fallocate(2),
> close(2), and only *then* the unlink(2) system call.

What is the difference between -o discard mount option ? I guess
that this way you can do it selectively on certain files, but I
wonder how useful it is going to be anyway ?

Nevertheless, I think that there is a conclusion that there is no
"security" to be had with file system and SECDISCARD. And no secure
erase with this type of interface would be "secure" enough.

If they are ok with only best effort, then we can have FISTRIM ioctl
which would use the same internal file system functionality as
FITRIM but we would add a flag to be able to call sb_issue_discard()
with BLKDEV_DISCARD_SECURE flag, disable the optimization to skip
already discarded groups and call sync on the file system before we
start doing any actuall work. I wish I added flags to the FITRIM
ioctl when I created it...

If we do this though we should not add word "security" anywhere for
the use to see :)

-Lukas

> 
> Cheers,
> 
> 						- Ted
> 

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Tue, Jun 17, 2014 at 03:00:40PM +0200, Lukáš Czerner wrote:
> 
> What is the difference between -o discard mount option ? I guess
> that this way you can do it selectively on certain files, but I
> wonder how useful it is going to be anyway ?

Well, it will reduce the amount of flash wear, since a SECDISCARD
requires an immediate copy of the remaining data in the erase block
followed by a erase.  This increases write magnification.

> Nevertheless, I think that there is a conclusion that there is no
> "security" to be had with file system and SECDISCARD. And no secure
> erase with this type of interface would be "secure" enough.

There's an assumption here that the eMMC SECDISCARD functionality is
more competently spec'ed out compared to the ATA/SCSI interface.  I'm
not sure whether or not that's true, but perhaps JP and Geremy can
confirm that.  And even if it isn't guaranteed by the MMC spec, a
mobile handset manufacturer is buying in sufficently large quantities
that they can probably negotiate with their suppliers and demand a
custom firmware which doesn't drop the discard command if the flash
device doesn't feel like it.  

(There's nothing new about this, by the way.  Very large buyers of
hard drives such as EMC, Amazon, Facebook, etc. do their own
performance and quality control testing, and then have demanded custom
firmware if necessary for a very long time.)

So at least in some specific use cases, it should be possible to make
this to be secure.  And the reason why to call it secure is SECDISCARD
is the term used in the spec.  And if the spec doesn't guarantee it,
we can mock the spec.  :-)

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Tue, Jun 17, 2014 at 6:54 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> There's an assumption here that the eMMC SECDISCARD functionality is
> more competently spec'ed out compared to the ATA/SCSI interface.  I'm
> not sure whether or not that's true, but perhaps JP and Geremy can
> confirm that.
We only care about the eMMC spec. In the eMMC spec there is no room
for ignoring the commands. If the card declares it can do a secure
erase/trim it must do it as per the spec.
  JEDEC Standard No. 84-A441
  7.6.9 Secure Erase
  7.6.10 Secure Trim

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1826 bytes --]

On Tue, 17 Jun 2014, JP Abgrall wrote:

> Date: Tue, 17 Jun 2014 10:53:21 -0700
> From: JP Abgrall <jpa@google.com>
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: Lukáš Czerner <lczerner@redhat.com>, Dave Chinner <david@fromorbit.com>,
>     Eric Sandeen <sandeen@redhat.com>, linux-ext4@vger.kernel.org,
>     Geremy Condra <gcondra@google.com>,
>     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH] ext4: Add support for SFITRIM,
>     an ioctl for secure FITRIM.
> 
> On Tue, Jun 17, 2014 at 6:54 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > There's an assumption here that the eMMC SECDISCARD functionality is
> > more competently spec'ed out compared to the ATA/SCSI interface.  I'm
> > not sure whether or not that's true, but perhaps JP and Geremy can
> > confirm that.
> We only care about the eMMC spec. In the eMMC spec there is no room
> for ignoring the commands. If the card declares it can do a secure
> erase/trim it must do it as per the spec.

Except when it does not. Looking at the mmc driver I can see that we
have already some devices where secure trim is broken.

/*
 * On these Samsung MoviNAND parts, performing secure erase or
 * secure trim can result in unrecoverable corruption due to a
 * firmware bug.
 */

And I have no illusion that those are the only ones that does not
work. This hardware can not be trusted and this must not be
advertised as a security feature.

Btw, AFACIT with this backlisted hardware the user will not even
notice that secure discard did not went through and the driver will
simply use normal discard, which is a bug if you ask me.

Let's call is deep discard, or whatever but avoid the security word
at least when file system comes into play.

-Lukas

>   JEDEC Standard No. 84-A441
>   7.6.9 Secure Erase
>   7.6.10 Secure Trim

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Wed, Jun 18, 2014 at 2:33 AM, Lukáš Czerner <lczerner@redhat.com> wrote:
> Except when it does not. Looking at the mmc driver I can see that we
> have already some devices where secure trim is broken.


And that is why we don't just blindly use random eMMC devices.

> Let's call is deep discard, or whatever but avoid the security word
> at least when file system comes into play.

Best effort discard :)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1212 bytes --]

On Wed, 18 Jun 2014, JP Abgrall wrote:

> Date: Wed, 18 Jun 2014 14:51:42 -0700
> From: JP Abgrall <jpa@google.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Theodore Ts'o <tytso@mit.edu>, Dave Chinner <david@fromorbit.com>,
>     Eric Sandeen <sandeen@redhat.com>, linux-ext4@vger.kernel.org,
>     Geremy Condra <gcondra@google.com>,
>     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH] ext4: Add support for SFITRIM,
>     an ioctl for secure FITRIM.
> 
> On Wed, Jun 18, 2014 at 2:33 AM, Lukáš Czerner <lczerner@redhat.com> wrote:
> > Except when it does not. Looking at the mmc driver I can see that we
> > have already some devices where secure trim is broken.
> 
> And that is why we don't just blindly use random eMMC devices.

'we' as in ... ? Remember, you're not the only one using this code.

> 
> > Let's call is deep discard, or whatever but avoid the security word
> > at least when file system comes into play.
> 
> Best effort discard :)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Theodore Ts'o]

On Wed, Jun 18, 2014 at 11:33:47AM +0200, Lukáš Czerner wrote:
> Except when it does not. Looking at the mmc driver I can see that we
> have already some devices where secure trim is broken.
> 
> /*
>  * On these Samsung MoviNAND parts, performing secure erase or
>  * secure trim can result in unrecoverable corruption due to a
>  * firmware bug.
>  */

The bug IMHO is that the eMMC driver is falling back to discard,
instead of returning EOPNOTSUPP.  The question of whether we should
fall back to discard or not should be made at a level much higher up
than the MMC device driver....

> And I have no illusion that those are the only ones that does not
> work. This hardware can not be trusted and this must not be
> advertised as a security feature.

There's always crappy hardware out there.  If that's true, should then
not call ATA Secure Erase by that term because somewhere out there,
there will be an incompetently implemented SSD that doesn't do the
right thing with ATA Secure Erase?  I just don't think that's
particularly useful.  If the command is called "secure erase" or
"secure discard" in the specification, then that's what we should use,
just to avoid confusion if nothing else.

Now, if the spec explicitly disclaims correct behavior, in the case of
discard and discard zeros data, that's a different matter.  But I
don't think that is what's going on here.  The MMC specification makes
certain guarantees; there is no "the drive is allowed to disregard the
discard command if it's too busy to attend to it mealy-mouthed
language", as there is in the ATA discard specification.

The fact that there happens to be buggy hardware out there, is just
the natural state of affairs.  But that's what black lists are for.

    	    	     	       	   - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Dave Chinner]

On Wed, Jun 18, 2014 at 06:06:01PM -0400, Theodore Ts'o wrote:
> On Wed, Jun 18, 2014 at 11:33:47AM +0200, Lukáš Czerner wrote:
> > And I have no illusion that those are the only ones that does not
> > work. This hardware can not be trusted and this must not be
> > advertised as a security feature.
> 
> There's always crappy hardware out there.  If that's true, should then
> not call ATA Secure Erase by that term because somewhere out there,
> there will be an incompetently implemented SSD that doesn't do the
> right thing with ATA Secure Erase?  I just don't think that's
> particularly useful.  If the command is called "secure erase" or
> "secure discard" in the specification, then that's what we should use,
> just to avoid confusion if nothing else.

That's just a steaming pile of rhetoric. If that was true, then we
wouldn't be calling our operations BLKDISCARD or "discard", would
we?  It would be called "TRIM" or "WRITE_SAME" because that's what
the device layer standards call the operations.

Sure, we have a "FITRIM" ioctl, but we acknowledged early on that it
was badly named because different protocols use different names.
That's why we started to use "discard" instead - it's a protocol and
device neutral term that describes the intent of the operation - to
-discard blocks-.

IOWs, I think that Lukas is right on the money here - we should not
imply something is secure when it is not, nor should we name high
level interfaces based on the standardise name on the low level
primitive some class of device or protocol uses. 

Rather, we should describe it for what it is: it is a command
to *scrub the data* from a range of blocks. i.e. it's not a
discard operation at all - it's a "scrub" operation that we are
asking the device to perform.

And further, scrubbing has a specific meaning in the security
environment - it doesn't imply security - it just means there is a
mechanism for physically removing data from it's known locations.
Security comes from what you do with the scrubbing mechanism at
higher layers.

Scrubbing is something people already understand and it's clear
that it's a data manipulation operation and not some magic "secure"
operation. And by calling it "scrub" we get away from the idea that
it only works on specific hardware - hardware acceleration is good,
but there's no reason why we should design the functionality to only
be useful on systems with hardware scrubbing capability...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3138 bytes --]

On Thu, 19 Jun 2014, Dave Chinner wrote:

> Date: Thu, 19 Jun 2014 10:36:57 +1000
> From: Dave Chinner <david@fromorbit.com>
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: Lukáš Czerner <lczerner@redhat.com>, JP Abgrall <jpa@google.com>,
>     Eric Sandeen <sandeen@redhat.com>, linux-ext4@vger.kernel.org,
>     Geremy Condra <gcondra@google.com>,
>     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH] ext4: Add support for SFITRIM,
>     an ioctl for secure FITRIM.
> 
> On Wed, Jun 18, 2014 at 06:06:01PM -0400, Theodore Ts'o wrote:
> > On Wed, Jun 18, 2014 at 11:33:47AM +0200, Lukáš Czerner wrote:
> > > And I have no illusion that those are the only ones that does not
> > > work. This hardware can not be trusted and this must not be
> > > advertised as a security feature.
> > 
> > There's always crappy hardware out there.  If that's true, should then
> > not call ATA Secure Erase by that term because somewhere out there,
> > there will be an incompetently implemented SSD that doesn't do the
> > right thing with ATA Secure Erase?  I just don't think that's
> > particularly useful.  If the command is called "secure erase" or
> > "secure discard" in the specification, then that's what we should use,
> > just to avoid confusion if nothing else.
> 
> That's just a steaming pile of rhetoric. If that was true, then we
> wouldn't be calling our operations BLKDISCARD or "discard", would
> we?  It would be called "TRIM" or "WRITE_SAME" because that's what
> the device layer standards call the operations.
> 
> Sure, we have a "FITRIM" ioctl, but we acknowledged early on that it
> was badly named because different protocols use different names.
> That's why we started to use "discard" instead - it's a protocol and
> device neutral term that describes the intent of the operation - to
> -discard blocks-.
> 
> IOWs, I think that Lukas is right on the money here - we should not
> imply something is secure when it is not, nor should we name high
> level interfaces based on the standardise name on the low level
> primitive some class of device or protocol uses. 
> 
> Rather, we should describe it for what it is: it is a command
> to *scrub the data* from a range of blocks. i.e. it's not a
> discard operation at all - it's a "scrub" operation that we are
> asking the device to perform.
> 
> And further, scrubbing has a specific meaning in the security
> environment - it doesn't imply security - it just means there is a
> mechanism for physically removing data from it's known locations.
> Security comes from what you do with the scrubbing mechanism at
> higher layers.
> 
> Scrubbing is something people already understand and it's clear
> that it's a data manipulation operation and not some magic "secure"
> operation. And by calling it "scrub" we get away from the idea that
> it only works on specific hardware - hardware acceleration is good,
> but there's no reason why we should design the functionality to only
> be useful on systems with hardware scrubbing capability...

+1 for the "scrub" operation, it makes perfect sense to me.

-Lukas

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Martin K. Petersen]

>>>>> "Lukáš" == Lukáš Czerner <lczerner@redhat.com> writes:

>> Scrubbing is something people already understand and it's clear that
>> it's a data manipulation operation and not some magic "secure"
>> operation. And by calling it "scrub" we get away from the idea that
>> it only works on specific hardware - hardware acceleration is good,
>> but there's no reason why we should design the functionality to only
>> be useful on systems with hardware scrubbing capability...

Lukáš> +1 for the "scrub" operation, it makes perfect sense to me.

I'm not sure I agree with the choice of "scrub" to describe this:

	http://en.wikipedia.org/wiki/Data_scrubbing

What about purge or sanitize? That's what the security folks generally
use...

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3801 bytes --]

On Wed, 18 Jun 2014, Theodore Ts'o wrote:

> Date: Wed, 18 Jun 2014 18:06:01 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: JP Abgrall <jpa@google.com>, Dave Chinner <david@fromorbit.com>,
>     Eric Sandeen <sandeen@redhat.com>, linux-ext4@vger.kernel.org,
>     Geremy Condra <gcondra@google.com>,
>     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH] ext4: Add support for SFITRIM,
>     an ioctl for secure FITRIM.
> 
> On Wed, Jun 18, 2014 at 11:33:47AM +0200, Lukáš Czerner wrote:
> > Except when it does not. Looking at the mmc driver I can see that we
> > have already some devices where secure trim is broken.
> > 
> > /*
> >  * On these Samsung MoviNAND parts, performing secure erase or
> >  * secure trim can result in unrecoverable corruption due to a
> >  * firmware bug.
> >  */
> 
> The bug IMHO is that the eMMC driver is falling back to discard,
> instead of returning EOPNOTSUPP.  The question of whether we should
> fall back to discard or not should be made at a level much higher up
> than the MMC device driver....

Yes, that's what I meant and I already sent a patch to return
EOPNOTSUPP.

> 
> > And I have no illusion that those are the only ones that does not
> > work. This hardware can not be trusted and this must not be
> > advertised as a security feature.
> 
> There's always crappy hardware out there.  If that's true, should then
> not call ATA Secure Erase by that term because somewhere out there,
> there will be an incompetently implemented SSD that doesn't do the
> right thing with ATA Secure Erase?  I just don't think that's
> particularly useful.  If the command is called "secure erase" or
> "secure discard" in the specification, then that's what we should use,
> just to avoid confusion if nothing else.
> 
> Now, if the spec explicitly disclaims correct behavior, in the case of
> discard and discard zeros data, that's a different matter.  But I
> don't think that is what's going on here.  The MMC specification makes
> certain guarantees; there is no "the drive is allowed to disregard the
> discard command if it's too busy to attend to it mealy-mouthed
> language", as there is in the ATA discard specification.
> 
> The fact that there happens to be buggy hardware out there, is just
> the natural state of affairs.  But that's what black lists are for.

But there is no "security" in the way we're using it right now.
Moreover the secure trim command is actually split to two commands,
one which can be called multiple times to identify all the block
ranges and second one which will actually do the job. If the write
comes in between than the respective block (or blocks) will not be
processed. I do not think that this can happen as it is implemented
right now, but I might be wrong.

However currently we're not using it to it's full potential since
we're only doing the first step once, but I can imagine doing the
first step for all the ranges we want to discard and then doing the
actual discard. In that case we would actually need some guarantees
that no write can come in between. Also we would probably need some
protection against power failure and so on.

And what about defragmentation ? When the data is moving around we can
no longer tell where the previous version of that particular piece of
data is.

And I am sure there are loads of other problems as well, so
currently there are no guarantees at all and it simple does not have
anything to do with security. That's perfectly fine as long as we do
not staple it as "secure" operation. I think that the confusion here
comes from different point of view of the higher level file system and
the lower level device itself as Dave already pointed out.

-Lukas

> 
>     	    	     	       	   - Ted

Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[JP Abgrall]

On Tue, Jun 17, 2014 at 6:00 AM, Lukáš Czerner <lczerner@redhat.com> wrote:
>
> What is the difference between -o discard mount option ?
Android will not mount -o discard because of the overall penalty.

> I guess
> that this way you can do it selectively on certain files, but I
> wonder how useful it is going to be anyway ?
For certain files only. Useful enough.


> If they are ok with only best effort, then we can have FISTRIM ioctl
> which would use the same internal file system functionality as
> FITRIM but we would add a flag to be able to call sb_issue_discard()
> with BLKDEV_DISCARD_SECURE flag,
I think this was the idea of the original patch.

> disable the optimization to skip
> already discarded groups and call sync on the file system before we
> start doing any actuall work.
That would need adding to the original patch.

> If we do this though we should not add word "security" anywhere for
> the use to see :)
We can live with that.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html