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@xxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
