| Message ID | 1474527054-24207-1-git-send-email-richard@nod.at |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
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. 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 -- 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
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 -- 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
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 -- 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
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 -- 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
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> Thanks, applied. - 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
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)))
...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(+)