cr: credentials

Ony seems useful if you're using coda or hppfs, but go ahead and

restore a file's f_cred. This is set to the cred of the task doing the open, so often it will be the same as that of the restarted task.

Signed-off-by Serge E. Hallyn <serue-r/[email protected]>

---
 checkpoint/files.c             |   16 ++++++++++++++--
 include/linux/checkpoint_hdr.h |    1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

checkpoint/files.c +14 -2

index 6107361..edf81ce 100644
@@ -151,7 +151,11 @@ int checkpoint_file_common(struct ckpt_ctx *ctx, struct file *file,
 	h->f_pos = file->f_pos;
 	h->f_version = file->f_version;
 
-	/* FIX: need also file->uid, file->gid, file->f_owner, etc */
+	h->f_credref = checkpoint_obj(ctx, file->f_cred, CKPT_OBJ_CRED);
+	if (h->f_credref < 0)
+		return h->f_credref;
+
+	/* FIX: need also file->f_owner, etc */
 
 	return 0;
 }

@@ -359,8 +363,16 @@ int restore_file_common(struct ckpt_ctx *ctx, struct file *file,
 			struct ckpt_hdr_file *h)
 {
 	int ret;
+	struct cred *cred;
+
+	/* FIX: need to restore owner etc */
 
-	/* FIX: need to restore uid, gid, owner etc */
+	/* restore the cred */
+	cred = ckpt_obj_fetch(ctx, h->f_credref, CKPT_OBJ_CRED);
+	if (IS_ERR(cred))
+		return PTR_ERR(cred);
+	put_cred(file->f_cred);
+	file->f_cred = get_cred(cred);
 
 	/* safe to set 1st arg (fd) to 0, as command is F_SETFL */
 	ret = vfs_fcntl(0, F_SETFL, h->f_flags & CKPT_SETFL_MASK, file);

include/linux/checkpoint_hdr.h +0 -0

index 98fc0f7..2693a72 100644

Quoted 1 line · Serge E. Hallyn

Ony seems useful if you're using coda or hppfs, but go ahead and

^^^

(It is also used in some place under net/*...)

include/linux/checkpoint_hdr.h struct ckpt_hdr_file { · line 344

Quoted 9 lines

include/linux/checkpoint_hdr.h +1 -0

index 98fc0f7..2693a72 100644
@@ -341,6 +341,7 @@ struct ckpt_hdr_file {
 	__u32 _padding;
 	__u64 f_pos;
 	__u64 f_version;
+	__s32 f_credref;

This can replace the unused _padding field above ?

include/linux/checkpoint_hdr.h struct ckpt_hdr_file { · line 347

Quoted 3 lines

include/linux/checkpoint_hdr.h +0 -0

 } __attribute__((aligned(8)));
 
 struct ckpt_hdr_file_generic {

Acked-by Oren Laadan <[email protected]>

Quoted 6 lines · Oren Laadan

Quoted 1 line · Serge E. Hallyn

Ony seems useful if you're using coda or hppfs, but go ahead and

^^^

(It is also used in some place under net/*...)

ok

Quoted 10 lines

include/linux/checkpoint_hdr.h struct ckpt_hdr_file { · line 344

Quoted 9 lines

include/linux/checkpoint_hdr.h +1 -0

index 98fc0f7..2693a72 100644
@@ -341,6 +341,7 @@ struct ckpt_hdr_file {
 	__u32 _padding;
 	__u64 f_pos;
 	__u64 f_version;
+	__s32 f_credref;

This can replace the unused _padding field above ?

Oh, of course.

Quoted 4 lines

include/linux/checkpoint_hdr.h struct ckpt_hdr_file { · line 347

Quoted 3 lines

include/linux/checkpoint_hdr.h +0 -0

 } __attribute__((aligned(8)));
 
 struct ckpt_hdr_file_generic {

Acked-by Oren Laadan <[email protected]>

Thanks - did you want me to send a new version, or will you apply the updated patch?

Review

Quoted 3 lines · Serge E. Hallyn

(quoted diff) +3 -0

+/* move this code into kernel/cred.c and do proper perms checking of course */
+struct cred *restore_read_cred(struct ckpt_ctx *ctx)
+{

This function needs to fix up cred->security.

Or did you mean something else by 'fix up' cred->security?

Quoted 1 line · Serge E. Hallyn

Or did you mean something else by 'fix up' cred->security?

cred->security is inherited from the current process by virtue of calling prepare_creds() - as such, it is almost certainly going to be wrong. Can you just ask the LSM for a set of textual security labels when saving, and then set those back when restoring?

This patch adds the checkpointing and restart of credentials (uids, gids, and capabilities) to Oren's c/r patchset (on top of v14). It goes to great pains to re-use (and define when needed) common helpers, in order to make sure that as security code is modified, the cr code will be updated. Some of the helpers should still be moved (i.e. _creds() functions should be in kernel/cred.c).

When building the credentials for the restarted process, I
1. create a new struct cred as a copy of the running task's cred (using prepare_cred())
2. always authorize any changes to the new struct cred based on the permissions of current_cred() (not the current transient state of the new cred).

While this may mean that certain transient_cred1->transient_cred2 states are allowed which otherwise wouldn't be allowed, the fact remains that current_cred() is allowed to transition to transient_cred2.

The reconstructed creds are applied to the task at the very end of the sys_restart call. This ensures that any objects which need to be re-created (file, socket, etc) are re-created using the creds of the task calling sys_restart - preventing an unpriv user from creating a privileged object, and ensuring that a root task can restart a process which had started out privileged, created some privileged objects, then dropped its privilege.

With these patches, the root user can restart checkpoint images (created by either hallyn or root) of user hallyn's tasks, resulting in a program owned by hallyn.

Plenty of bugs to be found, no doubt.

Changelog:
May 18: fix more refcounting: if (userns 5, uid 0) had
no active tasks or child user_namespaces, then it shouldn't exist at restart or it, its namespace, and its whole chain of creators will be leaked.
May 14: fix some refcounting:
1. a new user_ns needs a ref to remain pinned by its root user
2. current_user_ns needs an extra ref bc objhash drops two on restart
3. cred needs a ref for the real credentials bc commit_creds eats one ref.
May 13: folded in fix to userns refcounting.

Signed-off-by Serge E. Hallyn <serue-r/[email protected]>

Serge -

The 'creator' makes the 'struct user' recursive because to save an object you need to first save its creator etc. However the implementation may not call checkpoint_obj() recursively, if the depth isn't bound a-priory. You probably need to convert checkpoint_write_user() to do an iterative (loop) implementation of the recursion...

Quoted 8 lines · Oren Laadan

Serge -

The 'creator' makes the 'struct user' recursive because to save an object you need to first save its creator etc. However the implementation may not call checkpoint_obj() recursively, if the depth isn't bound a-priory. You probably need to convert checkpoint_write_user() to do an iterative (loop) implementation of the recursion...

It's not done recursively, though. checkpoint_write_userns() will
only be called by checkpoint_write_user(). checkpoint_write_user()
will make sure there are only 5 levels deep of unwritten creator user namespace (else bail), then start at the oldest creator, and write it. That checkpoint_write_user will cause a write of its user_ns, then write the user, then return to the original checkpoint_write_user() which will now write the next-oldest user_struct

So the max recursion depth we get is:

	checkpoint_task_struct -> checkpoint_write_cred ->
	checkpoint_write_user -> checkpoint_write_user ->
	checkpoint_write_userns (END)

Quoted 19 lines · Serge E. Hallyn

Quoted 8 lines · Oren Laadan

Serge -

The 'creator' makes the 'struct user' recursive because to save an object you need to first save its creator etc. However the implementation may not call checkpoint_obj() recursively, if the depth isn't bound a-priory. You probably need to convert checkpoint_write_user() to do an iterative (loop) implementation of the recursion...

It's not done recursively, though. checkpoint_write_userns() will
only be called by checkpoint_write_user(). checkpoint_write_user()
will make sure there are only 5 levels deep of unwritten creator user namespace (else bail), then start at the oldest creator, and write it. That checkpoint_write_user will cause a write of its user_ns, then write the user, then return to the original checkpoint_write_user() which will now write the next-oldest user_struct

Quoted 5 lines

So the max recursion depth we get is:

	checkpoint_task_struct -> checkpoint_write_cred ->
	checkpoint_write_user -> checkpoint_write_user ->
	checkpoint_write_userns (END)

Ok. Let's not restrict to 5 levels :)

This patch adds the checkpointing and restart of credentials (uids, gids, and capabilities) to Oren's c/r patchset (on top of v14). It goes to great pains to re-use (and define when needed) common helpers, in order to make sure that as security code is modified, the cr code will be updated. Some of the helpers should still be moved (i.e. _creds() functions should be in kernel/cred.c).

When building the credentials for the restarted process, I
1. create a new struct cred as a copy of the running task's cred (using prepare_cred())
2. always authorize any changes to the new struct cred based on the permissions of current_cred() (not the current transient state of the new cred).

While this may mean that certain transient_cred1->transient_cred2 states are allowed which otherwise wouldn't be allowed, the fact remains that current_cred() is allowed to transition to transient_cred2.

The reconstructed creds are applied to the task at the very end of the sys_restart call. This ensures that any objects which need to be re-created (file, socket, etc) are re-created using the creds of the task calling sys_restart - preventing an unpriv user from creating a privileged object, and ensuring that a root task can restart a process which had started out privileged, created some privileged objects, then dropped its privilege.

With these patches, the root user can restart checkpoint images (created by either hallyn or root) of user hallyn's tasks, resulting in a program owned by hallyn.

Plenty of bugs to be found, no doubt.

Changelog:
May 18: fix more refcounting: if (userns 5, uid 0) had
no active tasks or child user_namespaces, then it shouldn't exist at restart or it, its namespace, and its whole chain of creators will be leaked.
May 14: fix some refcounting:
1. a new user_ns needs a ref to remain pinned by its root user
2. current_user_ns needs an extra ref bc objhash drops two on restart
3. cred needs a ref for the real credentials bc commit_creds eats one ref.
May 13: folded in fix to userns refcounting.

Signed-off-by Serge E. Hallyn <serue-r/[email protected]>

---

(quoted diff) +7 -0

+{
+	struct ckpt_obj *obj = obj_find_by_ptr(ctx, ptr);
+	if (obj)
+		return obj->objref;
+	return 0;
+}
+
 static struct ckpt_obj *obj_find_by_objref(struct ckpt_ctx *ctx, int objref)
 {
 	struct hlist_head *h;

(quoted diff) +0 -0

 	return objref;
 }
 

checkpoint/process.c +1 -0

index d8d346b..74b411a 100644
@@ -17,6 +17,7 @@
 #include <linux/poll.h>
 #include <linux/nsproxy.h>
 #include <linux/utsname.h>
+#include <linux/user_namespace.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
 #include <linux/syscalls.h>

checkpoint/process.c +3 -0

+/* move this fn into kernel/sys.c next to group functions? */
+static int checkpoint_write_groupinfo(struct ckpt_ctx *ctx,
+					struct group_info *g)

checkpoint/process.c +1 -0

+
 /* dump the task_struct of a given task */
 static int checkpoint_task_struct(struct ckpt_ctx *ctx, struct task_struct *t)
 {
 	struct ckpt_hdr_task *h;

checkpoint/process.c +8 -0

+}
+
+/* move this code into kernel/cred.c and do proper perms checking of course */
+struct cred *restore_read_cred(struct ckpt_ctx *ctx)
+{
+	struct cred *cred;
+	struct ckpt_hdr_cred *h;
+	struct user_struct *user;

{

checkpoint/process.c +33 -0

 	struct ckpt_hdr_task *h;
 	struct task_struct *t = current;

@@ -365,8 +778,21 @@ static int restore_task_struct(struct ckpt_ctx *ctx)
 
 	memset(t->comm, 0, TASK_COMM_LEN);
 	ret = _ckpt_read_string(ctx, t->comm, h->task_comm_len);
+	if (ret < 0)
+		goto out;
 
 	/* FIXME: restore remaining relevant task_struct fields */
+
+	ret = 0;
+	*realcredp = ckpt_obj_fetch(ctx, h->cred_ref, CKPT_OBJ_CRED);
+	if (IS_ERR(*realcredp)) {
+		ret = PTR_ERR(*realcredp);
+		goto out;
+	}
+	*ecredp = ckpt_obj_fetch(ctx, h->ecred_ref, CKPT_OBJ_CRED);
+	if (IS_ERR(*ecredp))
+		ret = PTR_ERR(*ecredp);
+
  out:
 	ckpt_hdr_put(ctx, h);
 	return ret;

@@ -648,12 +1074,35 @@ static int restore_task_shared(struct ckpt_ctx *ctx)
 	return ret;
 }
 
+static int restore_creds(struct ckpt_ctx *ctx, struct cred *rcred,
+			struct cred *ecred)
+{
+	int ret;
+	const struct cred *old;
+
+	/* commit_creds will take one ref for the eff creds, but
+	 * expects us to hold a ref for the obj creds, so take a
+	 * ref here */
+	get_cred(rcred);
+	ret = commit_creds(rcred);
+	if (ret)
+		return ret;
+
+	if (ecred == rcred)
+		return 0;
+
+	old =  override_creds(ecred); /* override_creds otoh takes new ref */
+	put_cred(old);
+	return 0;

Quoted 4 lines

/* dump the task_struct of a given task */
static int checkpoint_task_struct(struct ckpt_ctx *ctx, struct task_struct *t) {

(quoted diff) +0 -0

 	struct ckpt_hdr_task *h;

Review

Quoted 10 lines

(quoted diff) +9 -0

 	int ret;
+	int realcred_ref, ecred_ref;
+
+	realcred_ref = checkpoint_obj(ctx, t->real_cred, CKPT_OBJ_CRED);
+	if (realcred_ref < 0)
+		return realcred_ref;
+
+	ecred_ref = checkpoint_obj(ctx, t->cred, CKPT_OBJ_CRED);
+	if (ecred_ref < 0)
+		return ecred_ref;

Is this safe even if the checkpointed task's state changes ? (e.g. unfrozen - yes, I know there is a patch in the works to prevent this; but if we ever want to checkpoint STOPPED tasks... for instance).

Would incrementing the refcount on t->{cred,real_cred} help ?

Quoted 19 lines · Oren Laadan

Quoted 4 lines

/* dump the task_struct of a given task */
static int checkpoint_task_struct(struct ckpt_ctx *ctx, struct task_struct *t) {

(quoted diff) +0 -0

 	struct ckpt_hdr_task *h;

Review

Quoted 10 lines

(quoted diff) +9 -0

 	int ret;
+	int realcred_ref, ecred_ref;
+
+	realcred_ref = checkpoint_obj(ctx, t->real_cred, CKPT_OBJ_CRED);
+	if (realcred_ref < 0)
+		return realcred_ref;
+
+	ecred_ref = checkpoint_obj(ctx, t->cred, CKPT_OBJ_CRED);
+	if (ecred_ref < 0)
+		return ecred_ref;

Is this safe even if the checkpointed task's state changes ? (e.g. unfrozen - yes, I know there is a patch in the works to prevent this; but if we ever want to checkpoint STOPPED tasks... for instance).

Quoted 1 line

Would incrementing the refcount on t->{cred,real_cred} help ?

Doesn't checkpoint_obj already do that through obj_new?

Quoted 20 lines · Serge E. Hallyn

Quoted 19 lines · Oren Laadan

Quoted 4 lines

/* dump the task_struct of a given task */
static int checkpoint_task_struct(struct ckpt_ctx *ctx, struct task_struct *t) {

(quoted diff) +0 -0

 	struct ckpt_hdr_task *h;

Review

Quoted 10 lines

(quoted diff) +9 -0

 	int ret;
+	int realcred_ref, ecred_ref;
+
+	realcred_ref = checkpoint_obj(ctx, t->real_cred, CKPT_OBJ_CRED);
+	if (realcred_ref < 0)
+		return realcred_ref;
+
+	ecred_ref = checkpoint_obj(ctx, t->cred, CKPT_OBJ_CRED);
+	if (ecred_ref < 0)
+		return ecred_ref;

Is this safe even if the checkpointed task's state changes ? (e.g. unfrozen - yes, I know there is a patch in the works to prevent this; but if we ever want to checkpoint STOPPED tasks... for instance).

Quoted 4 lines

Quoted 1 line

Would incrementing the refcount on t->{cred,real_cred} help ?

Doesn't checkpoint_obj already do that through obj_new?

No, it does not. There is a (potentially long) window of opportunity between the callback invoked from checkpoint_obj() - where the pointer is used, and when checkpoint_obj() later takes the extra reference.

See for comparison checkpoint_mm_obj(), it safely grabs the task->mm (with a reference) around the invocation of checkpoint_obj().

Review

Quoted 4 lines

/* read the task_struct into the current task */

(quoted diff) +2 -1

-static int restore_task_struct(struct ckpt_ctx *ctx)
+static int restore_task_struct(struct ckpt_ctx *ctx, struct cred **realcredp,
+				struct cred **ecredp)

				^^^^^^^^^^^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^^

This probably belongs to restore_task_shared() ?

Quoted 6 lines · Oren Laadan

Review

Quoted 4 lines

/* read the task_struct into the current task */

(quoted diff) +2 -1

-static int restore_task_struct(struct ckpt_ctx *ctx)
+static int restore_task_struct(struct ckpt_ctx *ctx, struct cred **realcredp,
+				struct cred **ecredp)

				^^^^^^^^^^^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

This probably belongs to restore_task_shared() ?

Why? The task->cred and task->ecred are task properties, so their reference should be stored in the ckpt_hdr_task->cred_ref and ckpt_hdr_task->ecred_ref, no?

Review

Quoted 8 lines

/* read the entire state of the current task */
int restore_task(struct ckpt_ctx *ctx)
{

(quoted diff) +2 -1

 	int ret;
+	struct cred *realcred, *ecred;
 
-	ret = restore_task_struct(ctx);
+	ret = restore_task_struct(ctx, &realcred, &ecred);

Actually, this is one of several cases where we need to restore some resources but only apply it to a process at the end of its restart.

Another example would be restoring pending signals and the blocked signal mask in the future.

I suggest that we keep a pointer on the task_struct to a structure that will hold all that do-later work. The structure can encapsulate the pending work either explicitly - e.g. a struct with fields like realcred, ecred, signal mask, etc... - or implicitly, by reusing the deferqueue framework, per task.

Actually, that pointer can be kept on the ckpt_ctx structure, to be used by the current-restarting-task only.

Review int restore_task(struct ckpt_ctx *ctx) · line 1125

Quoted 10 lines

(quoted diff) +3 -0

 	ckpt_debug("ret %d\n", ret);
 	if (ret < 0)
 		goto out;

@@ -671,6 +1120,10 @@ int restore_task(struct ckpt_ctx *ctx)
 		goto out;
 	ret = restore_cpu(ctx);
 	ckpt_debug("cpu: ret %d\n", ret);
+	if (ret < 0)
+		goto out;
+	ret = restore_creds(ctx, realcred, ecred);

... and this would then be called from a restore_task_finalize() function explicitly or implicitly by deferqueue_run().

Quoted 23 lines · Oren Laadan

Review

Quoted 8 lines

/* read the entire state of the current task */
int restore_task(struct ckpt_ctx *ctx)
{

(quoted diff) +2 -1

 	int ret;
+	struct cred *realcred, *ecred;
 
-	ret = restore_task_struct(ctx);
+	ret = restore_task_struct(ctx, &realcred, &ecred);

Actually, this is one of several cases where we need to restore some resources but only apply it to a process at the end of its restart.

Another example would be restoring pending signals and the blocked signal mask in the future.

I suggest that we keep a pointer on the task_struct to a structure that will hold all that do-later work. The structure can encapsulate the pending work either explicitly - e.g. a struct with fields like realcred, ecred, signal mask, etc... - or implicitly, by reusing the deferqueue framework, per task.

Actually, that pointer can be kept on the ckpt_ctx structure, to be used by the current-restarting-task only.

Quoted 12 lines

Review int restore_task(struct ckpt_ctx *ctx) · line 1125

Quoted 10 lines

(quoted diff) +3 -0

 	ckpt_debug("ret %d\n", ret);
 	if (ret < 0)
 		goto out;

@@ -671,6 +1120,10 @@ int restore_task(struct ckpt_ctx *ctx)
 		goto out;
 	ret = restore_cpu(ctx);
 	ckpt_debug("cpu: ret %d\n", ret);
+	if (ret < 0)
+		goto out;
+	ret = restore_creds(ctx, realcred, ecred);

... and this would then be called from a restore_task_finalize() function explicitly or implicitly by deferqueue_run().

deferqueue_run() won't do, since that's done only once for the whole container, and we (as you say above) want to reuse one set of fields in the ckpt_ctx for each task's sys_restart() run.

I'll go ahead and put fields in the ckpt_ctx this time around and use those, but won't go further right now as I'd be overgeneralizing before we have the signals and such work done. When we do that, we can move the restore_creds() fn if appropriate.

-serge