* [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() @ 2016-09-22 6:50 Richard Weinberger 2016-09-22 19:49 ` Eric Biggers 2016-09-30 5:53 ` Theodore Ts'o 0 siblings, 2 replies; 6+ messages in thread From: Richard Weinberger @ 2016-09-22 6:50 UTC (permalink / raw) To: tytso; +Cc: adilger.kernel, linux-kernel, linux-ext4, david, Richard Weinberger ...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, (struct fscrypt_policy __user *)arg, sizeof(policy))) -- 2.7.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() 2016-09-22 6:50 [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() Richard Weinberger @ 2016-09-22 19:49 ` Eric Biggers 2016-09-22 20:17 ` Richard Weinberger 2016-09-22 22:38 ` Theodore Ts'o 2016-09-30 5:53 ` Theodore Ts'o 1 sibling, 2 replies; 6+ messages in thread From: Eric Biggers @ 2016-09-22 19:49 UTC (permalink / raw) To: Richard Weinberger Cc: tytso, adilger.kernel, linux-kernel, linux-ext4, david, jaegeuk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() 2016-09-22 19:49 ` Eric Biggers @ 2016-09-22 20:17 ` Richard Weinberger 2016-09-22 22:38 ` Theodore Ts'o 1 sibling, 0 replies; 6+ messages in thread From: Richard Weinberger @ 2016-09-22 20:17 UTC (permalink / raw) To: Eric Biggers Cc: tytso, adilger.kernel, linux-kernel, linux-ext4, david, jaegeuk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() 2016-09-22 19:49 ` Eric Biggers 2016-09-22 20:17 ` Richard Weinberger @ 2016-09-22 22:38 ` Theodore Ts'o 2016-09-23 21:56 ` Eric Biggers 1 sibling, 1 reply; 6+ messages in thread From: Theodore Ts'o @ 2016-09-22 22:38 UTC (permalink / raw) To: Eric Biggers Cc: Richard Weinberger, adilger.kernel, linux-kernel, linux-ext4, david, jaegeuk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() 2016-09-22 22:38 ` Theodore Ts'o @ 2016-09-23 21:56 ` Eric Biggers 0 siblings, 0 replies; 6+ messages in thread From: Eric Biggers @ 2016-09-23 21:56 UTC (permalink / raw) To: Theodore Ts'o, Richard Weinberger, adilger.kernel, linux-kernel, linux-ext4, david, jaegeuk 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() 2016-09-22 6:50 [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() Richard Weinberger 2016-09-22 19:49 ` Eric Biggers @ 2016-09-30 5:53 ` Theodore Ts'o 1 sibling, 0 replies; 6+ messages in thread From: Theodore Ts'o @ 2016-09-30 5:53 UTC (permalink / raw) To: Richard Weinberger; +Cc: adilger.kernel, linux-kernel, linux-ext4, david 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-30 5:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-22 6:50 [PATCH] ext4: Check for encryption feature before fscrypt_process_policy() Richard Weinberger 2016-09-22 19:49 ` Eric Biggers 2016-09-22 20:17 ` Richard Weinberger 2016-09-22 22:38 ` Theodore Ts'o 2016-09-23 21:56 ` Eric Biggers 2016-09-30 5:53 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).