Messages in this thread | | | Subject | Re: [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() | From | Richard Weinberger <> | Date | Thu, 22 Sep 2016 22:17:47 +0200 |
| |
Eric,
On 22.09.2016 21:49, Eric Biggers wrote: > On Thu, Sep 22, 2016 at 08:50:54AM +0200, Richard Weinberger wrote: >> ...otherwise an user can enable encryption for certain files even >> when the filesystem is unable to support it. >> Such a case would be a filesystem created by mkfs.ext4's default >> settings, 1KiB block size. Ext4 supports encyption only when block size >> is equal to PAGE_SIZE. >> But this constraint is only checked when the encryption feature flag >> is set. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> fs/ext4/ioctl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c >> index 1bb7df5..9e9a73e 100644 >> --- a/fs/ext4/ioctl.c >> +++ b/fs/ext4/ioctl.c >> @@ -772,6 +772,9 @@ resizefs_out: >> #ifdef CONFIG_EXT4_FS_ENCRYPTION >> struct fscrypt_policy policy; >> >> + if (!ext4_has_feature_encrypt(sb)) >> + return -EOPNOTSUPP; >> + >> if (copy_from_user(&policy, > > Hi Richard, > > This is a good observation, and it happens this is already on my list of bugs to > address. I had not previously considered the fact that it allows the block_size > == PAGE_SIZE restriction to be easily circumvented. > > Ted had actually pointed out that the reason this hasn't already been fixed is > that some users, e.g. Android, do not set the feature flag but still expect the > filesystem encryption code to work. Maybe he can chime in with regards to when > (if ever) it would make sense to make this change.
We could automatically enable the feature flag in EXT4_IOC_SET_ENCRYPTION_POLICY, if possible. E.g. if (!ext4_sb_has_crypto(sb)) { if (sb_min_blocksize(sb, EXT4_MIN_BLOCK_SIZE) != PAGE_SIZE) return -EOPNOTSUPP; else { ext4_set_feature_encrypt(sb); ext4_commit_super(sb, 1); } }
> It should be noted that f2fs appears to have the same bug as well, with regards > to the corresponding f2fs feature flag. (Added Jaegeuk to the CC.) > > With regards to the proposed patch, I did notice that the code to handle > EXT4_IOC_GET_ENCRYPTION_PWSALT, just below the modified code, calls > ext4_sb_has_crypto() instead of ext4_has_feature_encrypt(). These are actually > the same thing, and ext4_sb_has_crypto() is only called from that one place. I > think the ioctl code should be consistent, so it may make sense to, as part of > the patch, remove ext4_sb_has_crypto() and switch EXT4_IOC_GET_ENCRYPTION_PWSALT > to using ext4_has_feature_encrypt().
Sure.
> Also, it seems the default block size for mkfs.ext4 is determined by a heuristic > and isn't guaranteed to be 1 KiB. So the commit message probably should say > something more general like "filesystems created with a block size other than > PAGE_SIZE".
Ahh, I thought 1KiB is default, my bad. Will happily update the commit log.
Thanks, //richard
| |