...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 <[email protected]>
---
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,
(struct fscrypt_policy __user *)arg,
sizeof(policy)))
--
2.7.3
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 <[email protected]>
> ---
> 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.
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().
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".
Eric
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 <[email protected]>
>> ---
>> 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
On Thu, Sep 22, 2016 at 12:49:31PM -0700, Eric Biggers wrote:
>
> 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.
I think it's fine to fix it now in upstream. It might cause some
problems for Cyanogen developers if they want to try to use an
upstream kernel and also enable the ext4 encryption feature, but the
fix to make_ext4fs isn't all that hard.
> 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".
In fact mke2fs only uses 1k block sizes for filessystems smaller than
512M. If you were take a census of all ext4 file systems out there,
the most common block size you would see is 4k.
Cheers,
- Ted
On Thu, Sep 22, 2016 at 06:38:03PM -0400, Theodore Ts'o wrote:
>
> I think it's fine to fix it now in upstream. It might cause some
> problems for Cyanogen developers if they want to try to use an
> upstream kernel and also enable the ext4 encryption feature, but the
> fix to make_ext4fs isn't all that hard.
Would it make sense to at least provide a helpful error message in the kernel
log? For example:
if (!ext4_has_feature_encrypt(sb)) {
ext4_msg(sb, KERN_INFO,
"warning: process `%s' tried to set "
"encryption policy on filesystem without "
"encryption enabled. This is not supported. "
"Use 'tune2fs -O encrypt' to enable the "
"encryption feature flag first.",
current->comm);
return -EOPNOTSUPP;
}
Eric
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 <[email protected]>
Thanks, applied.
- Ted