| Message ID | 20170102204342.vib5ayse4kthruma@thunk.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ted, On Mon, Jan 02, 2017 at 03:43:43PM -0500, Theodore Ts'o wrote: > The following patch is smaller (and causes a net reduction in code). > It also uses allows us to properly test the context inheritance code, > by completely removing any test_dummy_encryption specific hacks from > fscrypt_inherit_context(). > > The suggested userspace shell script fragment works on big-endian > systems, and uses fixed key instead of a random one. The latter is > useful for test sequences where it is useful to validate a file system > that had been previously created using an older kernel. I'm fine with your proposed version, though I'm not convinced it's really any better than mine, since it basically just moves the "hack" from fscrypt_inherit_context() to fscrypt_get_encryption_info(). The reason I preferred it in fscrypt_inherit_context() was that allowing fscrypt_get_encryption_info() to work on unencrypted files is kind of weird and could allow for confusing scenarios where a previously existing unencrypted file is accidentally treated as an encrypted one --- though that would require a missing ext4_encrypted_inode() check of course. Thanks, Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 04, 2017 at 04:16:06PM -0800, Eric Biggers wrote: > I'm fine with your proposed version, though I'm not convinced it's really any > better than mine, since it basically just moves the "hack" from > fscrypt_inherit_context() to fscrypt_get_encryption_info(). The reason I > preferred it in fscrypt_inherit_context() was that allowing > fscrypt_get_encryption_info() to work on unencrypted files is kind of weird and > could allow for confusing scenarios where a previously existing unencrypted file > is accidentally treated as an encrypted one --- though that would require a > missing ext4_encrypted_inode() check of course. Except that you *always* need to call ext4_encrypt_inode() before you call fscrypt_get_encryption_info(), because otherwise it becomes a performance disaster in the no encryption case, because we would be constantly doing failing xattr lookups. It also made for some especially tangled logic, which I noticed when you had to make a change in fs/ext4/namei.c: if ((ext4_encrypted_inode(dir) || <----------------------- DUMMY_ENCRYPTION_ENABLED(EXT4_SB(dir->i_sb))) && (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))) { - err = fscrypt_get_encryption_info(dir); - if (err) - return ERR_PTR(err); - if (!fscrypt_has_encryption_key(dir)) - return ERR_PTR(-EPERM); + if (ext4_encrypted_inode(dir)) { <------------------- + err = fscrypt_get_encryption_info(dir); + if (err) + return ERR_PTR(err); + if (!fscrypt_has_encryption_key(dir)) + return ERR_PTR(-EPERM); + } if (!handle) Your patch required the addition of a *second* call to ext4_encrypted_inode() or else the call to fscrypt_get_encryption_info() would fail in the test_dummy_encryption case. Not having hacks in the fscrypt_inherit_context() case also has the happy advantage that we test the normal context inheritance code path when creating files in the (unencrypted) root directory. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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/crypto/keyinfo.c b/fs/crypto/keyinfo.c index 95cd4c3b06c3..80f145c8d550 100644 --- a/fs/crypto/keyinfo.c +++ b/fs/crypto/keyinfo.c @@ -206,12 +206,16 @@ int fscrypt_get_crypt_info(struct inode *inode) res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx)); if (res < 0) { - if (!fscrypt_dummy_context_enabled(inode)) + if (!fscrypt_dummy_context_enabled(inode) || + inode->i_sb->s_cop->is_encrypted(inode)) return res; + /* Fake up a context for an unencrypted directory */ + memset(&ctx, 0, sizeof(ctx)); ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1; ctx.contents_encryption_mode = FS_ENCRYPTION_MODE_AES_256_XTS; ctx.filenames_encryption_mode = FS_ENCRYPTION_MODE_AES_256_CTS; - ctx.flags = 0; + memset(ctx.master_key_descriptor, 0x42, FS_KEY_DESCRIPTOR_SIZE); + res = sizeof(ctx); } else if (res != sizeof(ctx)) { return -EINVAL; } @@ -247,12 +251,6 @@ int fscrypt_get_crypt_info(struct inode *inode) if (!raw_key) goto out; - if (fscrypt_dummy_context_enabled(inode)) { - memset(raw_key, 0x42, keysize/2); - memset(raw_key+keysize/2, 0x24, keysize - (keysize/2)); - goto got_key; - } - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, FS_KEY_DESC_PREFIX_SIZE); if (res && inode->i_sb->s_cop->key_prefix) { @@ -270,7 +268,6 @@ int fscrypt_get_crypt_info(struct inode *inode) } else if (res) { goto out; } -got_key: ctfm = crypto_alloc_skcipher(cipher_str, 0, 0); if (!ctfm || IS_ERR(ctfm)) { res = ctfm ? PTR_ERR(ctfm) : -ENOMEM; diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c index 4c99972899c7..14b76da71269 100644 --- a/fs/crypto/policy.c +++ b/fs/crypto/policy.c @@ -198,9 +198,9 @@ EXPORT_SYMBOL(fscrypt_has_permitted_context); * @parent: Parent inode from which the context is inherited. * @child: Child inode that inherits the context from @parent. * @fs_data: private data given by FS. - * @preload: preload child i_crypt_info + * @preload: preload child i_crypt_info if true * - * Return: Zero on success, non-zero otherwise + * Return: 0 on success, -errno on failure */ int fscrypt_inherit_context(struct inode *parent, struct inode *child, void *fs_data, bool preload) @@ -221,19 +221,11 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child, return -ENOKEY; ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1; - if (fscrypt_dummy_context_enabled(parent)) { - ctx.contents_encryption_mode = FS_ENCRYPTION_MODE_AES_256_XTS; - ctx.filenames_encryption_mode = FS_ENCRYPTION_MODE_AES_256_CTS; - ctx.flags = 0; - memset(ctx.master_key_descriptor, 0x42, FS_KEY_DESCRIPTOR_SIZE); - res = 0; - } else { - ctx.contents_encryption_mode = ci->ci_data_mode; - ctx.filenames_encryption_mode = ci->ci_filename_mode; - ctx.flags = ci->ci_flags; - memcpy(ctx.master_key_descriptor, ci->ci_master_key, - FS_KEY_DESCRIPTOR_SIZE); - } + ctx.contents_encryption_mode = ci->ci_data_mode; + ctx.filenames_encryption_mode = ci->ci_filename_mode; + ctx.flags = ci->ci_flags; + memcpy(ctx.master_key_descriptor, ci->ci_master_key, + FS_KEY_DESCRIPTOR_SIZE); get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE); res = parent->i_sb->s_cop->set_context(child, &ctx, sizeof(ctx), fs_data);