Home » Mailing lists » Devel » Re: [PATCH] eCryptfs: Make key module subsystem respect namespaces
Re: [PATCH] eCryptfs: Make key module subsystem respect namespaces [message #29584] |
Thu, 17 April 2008 15:34 |
serue
Messages: 750 Registered: February 2006
|
Senior Member |
|
|
Quoting Michael Halcrow (mhalcrow@us.ibm.com):
> On Tue, Apr 15, 2008 at 04:34:02PM -0500, Serge E. Hallyn wrote:
> > Quoting Andrew Morton (akpm@linux-foundation.org):
> > > On Tue, 15 Apr 2008 15:23:13 -0500
> > > Michael Halcrow <mhalcrow@us.ibm.com> wrote:
> > >
> > > > ...
> > > > + rc = ecryptfs_find_daemon_by_euid(&daemon, current->euid);
> > > > + if (daemon->pid != current->pid) {
> > > > + rc = ecryptfs_find_daemon_by_euid(&daemon, current->euid);
> > > > + BUG_ON(current->euid != daemon->euid);
> > > > + BUG_ON(current->pid != daemon->pid);
> > >
> > > This code uses pids and uids all over the place. Will it operate
> > > correctly in a containerised environment?
> >
> > Thanks Andrew.
> >
> > Mike, the pid_t definately needs to be replaced with a struct pid.
> >
> > As for the euid, it'd be best if you also compared the user_namespace *
> > to make sure we support one ecryptfs deamon per user namespace.
>
> Make eCryptfs key module subsystem respect namespaces.
>
> Since I will be removing the netlink interface in a future patch, I
> just made changes to the netlink.c code so that it will not break the
> build. With my recent patches, the kernel module currently defaults to
> the device handle interface rather than the netlink interface.
>
> Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>
> ---
> fs/ecryptfs/ecryptfs_kernel.h | 25 +++++++++-----
> fs/ecryptfs/messaging.c | 71 +++++++++++++++++++++++++++++-----------
> fs/ecryptfs/miscdev.c | 68 +++++++++++++++++++++++++--------------
> fs/ecryptfs/netlink.c | 25 +++++++++-----
> 4 files changed, 126 insertions(+), 63 deletions(-)
>
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index 88b85f7..dc11431 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -34,6 +34,7 @@
> #include <linux/namei.h>
> #include <linux/scatterlist.h>
> #include <linux/hash.h>
> +#include <linux/nsproxy.h>
>
> /* Version verification for shared data structures w/ userspace */
> #define ECRYPTFS_VERSION_MAJOR 0x00
> @@ -410,8 +411,9 @@ struct ecryptfs_daemon {
> #define ECRYPTFS_DAEMON_MISCDEV_OPEN 0x00000008
> u32 flags;
> u32 num_queued_msg_ctx;
> - pid_t pid;
> + struct pid *pid;
> uid_t euid;
> + struct user_namespace *user_ns;
> struct task_struct *task;
> struct mutex mux;
> struct list_head msg_ctx_out_queue;
> @@ -610,10 +612,13 @@ int
> ecryptfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> size_t size, int flags);
> int ecryptfs_read_xattr_region(char *page_virt, struct inode *ecryptfs_inode);
> -int ecryptfs_process_helo(unsigned int transport, uid_t uid, pid_t pid);
> -int ecryptfs_process_quit(uid_t uid, pid_t pid);
> -int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t uid,
> - pid_t pid, u32 seq);
> +int ecryptfs_process_helo(unsigned int transport, uid_t euid,
> + struct user_namespace *user_ns, struct pid *pid);
> +int ecryptfs_process_quit(uid_t euid, struct user_namespace *user_ns,
> + struct pid *pid);
> +int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t euid,
> + struct user_namespace *user_ns, struct pid *pid,
> + u32 seq);
> int ecryptfs_send_message(unsigned int transport, char *data, int data_len,
> struct ecryptfs_msg_ctx **msg_ctx);
> int ecryptfs_wait_for_response(struct ecryptfs_msg_ctx *msg_ctx,
> @@ -623,13 +628,13 @@ void ecryptfs_release_messaging(unsigned int transport);
>
> int ecryptfs_send_netlink(char *data, int data_len,
> struct ecryptfs_msg_ctx *msg_ctx, u8 msg_type,
> - u16 msg_flags, pid_t daemon_pid);
> + u16 msg_flags, struct pid *daemon_pid);
> int ecryptfs_init_netlink(void);
> void ecryptfs_release_netlink(void);
>
> int ecryptfs_send_connector(char *data, int data_len,
> struct ecryptfs_msg_ctx *msg_ctx, u8 msg_type,
> - u16 msg_flags, pid_t daemon_pid);
> + u16 msg_flags, struct pid *daemon_pid);
> int ecryptfs_init_connector(void);
> void ecryptfs_release_connector(void);
> void
> @@ -672,7 +677,8 @@ int ecryptfs_read_lower_page_segment(struct page *page_for_ecryptfs,
> struct inode *ecryptfs_inode);
> struct page *ecryptfs_get_locked_page(struct file *file, loff_t index);
> int ecryptfs_exorcise_daemon(struct ecryptfs_daemon *daemon);
> -int ecryptfs_find_daemon_by_euid(struct ecryptfs_daemon **daemon, uid_t euid);
> +int ecryptfs_find_daemon_by_euid(struct ecryptfs_daemon **daemon, uid_t euid,
> + struct user_namespace *user_ns);
> int ecryptfs_parse_packet_length(unsigned char *data, size_t *size,
> size_t *length_size);
> int ecryptfs_write_packet_length(char *dest, size_t size,
> @@ -684,6 +690,7 @@ int ecryptfs_send_miscdev(char *data, size_t data_size,
> u16 msg_flags, struct ecryptfs_daemon *daemon);
> void ecryptfs_msg_ctx_alloc_to_free(struct ecryptfs_msg_ctx *msg_ctx);
> int
> -ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid, pid_t pid);
> +ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid,
> + struct user_namespace *user_ns, struct pid *pid);
>
> #endif /* #ifndef ECRYPTFS_KERNEL_H */
> diff --git a/fs/ecryptfs/messaging.c b/fs/ecryptfs/messaging.c
> index e3f2e97..fad161b 100644
> --- a/fs/ecryptfs/messaging.c
> +++ b/fs/ecryptfs/messaging.c
> @@ -103,6 +103,7 @@ void ecryptfs_msg_ctx_alloc_to_free(struct ecryptfs_msg_ctx *msg_ctx)
> /**
> * ecryptfs_find_daemon_by_euid
> * @euid: The effective user id which maps to the desired daemon id
> + * @user_ns: The namespace in which @euid applies
> * @daemon: If return value is zero, points to the desired daemon pointer
> *
> * Must be called with ecryptfs_daemon_hash_mux held.
> @@ -111,7 +112,8 @@ void ecryptfs_msg_ctx_alloc_to_free(struct ecryptfs_msg_ctx *msg_ctx)
> *
> * Returns zero if the user id exists in the list; non-zero otherwise.
> */
> -int ecryptfs_find_daemon_by_euid(struct ecryptfs_daemon **daemon, uid_t euid)
> +int ecryptfs_find_daemon_by_euid(struct ecryptfs_daemon **daemon, uid_t euid,
> + struct user_namespace *user_ns)
> {
> struct hlist_node *elem;
> int rc;
> @@ -119,7 +121,7 @@ int ecryptfs_find_daemon_by_euid(struct ecryptfs_daemon **daemon, uid_t euid)
> hlist_for_each_entry(*daemon, elem,
> &ecryptfs_daemon_hash[ecryptfs_uid_hash(euid)],
> euid_chain) {
> - if ((*daemon)->euid == euid) {
> + if ((*daemon)->euid == euid && (*daemon)->user_ns == user_ns) {
> rc = 0;
> goto out;
> }
> @@ -186,6 +188,7 @@ out:
> * ecryptfs_spawn_daemon - Create and initialize a new daemon struct
> * @daemon: Pointer to set to newly allocated daemon struct
> * @euid: Effective user id for the daemon
> + * @user_ns: The namespace in which @euid applies
> * @pid: Process id for the daemon
> *
> * Must be called ceremoniously while in possession of
> @@ -194,7 +197,8 @@ out:
> * Returns zero on success; non-zero otherwise
> */
> int
> -ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid, pid_t pid)
> +ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid,
> + struct user_namespace *user_ns, struct pid *pid)
> {
> int rc = 0;
>
> @@ -206,6 +210,7 @@ ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid, pid_t pid)
> goto out;
> }
> (*daemon)->euid = euid;
> + (*daemon)->user_ns = user_ns;
> (*daemon)->pid = pid;
You'll want to do a get_pid() here, no?
And get_user_ns().
It's not because you particulary need them to stick around, but just to
ensure no wraparound causes another daemon with the same struct pid or
user_namespace to be spawned. Pretty gosh-darned unlikely, but still...
> (*daemon)->task = current;
> mutex_init(&(*daemon)->mux);
> @@ -222,6 +227,7 @@ out:
> * ecryptfs_process_helo
> * @transport: The underlying transport (netlink, etc.)
> * @euid: The user ID owner of the message
> + * @user_ns: The namespace in which @euid applies
> * @pid: The process ID for the userspace program that sent the
> * message
> *
> @@ -231,32 +237,33 @@ out:
> * Returns zero after adding a new daemon to the hash list;
> * non-zero otherwise.
> */
> -int ecryptfs_process_helo(unsigned int transport, uid_t euid, pid_t pid)
> +int ecryptfs_process_helo(unsigned int transport, uid_t euid,
> + struct user_namespace *user_ns, struct pid *pid)
> {
> struct ecryptfs_daemon *new_daemon;
> struct ecryptfs_daemon *old_daemon;
> int rc;
>
> mutex_lock(&ecryptfs_daemon_hash_mux);
> - rc = ecryptfs_find_daemon_by_euid(&old_daemon, euid);
> + rc = ecryptfs_find_daemon_by_euid(&old_daemon, euid, user_ns);
> if (rc != 0) {
> printk(KERN_WARNING "Received request from user [%d] "
> - "to register daemon [%d]; unregistering daemon "
> - "[%d]\n", euid, pid, old_daemon->pid);
> + "to register daemon [0x%p]; unregistering daemon "
> + "[0x%p]\n", euid, pid, old_daemon->pid);
> rc = ecryptfs_send_raw_message(transport, ECRYPTFS_MSG_QUIT,
> o
...
|
|
|
[PATCH] eCryptfs: Fix refs to pid and user_ns [message #29666 is a reply to message #29584] |
Thu, 17 April 2008 17:03 |
Michael Halcrow
Messages: 6 Registered: February 2007
|
Junior Member |
|
|
On Thu, Apr 17, 2008 at 10:34:06AM -0500, Serge E. Hallyn wrote:
> Quoting Michael Halcrow (mhalcrow@us.ibm.com):
> > @@ -206,6 +210,7 @@ ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid, pid_t pid)
> > goto out;
> > }
> > (*daemon)->euid = euid;
> > + (*daemon)->user_ns = user_ns;
> > (*daemon)->pid = pid;
>
> You'll want to do a get_pid() here, no?
>
> And get_user_ns().
>
> It's not because you particulary need them to stick around, but just
> to ensure no wraparound causes another daemon with the same struct
> pid or user_namespace to be spawned. Pretty gosh-darned unlikely,
> but still...
> ...
> > @@ -372,12 +383,24 @@ int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t euid,
> > msg_ctx = &ecryptfs_msg_ctx_arr[msg->index];
> > mutex_lock(&msg_ctx->mux);
> > mutex_lock(&ecryptfs_daemon_hash_mux);
> > - rc = ecryptfs_find_daemon_by_euid(&daemon, msg_ctx->task->euid);
> > + rcu_read_lock();
> > + nsproxy = task_nsproxy(msg_ctx->task);
> > + if (nsproxy == NULL) {
> > + rc = -EBADMSG;
> > + printk(KERN_ERR "%s: Receiving process is a zombie. Dropping "
> > + "message.\n", __func__);
> > + rcu_read_unlock();
> > + mutex_unlock(&ecryptfs_daemon_hash_mux);
> > + goto wake_up;
> > + }
> > + rc = ecryptfs_find_daemon_by_euid(&daemon, msg_ctx->task->euid,
> > + nsproxy->user_ns);
> > + rcu_read_unlock();
> > mutex_unlock(&ecryptfs_daemon_hash_mux);
> > if (rc) {
> > rc = -EBADMSG;
> > printk(KERN_WARNING "%s: User [%d] received a "
> > - "message response from process [%d] but does "
> > + "message response from process [0x%p] but does "
> > "not have a registered daemon\n", __func__,
> > msg_ctx->task->euid, pid);
> > goto wake_up;
> > @@ -389,10 +412,17 @@ int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t euid,
> > euid, msg_ctx->task->euid);
> > goto unlock;
> > }
> > + if (nsproxy->user_ns != user_ns) {
>
> Since you didn't grab a ref to the nsproxy, it's possible that it
> will have been freed before this, right? So you probably just want
> to grab a copy of nsproxy->user_ns while under the rcu_read_lock,
> where you can be sure it's still around.
Have eCryptfs properly reference the pid and user_ns objects. Copy
user_ns out of nsproxy in case nsproxy goes away after we drop the
lock.
Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>
---
fs/ecryptfs/messaging.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/fs/ecryptfs/messaging.c b/fs/ecryptfs/messaging.c
index f0d74b8..61506e5 100644
--- a/fs/ecryptfs/messaging.c
+++ b/fs/ecryptfs/messaging.c
@@ -20,6 +20,8 @@
* 02111-1307, USA.
*/
#include <linux/sched.h>
+#include <linux/user_namespace.h>
+#include <linux/nsproxy.h>
#include "ecryptfs_kernel.h"
static LIST_HEAD(ecryptfs_msg_ctx_free_list);
@@ -208,8 +210,8 @@ ecryptfs_spawn_daemon(struct ecryptfs_daemon **daemon, uid_t euid,
goto out;
}
(*daemon)->euid = euid;
- (*daemon)->user_ns = user_ns;
- (*daemon)->pid = pid;
+ (*daemon)->user_ns = get_user_ns(user_ns);
+ (*daemon)->pid = get_pid(pid);
(*daemon)->task = current;
mutex_init(&(*daemon)->mux);
INIT_LIST_HEAD(&(*daemon)->msg_ctx_out_queue);
@@ -298,6 +300,10 @@ int ecryptfs_exorcise_daemon(struct ecryptfs_daemon *daemon)
hlist_del(&daemon->euid_chain);
if (daemon->task)
wake_up_process(daemon->task);
+ if (daemon->pid)
+ put_pid(daemon->pid);
+ if (daemon->user_ns)
+ put_user_ns(daemon->user_ns);
mutex_unlock(&daemon->mux);
memset(daemon, 0, sizeof(*daemon));
kfree(daemon);
@@ -368,6 +374,7 @@ int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t euid,
struct ecryptfs_msg_ctx *msg_ctx;
size_t msg_size;
struct nsproxy *nsproxy;
+ struct user_namespace *current_user_ns;
int rc;
if (msg->index >= ecryptfs_message_buf_len) {
@@ -391,8 +398,9 @@ int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t euid,
mutex_unlock(&ecryptfs_daemon_hash_mux);
goto wake_up;
}
+ current_user_ns = nsproxy->user_ns;
rc = ecryptfs_find_daemon_by_euid(&daemon, msg_ctx->task->euid,
- nsproxy->user_ns);
+ current_user_ns);
rcu_read_unlock();
mutex_unlock(&ecryptfs_daemon_hash_mux);
if (rc) {
@@ -410,7 +418,7 @@ int ecryptfs_process_response(struct ecryptfs_message *msg, uid_t euid,
euid, msg_ctx->task->euid);
goto unlock;
}
- if (nsproxy->user_ns != user_ns) {
+ if (current_user_ns != user_ns) {
rc = -EBADMSG;
printk(KERN_WARNING "%s: Received message from user_ns "
"[0x%p]; expected message from user_ns [0x%p]\n",
--
1.5.1.6
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
|
|
|
Goto Forum:
Current Time: Mon Sep 16 15:57:06 GMT 2024
Total time taken to generate the page: 0.04672 seconds
|