On 2016-04-12 03:15:44, Ricky Zhou wrote: > In LSMs such as SELinux, files can be associated with state from the > credentials of the task that opens it. Since ecryptfs shares a single > handle to lower files across tasks that access it, others tasks can > later be denied access to the lower file as a result. > > This change removes the kthread and unconditionally opens lower files > with kernel service credentials. Under SELinux, users will need to allow > the FD__USE permissions on the kernel context to process that need to > access files on an ecryptfs filesystem. > > Signed-off-by: Ricky Zhou <ric...@chromium.org> > --- > fs/ecryptfs/Makefile | 2 +- > fs/ecryptfs/ecryptfs_kernel.h | 4 - > fs/ecryptfs/kthread.c | 172 > ------------------------------------------ > fs/ecryptfs/main.c | 78 +++++++++++++++---- > 4 files changed, 65 insertions(+), 191 deletions(-) > delete mode 100644 fs/ecryptfs/kthread.c > > diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile > index 49678a6..f0bcf51 100644 > --- a/fs/ecryptfs/Makefile > +++ b/fs/ecryptfs/Makefile > @@ -5,6 +5,6 @@ > obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o > > ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o read_write.o \ > - crypto.o keystore.o kthread.o debug.o > + crypto.o keystore.o debug.o > > ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o > diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h > index d123fba..6434736 100644 > --- a/fs/ecryptfs/ecryptfs_kernel.h > +++ b/fs/ecryptfs/ecryptfs_kernel.h > @@ -695,10 +695,6 @@ int ecryptfs_find_daemon_by_euid(struct ecryptfs_daemon > **daemon); > #endif > int ecryptfs_init_kthread(void); > void ecryptfs_destroy_kthread(void);
The ecryptfs_{init,destroy}_kthread() declarations also need to be
removed.
> -int ecryptfs_privileged_open(struct file **lower_file,
> - struct dentry *lower_dentry,
> - struct vfsmount *lower_mnt,
> - const struct cred *cred);
> int ecryptfs_get_lower_file(struct dentry *dentry, struct inode *inode);
> void ecryptfs_put_lower_file(struct inode *inode);
> int
> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
> deleted file mode 100644
> index 866bb18..0000000
> --- a/fs/ecryptfs/kthread.c
> +++ /dev/null
> @@ -1,172 +0,0 @@
> -/**
> - * eCryptfs: Linux filesystem encryption layer
> - *
> - * Copyright (C) 2008 International Business Machines Corp.
> - * Author(s): Michael A. Halcrow <mahal...@us.ibm.com>
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License as
> - * published by the Free Software Foundation; either version 2 of the
> - * License, or (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - * General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
> - * 02111-1307, USA.
> - */
> -
> -#include <linux/kthread.h>
> -#include <linux/freezer.h>
> -#include <linux/slab.h>
> -#include <linux/wait.h>
> -#include <linux/mount.h>
> -#include "ecryptfs_kernel.h"
> -
> -struct ecryptfs_open_req {
> - struct file **lower_file;
> - struct path path;
> - struct completion done;
> - struct list_head kthread_ctl_list;
> -};
> -
> -static struct ecryptfs_kthread_ctl {
> -#define ECRYPTFS_KTHREAD_ZOMBIE 0x00000001
> - u32 flags;
> - struct mutex mux;
> - struct list_head req_list;
> - wait_queue_head_t wait;
> -} ecryptfs_kthread_ctl;
> -
> -static struct task_struct *ecryptfs_kthread;
> -
> -/**
> - * ecryptfs_threadfn
> - * @ignored: ignored
> - *
> - * The eCryptfs kernel thread that has the responsibility of getting
> - * the lower file with RW permissions.
> - *
> - * Returns zero on success; non-zero otherwise
> - */
> -static int ecryptfs_threadfn(void *ignored)
> -{
> - set_freezable();
> - while (1) {
> - struct ecryptfs_open_req *req;
> -
> - wait_event_freezable(
> - ecryptfs_kthread_ctl.wait,
> - (!list_empty(&ecryptfs_kthread_ctl.req_list)
> - || kthread_should_stop()));
> - mutex_lock(&ecryptfs_kthread_ctl.mux);
> - if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
> - mutex_unlock(&ecryptfs_kthread_ctl.mux);
> - goto out;
> - }
> - while (!list_empty(&ecryptfs_kthread_ctl.req_list)) {
> - req = list_first_entry(&ecryptfs_kthread_ctl.req_list,
> - struct ecryptfs_open_req,
> - kthread_ctl_list);
> - list_del(&req->kthread_ctl_list);
> - *req->lower_file = dentry_open(&req->path,
> - (O_RDWR | O_LARGEFILE), current_cred());
> - complete(&req->done);
> - }
> - mutex_unlock(&ecryptfs_kthread_ctl.mux);
> - }
> -out:
> - return 0;
> -}
> -
> -int __init ecryptfs_init_kthread(void)
> -{
> - int rc = 0;
> -
> - mutex_init(&ecryptfs_kthread_ctl.mux);
> - init_waitqueue_head(&ecryptfs_kthread_ctl.wait);
> - INIT_LIST_HEAD(&ecryptfs_kthread_ctl.req_list);
> - ecryptfs_kthread = kthread_run(&ecryptfs_threadfn, NULL,
> - "ecryptfs-kthread");
> - if (IS_ERR(ecryptfs_kthread)) {
> - rc = PTR_ERR(ecryptfs_kthread);
> - printk(KERN_ERR "%s: Failed to create kernel thread; rc = [%d]"
> - "\n", __func__, rc);
> - }
> - return rc;
> -}
> -
> -void ecryptfs_destroy_kthread(void)
> -{
> - struct ecryptfs_open_req *req, *tmp;
> -
> - mutex_lock(&ecryptfs_kthread_ctl.mux);
> - ecryptfs_kthread_ctl.flags |= ECRYPTFS_KTHREAD_ZOMBIE;
> - list_for_each_entry_safe(req, tmp, &ecryptfs_kthread_ctl.req_list,
> - kthread_ctl_list) {
> - list_del(&req->kthread_ctl_list);
> - *req->lower_file = ERR_PTR(-EIO);
> - complete(&req->done);
> - }
> - mutex_unlock(&ecryptfs_kthread_ctl.mux);
> - kthread_stop(ecryptfs_kthread);
> - wake_up(&ecryptfs_kthread_ctl.wait);
> -}
> -
> -/**
> - * ecryptfs_privileged_open
> - * @lower_file: Result of dentry_open by root on lower dentry
> - * @lower_dentry: Lower dentry for file to open
> - * @lower_mnt: Lower vfsmount for file to open
> - *
> - * This function gets a r/w file opened againt the lower dentry.
> - *
> - * Returns zero on success; non-zero otherwise
> - */
> -int ecryptfs_privileged_open(struct file **lower_file,
> - struct dentry *lower_dentry,
> - struct vfsmount *lower_mnt,
> - const struct cred *cred)
> -{
> - struct ecryptfs_open_req req;
> - int flags = O_LARGEFILE;
> - int rc = 0;
> -
> - init_completion(&req.done);
> - req.lower_file = lower_file;
> - req.path.dentry = lower_dentry;
> - req.path.mnt = lower_mnt;
> -
> - /* Corresponding dput() and mntput() are done when the
> - * lower file is fput() when all eCryptfs files for the inode are
> - * released. */
> - flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
> - (*lower_file) = dentry_open(&req.path, flags, cred);
> - if (!IS_ERR(*lower_file))
> - goto out;
> - if ((flags & O_ACCMODE) == O_RDONLY) {
> - rc = PTR_ERR((*lower_file));
> - goto out;
> - }
> - mutex_lock(&ecryptfs_kthread_ctl.mux);
> - if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
> - rc = -EIO;
> - mutex_unlock(&ecryptfs_kthread_ctl.mux);
> - printk(KERN_ERR "%s: We are in the middle of shutting down; "
> - "aborting privileged request to open lower file\n",
> - __func__);
> - goto out;
> - }
> - list_add_tail(&req.kthread_ctl_list, &ecryptfs_kthread_ctl.req_list);
> - mutex_unlock(&ecryptfs_kthread_ctl.mux);
> - wake_up(&ecryptfs_kthread_ctl.wait);
> - wait_for_completion(&req.done);
> - if (IS_ERR(*lower_file))
> - rc = PTR_ERR(*lower_file);
> -out:
> - return rc;
> -}
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index 1698132..9459fbb 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -24,6 +24,7 @@
> * 02111-1307, USA.
> */
>
> +#include <linux/cred.h>
> #include <linux/dcache.h>
> #include <linux/file.h>
> #include <linux/module.h>
> @@ -95,6 +96,56 @@ void __ecryptfs_printk(const char *fmt, ...)
> }
>
> /**
> + * Credentials for opening lower files.
> + */
> +static const struct cred *kernel_cred;
> +
> +/**
> + * ecryptfs_privileged_open
> + * @lower_file: Result of dentry_open by root on lower dentry
> + * @lower_dentry: Lower dentry for file to open
> + * @lower_mnt: Lower vfsmount for file to open
> + *
> + * This function gets a r/w file opened againt the lower dentry.
s/againt/against/
> + *
> + * Returns zero on success; non-zero otherwise
> + */
> +static int ecryptfs_privileged_open(struct file **lower_file,
> + struct dentry *lower_dentry,
> + struct vfsmount *lower_mnt)
> +{
> + struct path path;
> + int flags = O_LARGEFILE;
> + int rc = 0;
> + const struct cred *old_cred;
> +
> + path.dentry = lower_dentry;
> + path.mnt = lower_mnt;
> + flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
> +
> + /*
> + * Use kernel service credentials to open the lower file, as the current
> + * task may not have write privileges. Uses kernel creds instead of
> + * normal creds with CAP_DAC_OVERRIDE because because some LSMs like
> + * SELinux associate the file with extra state from the current
> + * credentials. When this happens, access to the lower file can
> + * be affected by which task was the first to open it.
> + */
> + old_cred = override_creds(kernel_cred);
Do you know if we have to call override_creds() here or can we simply
pass kernel_cred to dentry_open()?
The LSMs would get kernel_cred passed to their file_open hook but I'm
wondering if we could potentially hit an issue where we don't have
enough privs when calling into a filesystem's d_op->d_select_inode() or
f_op->open().
> +
> + /* Corresponding dput() and mntput() are done when the
> + * lower file is fput() when all eCryptfs files for the inode are
> + * released. */
> + (*lower_file) = dentry_open(&path, flags, kernel_cred);
> +
> + revert_creds(old_cred);
> +
> + if (IS_ERR(*lower_file))
> + rc = PTR_ERR(*lower_file);
> + return rc;
> +}
> +
> +/**
> * ecryptfs_init_lower_file
> * @ecryptfs_dentry: Fully initialized eCryptfs dentry object, with
> * the lower dentry and the lower mount set
> @@ -118,12 +169,10 @@ void __ecryptfs_printk(const char *fmt, ...)
> static int ecryptfs_init_lower_file(struct dentry *dentry,
> struct file **lower_file)
> {
> - const struct cred *cred = current_cred();
> struct path *path = ecryptfs_dentry_to_lower_path(dentry);
> int rc;
>
> - rc = ecryptfs_privileged_open(lower_file, path->dentry, path->mnt,
> - cred);
> + rc = ecryptfs_privileged_open(lower_file, path->dentry, path->mnt);
> if (rc) {
> printk(KERN_ERR "Error opening lower file "
> "for lower_dentry [0x%p] and lower_mnt [0x%p]; "
> @@ -829,29 +878,30 @@ static int __init ecryptfs_init(void)
> (unsigned long)PAGE_SIZE);
> goto out;
> }
> + kernel_cred = prepare_kernel_cred(NULL);
I see the prepare_kernel_cred(NULL) results in the capabilities being
set to CAP_FULL_SET. I don't love the thought of that because I don't
think it is necessary. To reduce the potential for mistakes/abuse, I'd
like to see us use a kernel cred that has the bare minimum set of caps
that is needed to perform this dentry_open().
I think we're getting close to landing on a final patch. Thanks for
being patient with my slow review turnarounds. Things have been busy
with the Ubuntu 16.04 release.
Tyler
> + if (kernel_cred == NULL) {
> + rc = -ENOMEM;
> + ecryptfs_printk(KERN_ERR,
> + "Failed to prepare kernel credentials\n");
> + goto out;
> + }
> rc = ecryptfs_init_kmem_caches();
> if (rc) {
> printk(KERN_ERR
> "Failed to allocate one or more kmem_cache objects\n");
> - goto out;
> + goto out_put_cred;
> }
> rc = do_sysfs_registration();
> if (rc) {
> printk(KERN_ERR "sysfs registration failed\n");
> goto out_free_kmem_caches;
> }
> - rc = ecryptfs_init_kthread();
> - if (rc) {
> - printk(KERN_ERR "%s: kthread initialization failed; "
> - "rc = [%d]\n", __func__, rc);
> - goto out_do_sysfs_unregistration;
> - }
> rc = ecryptfs_init_messaging();
> if (rc) {
> printk(KERN_ERR "Failure occurred while attempting to "
> "initialize the communications channel to "
> "ecryptfsd\n");
> - goto out_destroy_kthread;
> + goto out_do_sysfs_unregistration;
> }
> rc = ecryptfs_init_crypto();
> if (rc) {
> @@ -873,12 +923,12 @@ out_destroy_crypto:
> ecryptfs_destroy_crypto();
> out_release_messaging:
> ecryptfs_release_messaging();
> -out_destroy_kthread:
> - ecryptfs_destroy_kthread();
> out_do_sysfs_unregistration:
> do_sysfs_unregistration();
> out_free_kmem_caches:
> ecryptfs_free_kmem_caches();
> +out_put_cred:
> + put_cred(kernel_cred);
> out:
> return rc;
> }
> @@ -892,10 +942,10 @@ static void __exit ecryptfs_exit(void)
> printk(KERN_ERR "Failure whilst attempting to destroy crypto; "
> "rc = [%d]\n", rc);
> ecryptfs_release_messaging();
> - ecryptfs_destroy_kthread();
> do_sysfs_unregistration();
> unregister_filesystem(&ecryptfs_fs_type);
> ecryptfs_free_kmem_caches();
> + put_cred(kernel_cred);
> }
>
> MODULE_AUTHOR("Michael A. Halcrow <mhalc...@us.ibm.com>");
> --
> 2.8.0.rc3.226.g39d4020
>
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
signature.asc
Description: PGP signature
