From: Eric Biggers <ebiggers@kernel.org> To: Richard Weinberger <richard@nod.at> Cc: linux-mtd@lists.infradead.org, linux-fscrypt@vger.kernel.org, jaegeuk@kernel.org, tytso@mit.edu, linux-unionfs@vger.kernel.org, miklos@szeredi.hu, amir73il@gmail.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, paullawrence@google.com Subject: Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required Date: Thu, 14 Mar 2019 10:49:14 -0700 [thread overview] Message-ID: <20190314174913.GA30026@gmail.com> (raw) In-Reply-To: <20190314171559.27584-5-richard@nod.at> Hi Richard, On Thu, Mar 14, 2019 at 06:15:59PM +0100, Richard Weinberger wrote: > Usually fscrypt allows limited access to encrypted files even > if no key is available. > Encrypted filenames are shown and based on this names users > can unlink and move files. Actually, fscrypt doesn't allow moving files without the key. It would only be possible for cross-renames, i.e. renames with the RENAME_EXCHANGE flag. So for consistency with regular renames, fscrypt also forbids cross-renames if the key for either the source or destination directory is missing. So the main use case for the ciphertext view is *deleting* files. For example, deleting a user's home directory after that user has been removed from the system. Or the system freeing up space by deleting cache files from a user who isn't currently logged in. > > This is not always what people expect. The fscrypt_key_required mount > option disables this feature. > If no key is present all access is denied with the -ENOKEY error code. The problem with this mount option is that it allows users to create undeletable files. So I'm not really convinced yet this is a good change. And though the fscrypt_key_required semantics are easier to implement, we'd still have to support the existing semantics too, thus increasing the maintenance cost. > > The side benefit of this is that we don't need ->d_revalidate(). > Not having ->d_revalidate() makes an encrypted ubifs usable > as overlayfs upper directory. > It would be preferable if we could get overlayfs to work without providing a special mount option. > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > fs/ubifs/crypto.c | 2 +- > fs/ubifs/dir.c | 29 ++++++++++++++++++++++++++--- > fs/ubifs/super.c | 15 +++++++++++++++ > fs/ubifs/ubifs.h | 1 + > 4 files changed, 43 insertions(+), 4 deletions(-) > Shouldn't readlink() honor the mount option too? > diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c > index 4aaedf2d7f44..a6a48c5dc058 100644 > --- a/fs/ubifs/crypto.c > +++ b/fs/ubifs/crypto.c > @@ -76,7 +76,7 @@ int ubifs_decrypt(const struct inode *inode, struct ubifs_data_node *dn, > } > > const struct fscrypt_operations ubifs_crypt_operations = { > - .flags = FS_CFLG_OWN_PAGES, > + .flags = FS_CFLG_OWN_PAGES | FS_CFLG_OWN_D_OPS, > .key_prefix = "ubifs:", > .get_context = ubifs_crypt_get_context, > .set_context = ubifs_crypt_set_context, > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c > index b0cb913697c5..4d029f08b80d 100644 > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -208,6 +208,16 @@ static int dbg_check_name(const struct ubifs_info *c, > return 0; > } > > +static void ubifs_set_dentry_ops(struct inode *dir, struct dentry *dentry) > +{ > +#ifdef CONFIG_FS_ENCRYPTION > + struct ubifs_info *c = dir->i_sb->s_fs_info; > + > + if (IS_ENCRYPTED(dir) && !c->fscrypt_key_required) > + d_set_d_op(dentry, &fscrypt_d_ops); > +#endif > +} > + > static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > unsigned int flags) > { > @@ -224,7 +234,10 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > if (err) > return ERR_PTR(err); > > - err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm); > + ubifs_set_dentry_ops(dir, dentry); > + > + err = fscrypt_setup_filename(dir, &dentry->d_name, > + !c->fscrypt_key_required, &nm); > if (err) > return ERR_PTR(err); > > @@ -240,6 +253,11 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > } > > if (nm.hash) { > + if (c->fscrypt_key_required) { > + inode = ERR_PTR(-ENOKEY); > + goto done; > + } > + > ubifs_assert(c, fname_len(&nm) == 0); > ubifs_assert(c, fname_name(&nm) == NULL); > dent_key_init_hash(c, &key, dir->i_ino, nm.hash); > @@ -529,6 +547,9 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx) > if (err) > return err; > > + if (c->fscrypt_key_required && !dir->i_crypt_info) > + return -ENOKEY; > + How about returning -ENOKEY when trying to open the directory in the first place, rather than allowing getting to readdir()? That would match the behavior of regular files. > err = fscrypt_fname_alloc_buffer(dir, UBIFS_MAX_NLEN, &fstr); > if (err) > return err; > @@ -798,7 +819,8 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry) > return err; > } > > - err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm); > + err = fscrypt_setup_filename(dir, &dentry->d_name, !c->fscrypt_key_required, > + &nm); > if (err) > return err; > > @@ -908,7 +930,8 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry) > return err; > } > > - err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm); > + err = fscrypt_setup_filename(dir, &dentry->d_name, > + !c->fscrypt_key_required, &nm); > if (err) > return err; > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index 8dc2818fdd84..e081b00236b6 100644 > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -445,6 +445,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root) > ubifs_compr_name(c, c->mount_opts.compr_type)); > } > > + if (c->fscrypt_key_required) > + seq_puts(s, ",fscrypt_key_required"); > + > seq_printf(s, ",assert=%s", ubifs_assert_action_name(c)); > seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id); > > @@ -952,6 +955,7 @@ enum { > Opt_assert, > Opt_auth_key, > Opt_auth_hash_name, > + Opt_fscrypt_key_required, > Opt_ignore, > Opt_err, > }; > @@ -969,6 +973,7 @@ static const match_table_t tokens = { > {Opt_ignore, "ubi=%s"}, > {Opt_ignore, "vol=%s"}, > {Opt_assert, "assert=%s"}, > + {Opt_fscrypt_key_required, "fscrypt_key_required"}, > {Opt_err, NULL}, > }; > > @@ -1008,6 +1013,7 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options, > { > char *p; > substring_t args[MAX_OPT_ARGS]; > + unsigned int old_fscrypt_key_required = c->fscrypt_key_required; > > if (!options) > return 0; > @@ -1099,6 +1105,15 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options, > if (!c->auth_hash_name) > return -ENOMEM; > break; > + case Opt_fscrypt_key_required: > + c->fscrypt_key_required = 1; > + > + if (is_remount && (old_fscrypt_key_required != c->fscrypt_key_required)) { > + ubifs_err(c, "fscrypt_key_required cannot changed by remount"); > + return -EINVAL; > + } > + > + break; > case Opt_ignore: > break; > default: > diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h > index 1ae12900e01d..71b03a6798ae 100644 > --- a/fs/ubifs/ubifs.h > +++ b/fs/ubifs/ubifs.h > @@ -1304,6 +1304,7 @@ struct ubifs_info { > unsigned int rw_incompat:1; > unsigned int assert_action:2; > unsigned int authenticated:1; > + unsigned int fscrypt_key_required:1; > > struct mutex tnc_mutex; > struct ubifs_zbranch zroot; > -- > 2.21.0 >
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers@kernel.org> To: Richard Weinberger <richard@nod.at> Cc: tytso@mit.edu, miklos@szeredi.hu, amir73il@gmail.com, linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org, paullawrence@google.com, linux-fscrypt@vger.kernel.org, linux-mtd@lists.infradead.org, linux-fsdevel@vger.kernel.org, jaegeuk@kernel.org Subject: Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required Date: Thu, 14 Mar 2019 10:49:14 -0700 [thread overview] Message-ID: <20190314174913.GA30026@gmail.com> (raw) In-Reply-To: <20190314171559.27584-5-richard@nod.at> Hi Richard, On Thu, Mar 14, 2019 at 06:15:59PM +0100, Richard Weinberger wrote: > Usually fscrypt allows limited access to encrypted files even > if no key is available. > Encrypted filenames are shown and based on this names users > can unlink and move files. Actually, fscrypt doesn't allow moving files without the key. It would only be possible for cross-renames, i.e. renames with the RENAME_EXCHANGE flag. So for consistency with regular renames, fscrypt also forbids cross-renames if the key for either the source or destination directory is missing. So the main use case for the ciphertext view is *deleting* files. For example, deleting a user's home directory after that user has been removed from the system. Or the system freeing up space by deleting cache files from a user who isn't currently logged in. > > This is not always what people expect. The fscrypt_key_required mount > option disables this feature. > If no key is present all access is denied with the -ENOKEY error code. The problem with this mount option is that it allows users to create undeletable files. So I'm not really convinced yet this is a good change. And though the fscrypt_key_required semantics are easier to implement, we'd still have to support the existing semantics too, thus increasing the maintenance cost. > > The side benefit of this is that we don't need ->d_revalidate(). > Not having ->d_revalidate() makes an encrypted ubifs usable > as overlayfs upper directory. > It would be preferable if we could get overlayfs to work without providing a special mount option. > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > fs/ubifs/crypto.c | 2 +- > fs/ubifs/dir.c | 29 ++++++++++++++++++++++++++--- > fs/ubifs/super.c | 15 +++++++++++++++ > fs/ubifs/ubifs.h | 1 + > 4 files changed, 43 insertions(+), 4 deletions(-) > Shouldn't readlink() honor the mount option too? > diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c > index 4aaedf2d7f44..a6a48c5dc058 100644 > --- a/fs/ubifs/crypto.c > +++ b/fs/ubifs/crypto.c > @@ -76,7 +76,7 @@ int ubifs_decrypt(const struct inode *inode, struct ubifs_data_node *dn, > } > > const struct fscrypt_operations ubifs_crypt_operations = { > - .flags = FS_CFLG_OWN_PAGES, > + .flags = FS_CFLG_OWN_PAGES | FS_CFLG_OWN_D_OPS, > .key_prefix = "ubifs:", > .get_context = ubifs_crypt_get_context, > .set_context = ubifs_crypt_set_context, > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c > index b0cb913697c5..4d029f08b80d 100644 > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -208,6 +208,16 @@ static int dbg_check_name(const struct ubifs_info *c, > return 0; > } > > +static void ubifs_set_dentry_ops(struct inode *dir, struct dentry *dentry) > +{ > +#ifdef CONFIG_FS_ENCRYPTION > + struct ubifs_info *c = dir->i_sb->s_fs_info; > + > + if (IS_ENCRYPTED(dir) && !c->fscrypt_key_required) > + d_set_d_op(dentry, &fscrypt_d_ops); > +#endif > +} > + > static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > unsigned int flags) > { > @@ -224,7 +234,10 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > if (err) > return ERR_PTR(err); > > - err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm); > + ubifs_set_dentry_ops(dir, dentry); > + > + err = fscrypt_setup_filename(dir, &dentry->d_name, > + !c->fscrypt_key_required, &nm); > if (err) > return ERR_PTR(err); > > @@ -240,6 +253,11 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, > } > > if (nm.hash) { > + if (c->fscrypt_key_required) { > + inode = ERR_PTR(-ENOKEY); > + goto done; > + } > + > ubifs_assert(c, fname_len(&nm) == 0); > ubifs_assert(c, fname_name(&nm) == NULL); > dent_key_init_hash(c, &key, dir->i_ino, nm.hash); > @@ -529,6 +547,9 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx) > if (err) > return err; > > + if (c->fscrypt_key_required && !dir->i_crypt_info) > + return -ENOKEY; > + How about returning -ENOKEY when trying to open the directory in the first place, rather than allowing getting to readdir()? That would match the behavior of regular files. > err = fscrypt_fname_alloc_buffer(dir, UBIFS_MAX_NLEN, &fstr); > if (err) > return err; > @@ -798,7 +819,8 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry) > return err; > } > > - err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm); > + err = fscrypt_setup_filename(dir, &dentry->d_name, !c->fscrypt_key_required, > + &nm); > if (err) > return err; > > @@ -908,7 +930,8 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry) > return err; > } > > - err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm); > + err = fscrypt_setup_filename(dir, &dentry->d_name, > + !c->fscrypt_key_required, &nm); > if (err) > return err; > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index 8dc2818fdd84..e081b00236b6 100644 > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -445,6 +445,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root) > ubifs_compr_name(c, c->mount_opts.compr_type)); > } > > + if (c->fscrypt_key_required) > + seq_puts(s, ",fscrypt_key_required"); > + > seq_printf(s, ",assert=%s", ubifs_assert_action_name(c)); > seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id); > > @@ -952,6 +955,7 @@ enum { > Opt_assert, > Opt_auth_key, > Opt_auth_hash_name, > + Opt_fscrypt_key_required, > Opt_ignore, > Opt_err, > }; > @@ -969,6 +973,7 @@ static const match_table_t tokens = { > {Opt_ignore, "ubi=%s"}, > {Opt_ignore, "vol=%s"}, > {Opt_assert, "assert=%s"}, > + {Opt_fscrypt_key_required, "fscrypt_key_required"}, > {Opt_err, NULL}, > }; > > @@ -1008,6 +1013,7 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options, > { > char *p; > substring_t args[MAX_OPT_ARGS]; > + unsigned int old_fscrypt_key_required = c->fscrypt_key_required; > > if (!options) > return 0; > @@ -1099,6 +1105,15 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options, > if (!c->auth_hash_name) > return -ENOMEM; > break; > + case Opt_fscrypt_key_required: > + c->fscrypt_key_required = 1; > + > + if (is_remount && (old_fscrypt_key_required != c->fscrypt_key_required)) { > + ubifs_err(c, "fscrypt_key_required cannot changed by remount"); > + return -EINVAL; > + } > + > + break; > case Opt_ignore: > break; > default: > diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h > index 1ae12900e01d..71b03a6798ae 100644 > --- a/fs/ubifs/ubifs.h > +++ b/fs/ubifs/ubifs.h > @@ -1304,6 +1304,7 @@ struct ubifs_info { > unsigned int rw_incompat:1; > unsigned int assert_action:2; > unsigned int authenticated:1; > + unsigned int fscrypt_key_required:1; > > struct mutex tnc_mutex; > struct ubifs_zbranch zroot; > -- > 2.21.0 > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2019-03-14 17:49 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-13 12:31 overlayfs vs. fscrypt Richard Weinberger 2019-03-13 12:31 ` Richard Weinberger 2019-03-13 12:36 ` Miklos Szeredi 2019-03-13 12:47 ` Richard Weinberger 2019-03-13 12:47 ` Richard Weinberger 2019-03-13 12:58 ` Miklos Szeredi 2019-03-13 13:00 ` Richard Weinberger 2019-03-13 13:00 ` Richard Weinberger 2019-03-13 13:24 ` Miklos Szeredi 2019-03-13 13:32 ` Richard Weinberger 2019-03-13 13:32 ` Richard Weinberger 2019-03-13 14:26 ` Amir Goldstein 2019-03-13 15:16 ` Theodore Ts'o 2019-03-13 15:30 ` Richard Weinberger 2019-03-13 15:30 ` Richard Weinberger 2019-03-13 15:36 ` James Bottomley 2019-03-13 15:51 ` Eric Biggers 2019-03-13 16:13 ` James Bottomley 2019-03-13 16:24 ` Richard Weinberger 2019-03-13 16:44 ` Theodore Ts'o 2019-03-13 17:45 ` James Bottomley 2019-03-13 18:58 ` Theodore Ts'o 2019-03-13 19:17 ` James Bottomley 2019-03-13 19:57 ` Eric Biggers 2019-03-13 20:06 ` James Bottomley 2019-03-13 20:25 ` Eric Biggers 2019-03-13 21:04 ` James Bottomley 2019-03-13 22:13 ` Eric Biggers 2019-03-13 22:29 ` James Bottomley 2019-03-13 22:58 ` Eric Biggers 2019-03-13 16:06 ` Al Viro 2019-03-13 16:44 ` Eric Biggers 2019-03-13 19:19 ` Al Viro 2019-03-13 19:43 ` Eric Biggers 2019-03-13 15:30 ` Eric Biggers 2019-03-13 15:30 ` Eric Biggers 2019-03-13 20:33 ` Richard Weinberger 2019-03-13 20:33 ` Richard Weinberger 2019-03-13 22:26 ` Eric Biggers 2019-03-13 22:26 ` Eric Biggers 2019-03-13 22:42 ` Richard Weinberger 2019-03-14 7:34 ` Miklos Szeredi 2019-03-14 17:15 ` [RFC] fscrypt_key_required mount option Richard Weinberger 2019-03-14 17:15 ` Richard Weinberger 2019-03-14 17:15 ` [PATCH 1/4] fscrypt: Implement FS_CFLG_OWN_D_OPS Richard Weinberger 2019-03-14 17:15 ` Richard Weinberger 2019-03-14 17:15 ` [PATCH 2/4] fscrypt: Export fscrypt_d_ops Richard Weinberger 2019-03-14 17:15 ` Richard Weinberger 2019-03-14 17:15 ` [PATCH 3/4] ubifs: Simplify fscrypt_get_encryption_info() error handling Richard Weinberger 2019-03-14 17:15 ` Richard Weinberger 2019-03-14 17:15 ` [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required Richard Weinberger 2019-03-14 17:15 ` Richard Weinberger 2019-03-14 17:49 ` Eric Biggers [this message] 2019-03-14 17:49 ` Eric Biggers 2019-03-14 20:54 ` Richard Weinberger 2019-03-14 20:54 ` Richard Weinberger 2019-03-14 23:07 ` Theodore Ts'o 2019-03-14 23:07 ` Theodore Ts'o 2019-03-15 0:26 ` Unsubscribe Shane Volpe 2019-03-15 7:48 ` [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required Richard Weinberger 2019-03-15 7:48 ` Richard Weinberger 2019-03-15 13:51 ` Theodore Ts'o 2019-03-15 13:51 ` Theodore Ts'o 2019-03-15 13:51 ` Theodore Ts'o 2019-03-15 13:59 ` Richard Weinberger 2019-03-15 13:59 ` Richard Weinberger 2019-03-14 23:15 ` James Bottomley 2019-03-14 23:15 ` James Bottomley 2019-03-14 23:42 ` Theodore Ts'o 2019-03-14 23:42 ` Theodore Ts'o 2019-03-14 23:55 ` James Bottomley 2019-03-14 23:55 ` James Bottomley 2019-03-13 15:01 ` overlayfs vs. fscrypt Eric Biggers 2019-03-13 15:01 ` Eric Biggers 2019-03-13 16:11 ` Al Viro 2019-03-13 16:33 ` Eric Biggers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190314174913.GA30026@gmail.com \ --to=ebiggers@kernel.org \ --cc=amir73il@gmail.com \ --cc=jaegeuk@kernel.org \ --cc=linux-fscrypt@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=linux-unionfs@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=paullawrence@google.com \ --cc=richard@nod.at \ --cc=tytso@mit.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.