credentials c/r: Introduction

An application checkpoint image will store capability sets (and the bounding set) as __u64s.  Define checkpoint and restart functions to translate between those and kernel_cap_t's.

Define a common function do_capset_tocred() which applies capability set changes to a passed-in struct cred.

The restore function uses do_capset_tocred() to apply the restored capabilities to the struct cred being crafted, subject to the current task's (task executing sys_restart()) permissions.

Changelog:
       May 28: add helpers to c/r securebits

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

---
 include/linux/capability.h |    7 +++
 kernel/capability.c        |   94 +++++++++++++++++++++++++++++++++++++------ security/commoncap.c       |   51 ++++++++++++++++--------

 3 files changed, 122 insertions(+), 30 deletions(-)

include/linux/capability.h +7 -0

index c302110..b3853ca 100644
@@ -536,6 +536,13 @@ extern const kernel_cap_t __cap_empty_set;
  extern const kernel_cap_t __cap_full_set;
  extern const kernel_cap_t __cap_init_eff_set;
 
+extern void checkpoint_save_cap(__u64 *dest, kernel_cap_t src);
+struct cred;
+extern int checkpoint_restore_cap(__u64 e, __u64 i, __u64 p, __u64 x,
+                               struct cred *cred);
+extern void checkpoint_save_securebits(unsigned *, unsigned);
+extern int checkpoint_restore_securebits(unsigned, struct cred *);
+
  /**
  * has_capability - Determine if a task has a superior capability available
  * @t: The task in question

kernel/capability.c +81 -13

index 4e17041..d2c9bb3 100644
@@ -217,6 +217,45 @@ SYSCALL_DEFINE2(capget, cap_user_header_t, header, cap_user_data_t, dataptr)
        return ret;
  }
 
+static int do_capset_tocred(kernel_cap_t *effective, kernel_cap_t *inheritable,
+                       kernel_cap_t *permitted, struct cred *new)
+{
+       int ret;
+
+       ret = security_capset(new, current_cred(),
+                             effective, inheritable, permitted);
+       if (ret < 0)
+               return ret;
+
+       /*
+        * for checkpoint-restart, do we want to wait until end of restart?
+        * not sure we care */
+       audit_log_capset(current->pid, new, current_cred());
+
+       return 0;
+}
+
+static int do_capset(kernel_cap_t *effective, kernel_cap_t *inheritable,
+                       kernel_cap_t *permitted)
+{
+       struct cred *new;
+       int ret;
+
+       new = prepare_creds();
+       if (!new)
+               return -ENOMEM;
+
+       ret = do_capset_tocred(effective, inheritable, permitted, new);
+       if (ret < 0)
+               goto error;
+
+       return commit_creds(new);
+
+error:
+       abort_creds(new);
+       return ret;
+}
+
  /**
  * sys_capset - set capabilities for a process or (*) a group of processes
  * @header: pointer to struct that contains capability version and

@@ -240,7 +279,6 @@ SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
        struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
        unsigned i, tocopy;
        kernel_cap_t inheritable, permitted, effective;
-       struct cred *new;
        int ret;
        pid_t pid;
 

@@ -271,22 +309,52 @@ SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
                i++;
        }
 
-       new = prepare_creds();
-       if (!new)
-               return -ENOMEM;
+       return do_capset(&effective, &inheritable, &permitted);
 
-       ret = security_capset(new, current_cred(),
-                             &effective, &inheritable, &permitted);
-       if (ret < 0)
-               goto error;
+}
 
-       audit_log_capset(pid, new, current_cred());
 
-       return commit_creds(new);
+void checkpoint_save_cap(__u64 *dest, kernel_cap_t src)
+{
+       *dest = src.cap[0] | (src.cap[1] << sizeof(__u32));
+}
 
-error:
-       abort_creds(new);
-       return ret;
+static void do_capbset_drop(struct cred *cred, int cap)
+{
+       cap_lower(cred->cap_bset, cap);
+}
+
+int checkpoint_restore_cap(__u64 newe, __u64 newi, __u64 newp, __u64 newx,
+                       struct cred *cred)
+{
+       kernel_cap_t effective, inheritable, permitted, bset;
+       int may_dropbcap = capable(CAP_SETPCAP);
+       int ret, i;
+
+       effective.cap[0] = newe;
+       effective.cap[1] = (newe >> sizeof(__u32));
+       inheritable.cap[0] = newi;
+       inheritable.cap[1] = (newi >> sizeof(__u32));
+       permitted.cap[0] = newp;
+       permitted.cap[1] = (newp >> sizeof(__u32));
+       bset.cap[0] = newx;
+       bset.cap[1] = (newx >> sizeof(__u32));
+
+       ret = do_capset_tocred(&effective, &inheritable, &permitted, cred);
+       if (ret < 0)
+               return ret;
+
+       for (i = 0; i < CAP_LAST_CAP; i++) {
+               if (cap_raised(bset, i))
+                       continue;
+               if (!cap_raised(current_cred()->cap_bset, i))
+                       continue;
+               if (!may_dropbcap)
+                       return -EPERM;
+               do_capbset_drop(cred, i);
+       }
+
+       return 0;
  }
 
  /**

security/commoncap.c +22 -0

index beac025..8054a07 100644
@@ -804,6 +804,28 @@ int cap_task_setnice (struct task_struct *p, int nice)
  }
  #endif
 
+int cap_set_securebits(struct cred *new, unsigned securebits)
+{
+       if ((((new->securebits & SECURE_ALL_LOCKS) >> 1)
+            & (new->securebits ^ securebits))                          /*[1]*/
+           || ((new->securebits & SECURE_ALL_LOCKS & ~securebits))     /*[2]*/
+           || (securebits & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS))     /*[3]*/
+           || (cap_capable(current, current_cred(), CAP_SETPCAP,
+                           SECURITY_CAP_AUDIT) != 0)                   /*[4]*/
+               /*
+                * [1] no changing of bits that are locked
+                * [2] no unlocking of locks
+                * [3] no setting of unsupported bits
+                * [4] doing anything requires privilege (go read about
+                *     the "sendmail capabilities bug")
+                */
+           )
+               /* cannot change a locked bit */
+               return -EPERM;
+       new->securebits = securebits;
+       return 0;
+}
+
  /**
  * cap_task_prctl - Implement process control functions for this security module
  * @option: The process control function requested

@@ -861,24 +883,9 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
         * capability-based-privilege environment.
         */

       case PR_SET_SECUREBITS:
-               error = -EPERM;
-               if ((((new->securebits & SECURE_ALL_LOCKS) >> 1)
-                    & (new->securebits ^ arg2))                        /*[1]*/

-                   || ((new->securebits & SECURE_ALL_LOCKS & ~arg2))   /*[2]*/
-                   || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS))   /*[3]*/
-                   || (cap_capable(current, current_cred(), CAP_SETPCAP,

-                                   SECURITY_CAP_AUDIT) != 0)           /*[4]*/
-                       /*
-                        * [1] no changing of bits that are locked
-                        * [2] no unlocking of locks
-                        * [3] no setting of unsupported bits
-                        * [4] doing anything requires privilege (go read about
-                        *     the "sendmail capabilities bug")
-                        */
-                   )
-                       /* cannot change a locked bit */
+               error = cap_set_securebits(new, arg2);
+               if (error)
                       goto error;
-               new->securebits = arg2;
               goto changed;

       case PR_GET_SECUREBITS:

security/commoncap.c +10 -1

@@ -921,6 +928,16 @@ error:
        return error;
  }
 
+void checkpoint_save_securebits(unsigned *b, unsigned cred_securebits)
+{
+       *b = cred_securebits;
+}
+
+int checkpoint_restore_securebits(unsigned b, struct cred *cred)
+{
+       return cap_set_securebits(cred, b);
+}
+
  /**
  * cap_syslog - Determine whether syslog function is permitted
  * @type: Function requested
--

1.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'm not sure I'm too happy with hard coding the 64-bitness of capability sets. It may well be a very long time before we increase their size, but couldn't you prepare for that with some reference to the prevailing magic numbers for the current ABI representation?

Serge,

I'm not sure I'm too happy with hard coding the 64-bitness of capability sets. It may well be a very long time before we increase their size, but couldn't you prepare for that with some reference to the prevailing magic numbers for the current ABI representation?

Also, the use of 'error' as both a variable and a goto destination looks a little confusing.

Quoted 6 lines · Andrew G. Morgan

Serge,

I'm not sure I'm too happy with hard coding the 64-bitness of capability sets. It may well be a very long time before we increase their size, but couldn't you prepare for that with some reference to the prevailing magic numbers for the current ABI representation?

Hmm, ok.  I figured since the c/r code was in capability.h it would be obvious that going past 64-bit would mean a new checkpoint image format.  I can see where that's silly...

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

Quoted 2 lines

Also, the use of 'error' as both a variable and a goto destination looks a little confusing.

Ok will change.

Did you see any problems with the way I authorize a task's resetting of capabilities at sys_restart()?

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 13 lines · Serge E. Hallyn

Quoted 6 lines · Andrew G. Morgan

Serge,

I'm not sure I'm too happy with hard coding the 64-bitness of capability sets. It may well be a very long time before we increase their size, but couldn't you prepare for that with some reference to the prevailing magic numbers for the current ABI representation?

Hmm, ok.  I figured since the c/r code was in capability.h it would be obvious that going past 64-bit would mean a new checkpoint image format.  I can see where that's silly...

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I guess I'm not really well up on what the plans are for checkpoint images. Is there some sort of version control/signature/checksum to protect a kernel from loading an image that has been hacked to modify the privilege it was running with when the checkpoint was created?

Quoted 7 lines

Quoted 2 lines

Also, the use of 'error' as both a variable and a goto destination looks a little confusing.

Ok will change.

Did you see any problems with the way I authorize a task's resetting of capabilities at sys_restart()?

[See above.] Is there a mailing list or something I can lurk on to get up to speed on what is being intended?

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

Quoted 5 lines

I guess I'm not really well up on what the plans are for checkpoint images. Is there some sort of version control/signature/checksum to protect a kernel from loading an image that has been hacked to modify the privilege it was running with when the checkpoint was created?

No.  One day we expect there will be TPM-signing of checkpoint images, but that will be up to userspace to properly exploit.  So if userspace wants to enforce a certain flow control to prevent an unprivileged user from modifying a checkpoint image (which of course it does), then it should set up DAC and/or MAC to enforce that.

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */
} checkpoint_caps_t;

Quoted 11 lines · Andrew G. Morgan

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */ } checkpoint_caps_t;

Sigh - Did a patch this way, but the problem is userspace needs to be able to parse the checkpoint image, so it needs to know what this struct looks like.  So if I put it the struct definition include/linux/capability.h, I run into a whole new set of problems trying to compile a userspace program to do a sys_restart().

So I went part-way to what you suggested in the patchset I'm about to send out (please see patch 6/8).  I think the caps code does look nicer in this new version.

Quoted 13 lines · Serge E. Hallyn

Quoted 11 lines · Andrew G. Morgan

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */ } checkpoint_caps_t;

Quoted 5 lines

Sigh - Did a patch this way, but the problem is userspace needs to be able to parse the checkpoint image, so it needs to know what this struct looks like.  So if I put it the struct definition include/linux/capability.h, I run into a whole new set of problems trying to compile a userspace program to do a sys_restart().

Does the user space app need to be able to modify the data in some way? It seems like embedding a length with the structure or something might simplify such a user space dependency.

Quoted 3 lines

So I went part-way to what you suggested in the patchset I'm about to send out (please see patch 6/8).  I think the caps code does look nicer in this new version.

Better, but I remain concerned that the code looks hard to maintain when structured this way.

Quoted 12 lines · Andrew G. Morgan

Quoted 11 lines · Serge E. Hallyn

Quoted 10 lines · Andrew G. Morgan

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */
} checkpoint_caps_t;

Quoted 8 lines

Quoted 5 lines

Sigh - Did a patch this way, but the problem is userspace needs to be able to parse the checkpoint image, so it needs to know what this struct looks like. So if I put it the struct definition include/linux/capability.h, I run into a whole new set of problems trying to compile a userspace program to do a sys_restart().

Does the user space app need to be able to modify the data in some way? It seems like embedding a length with the structure or something might simplify such a user space dependency.

Hmm, I suppose I could do something like define struct ckpt_capabilities in capabilities.h, then in checkpoint_hdr.h do

struct ckpt_capabilities;

struct ckpt_cap_dummy {
	__u64 dummies[9];
};

struct ckpt_hdr_cred {
	...
	union {
		struct ckpt_capabilities r;
		struct ckpt_cap_dummy d;
	} caps;
};

with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d). Ugly, but should suit everyone?

Quoted 5 lines

Quoted 3 lines

So I went part-way to what you suggested in the patchset I'm about to send out (please see patch 6/8). I think the caps code does look nicer in this new version.

Better, but I remain concerned that the code looks hard to maintain when structured this way.

Why exactly? Just having the struct defined in checkpoint_hdr.h? Or is there something else I'm unwittingly doing?

thanks,
-serge

Quoted 20 lines · Andrew G. Morgan

Quoted 13 lines · Serge E. Hallyn

Quoted 11 lines · Andrew G. Morgan

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */ } checkpoint_caps_t;

Quoted 5 lines

Sigh - Did a patch this way, but the problem is userspace needs to be able to parse the checkpoint image, so it needs to know what this struct looks like.  So if I put it the struct definition include/linux/capability.h, I run into a whole new set of problems trying to compile a userspace program to do a sys_restart().

Quoted 3 lines

Does the user space app need to be able to modify the data in some way? It seems like embedding a length with the structure or something might simplify such a user space dependency.

Hmm, I suppose I could do something like define struct ckpt_capabilities in capabilities.h, then in checkpoint_hdr.h do

struct ckpt_capabilities;
struct ckpt_cap_dummy {
       __u64 dummies[9];
};

struct ckpt_hdr_cred {
       ...
       union {
               struct ckpt_capabilities r;
               struct ckpt_cap_dummy d;
       } caps;
};

with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d).  Ugly, but should suit everyone?

Quoted 7 lines

Quoted 4 lines

So I went part-way to what you suggested in the patchset I'm about to send out (please see patch 6/8).  I think the caps code does look nicer in this new version.

Better, but I remain concerned that the code looks hard to maintain when structured this way.

Why exactly?  Just having the struct defined in checkpoint_hdr.h?  Or is there something else I'm unwittingly doing?

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 2 lines

with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d).  Ugly, but should suit everyone?

could the checkpointing code check the return value for cap_checkpoint_restore() and fail the restore if it returned an error?

[I'm sorry if I'm flogging a dead horse. I'm actually really excited by this functionality! :-) ]

Comments inline...

Quoted 29 lines · Serge E. Hallyn

Quoted 12 lines · Andrew G. Morgan

Quoted 11 lines · Serge E. Hallyn

Quoted 10 lines · Andrew G. Morgan

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */
} checkpoint_caps_t;

Quoted 8 lines

Quoted 5 lines

Sigh - Did a patch this way, but the problem is userspace needs to be able to parse the checkpoint image, so it needs to know what this struct looks like. So if I put it the struct definition include/linux/capability.h, I run into a whole new set of problems trying to compile a userspace program to do a sys_restart().

Does the user space app need to be able to modify the data in some way? It seems like embedding a length with the structure or something might simplify such a user space dependency.

Hmm, I suppose I could do something like define struct ckpt_capabilities in capabilities.h, then in checkpoint_hdr.h do

struct ckpt_capabilities;

struct ckpt_cap_dummy {
       __u64 dummies[9];
};

Quoted 7 lines

struct ckpt_hdr_cred {
       ...
       union {
               struct ckpt_capabilities r;
               struct ckpt_cap_dummy d;
       } caps;
};

Yes, something like this, but perhaps:

    struct ckpt_caps_part_s {
       int length; /* = sizeof(struct ckpt_capabilities) */
       cap_ckpt_t data;
    } caps;

and then the generic checkpoint code would do:

   #include <linux/capabilities.h>
   caps.ckpt_capabilities_length = cap_checkpoint_save(&caps.data);
   [...]
   cap_checkpoint_restore(caps.length, &caps.caps.data);

and the capability code could opaquely deal with the details.

The reason I think this is more maintainable is that its clear (to the capability code) what is being check-pointed and, conversely, for the checkpoint code it is abstracted with the responsibility for detailed state decisions elsewhere in the kernel.

I suspect I don't understand the user space code issue sufficiently. But if, for some reason, the user space source code is unable to include the definition of cap_ckpt_t, it should be clear that parsing this type of data structure, given that offsets are embedded in it, should be straightforward.

Quoted 2 lines

with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d). Ugly, but should suit everyone?

could the checkpointing code check the return value for cap_checkpoint_restore() and fail the restore if it returned an error?

Cheers

Andrew

Quoted 15 lines

Quoted 5 lines

Quoted 3 lines

So I went part-way to what you suggested in the patchset I'm about to send out (please see patch 6/8). I think the caps code does look nicer in this new version.

Better, but I remain concerned that the code looks hard to maintain when structured this way.

Why exactly? Just having the struct defined in checkpoint_hdr.h? Or is there something else I'm unwittingly doing?

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 64 lines · Andrew G. Morgan

[I'm sorry if I'm flogging a dead horse. I'm actually really excited by this functionality! :-) ]

Comments inline...

Quoted 29 lines · Serge E. Hallyn

Quoted 12 lines · Andrew G. Morgan

Quoted 11 lines · Serge E. Hallyn

Quoted 10 lines · Andrew G. Morgan

Quoted 2 lines · Serge E. Hallyn

Quoted 2 lines

Quoted 2 lines

I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that suffice?

I can't speak for other subsystems, but it seems to me as if for the capabilities, I'd want to create something like this in include/linux/capabilities.h

typedef struct checkpoint_caps_s {
   /* what goes in here is the capability code's business */ } checkpoint_caps_t;

Quoted 8 lines

Quoted 5 lines

Sigh - Did a patch this way, but the problem is userspace needs to be able to parse the checkpoint image, so it needs to know what this struct looks like.  So if I put it the struct definition include/linux/capability.h, I run into a whole new set of problems trying to compile a userspace program to do a sys_restart().

Does the user space app need to be able to modify the data in some way? It seems like embedding a length with the structure or something might simplify such a user space dependency.

Hmm, I suppose I could do something like define struct ckpt_capabilities in capabilities.h, then in checkpoint_hdr.h do

struct ckpt_capabilities;
struct ckpt_cap_dummy {
       __u64 dummies[9];
};

Quoted 7 lines

struct ckpt_hdr_cred {
       ...
       union {
               struct ckpt_capabilities r;
               struct ckpt_cap_dummy d;
       } caps;
};

Yes, something like this, but perhaps:

    struct ckpt_caps_part_s {
       int length; /* = sizeof(struct ckpt_capabilities) */ cap_ckpt_t data;
    } caps;

and then the generic checkpoint code would do:

   #include <linux/capabilities.h> caps.ckpt_capabilities_length = cap_checkpoint_save(&caps.data); [...]
   cap_checkpoint_restore(caps.length, &caps.caps.data);

and the capability code could opaquely deal with the details.

The reason I think this is more maintainable is that its clear (to the capability code) what is being check-pointed and, conversely, for the checkpoint code it is abstracted with the responsibility for detailed state decisions elsewhere in the kernel.

Quoted 6 lines

I suspect I don't understand the user space code issue sufficiently. But if, for some reason, the user space source code is unable to include the definition of cap_ckpt_t, it should be clear that parsing this type of data structure, given that offsets are embedded in it, should be straightforward.

As I said here:
https://lists.linux-foundation.org/pipermail/containers/2009-June/018288.html

Userspace needs to understand what's in the image to be able to provide debug/info about the image, and to be able to convert images from older kernel version to newer (or even vice versa if one insists).

Oren.

Quoted 26 lines

Quoted 2 lines

with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d).  Ugly, but should suit everyone?

could the checkpointing code check the return value for cap_checkpoint_restore() and fail the restore if it returned an error?

Cheers

Andrew

Quoted 15 lines

Quoted 5 lines

Quoted 3 lines

So I went part-way to what you suggested in the patchset I'm about to send out (please see patch 6/8).  I think the caps code does look nicer in this new version.

Better, but I remain concerned that the code looks hard to maintain when structured this way.

Why exactly?  Just having the struct defined in checkpoint_hdr.h?  Or is there something else I'm unwittingly doing?

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.html

You guys are well on the road to having something functional. I do not want to get in the way of that.

What I'm trying to question is the way the code is abstracted. My sense is that its not very friendly to the subsystems being checkpointed. I could offer an alternative patch for the capabilities code, but if it breaks some invisible constraint I'd rather you tell me about it first.

There are three sets of code in these changes:

1. the capabilities code (but I think I'm arguing about that because
it is an example I'm familiar with and this probably applies to other subsystems in the kernel)
2. the glue code in the kernel that implements sys_restore() etc.
3. the user space code

What seems to have happened with these patches is that the glue code and the user space code have knowledge of each other and things are written to make an API/ABI that they can agree on. (Part of this is evidenced by the u64 reference to capabilities and non-use of the more natural kernel_cap_t data that the kernel itself uses.)

It seems to me that a more natural abstraction is that 1 and 3 know about each other (actually 3 knows about how 1 chooses to define things), and 2 is a generic data packager interface.

This should be functionally equivalent, but my belief is an implementation based on the current abstraction will be broken more often because some subsystem (eg. 1) evolves its critical state and the corresponding (2+3)'s relationship is too confusing/constraining for the kernel subsystem developer to keep in sync (someone else's problem). And subtle problems of ignored state in checkpoints can be really hard to debug!

Bottom line. Don't wait for me to Ack this change. I intend to lurk and if I see a way to "clean it up", I'll offer a patch, which you can of course reject... But don't wait on me.

Quoted 23 lines · Andrew G. Morgan

You guys are well on the road to having something functional. I do not want to get in the way of that.

What I'm trying to question is the way the code is abstracted. My sense is that its not very friendly to the subsystems being checkpointed. I could offer an alternative patch for the capabilities code, but if it breaks some invisible constraint I'd rather you tell me about it first.

There are three sets of code in these changes:

  1. the capabilities code (but I think I'm arguing about that because it is an example I'm familiar with and this probably applies to other subsystems in the kernel)
  2. the glue code in the kernel that implements sys_restore() etc.
  3. the user space code

What seems to have happened with these patches is that the glue code and the user space code have knowledge of each other and things are written to make an API/ABI that they can agree on. (Part of this is evidenced by the u64 reference to capabilities and non-use of the more natural kernel_cap_t data that the kernel itself uses.)

Quoted 3 lines

It seems to me that a more natural abstraction is that 1 and 3 know about each other (actually 3 knows about how 1 chooses to define things), and 2 is a generic data packager interface.

One reason (3) and (2) need to know about each is other is bc userspace needs to read the checkpoint image enough to read the task states, clone off suitable children, and then each child calls sys_restart().  So to this end, there are certainly details which userspace doesn't need to be able to parse.

The other reason is that userspace will expect to be able to make changes to the checkpoint file.  For instance, if restarting a process from one kernel version with 34 capabilities defined, on a kernel version with 35 defined, then userspace may want to fill in an extra cap.

But I strongly agree with your concern that subsystems will get out of sync with the checkpoint code.

If we just move the struct ckpt_capabilities definition into include/linux/capabilities.h, does that suffice?  The c/r userspace could then do something like

cat > ckpthdr.sed << EOF
/^[^ \t]*struct ckpt[^\n]*{/,/}/p
EOF
cat /usr/include/linux/*.h /usr/include/asm/*.h | sed -n -f ckpthdr.sed \
       > checkpoint_headers.h

Now you mention using kernel_cap_t's...  we can go partway down that road, but the inherent problem is serializing the relevant data so it can be streamed to some file.  So I think it's better if the subsystems are required to specify a useful format (like struct ckpt_capabilities) and define the checkpoint/restart helpers to serialize data into the struct.  We can try and get cute with dual mirrored struct defs, one which is defined in terms the caps code can understand, and one defined in more arch-independent terms (which is why we need __u64s and packed, for instance). But that seems more fragile than having clear and simple requirements for the $SUBYSTEM_checkpoint() and $SUBSYSTEM_restart() helpers.

(I'll wait to hear whether you think I'm on the right track before reworking my patch)

Quoted 11 lines

This should be functionally equivalent, but my belief is an implementation based on the current abstraction will be broken more often because some subsystem (eg. 1) evolves its critical state and the corresponding (2+3)'s relationship is too confusing/constraining for the kernel subsystem developer to keep in sync (someone else's problem). And subtle problems of ignored state in checkpoints can be really hard to debug!

Bottom line. Don't wait for me to Ack this change. I intend to lurk and if I see a way to "clean it up", I'll offer a patch, which you can of course reject... But don't wait on me.

Thanks, it's much appreciated.  But please don't hold back, as your points are imo valid, and we need to work out how to address them.

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

Now you mention using kernel_cap_t's...  we can go partway down that road, but the inherent problem is serializing the relevant data so it can be streamed to some file.  So I think it's better if the subsystems are required to specify a useful format (like struct ckpt_capabilities) and define

Quoted 8 lines

the checkpoint/restart helpers to serialize data into the struct.  We can try and get cute with dual mirrored struct defs, one which is defined in terms the caps code can understand, and one defined in more arch-independent terms (which is why we need __u64s and packed, for instance). But that seems more fragile than having clear and simple requirements for the $SUBYSTEM_checkpoint() and $SUBSYSTEM_restart() helpers.

I like this $SUBSYSTEM_checkpoint() etc. thing.

I like the ckpthdr.sed thing. I think a similar rule could be used to generate the calls to the list of $SUBSYSTEM_checkpoint() functions.

For serialization, could a kernel "gcc -E checkpoint-headers.h > this-kernel-checkpoint-file.h" build rule be enough?

Quoting Andrew G. Morgan ([email protected]):
...

Quoted 14 lines

Quoted 13 lines

Now you mention using kernel_cap_t's...  we can go partway down that road, but the inherent problem is serializing the relevant data so it can be streamed to some file.  So I think it's better if the subsystems are required to specify a useful format (like struct ckpt_capabilities) and define the checkpoint/restart helpers to serialize data into the struct.  We can try and get cute with dual mirrored struct defs, one which is defined in terms the caps code can understand, and one defined in more arch-independent terms (which is why we need __u64s and packed, for instance). But that seems more fragile than having clear and simple requirements for the $SUBYSTEM_checkpoint() and $SUBSYSTEM_restart() helpers.

Quoted 4 lines

I like this $SUBSYSTEM_checkpoint() etc. thing.

I like the ckpthdr.sed thing. I think a similar rule could be used to generate the calls to the list of $SUBSYSTEM_checkpoint() functions.

Sorry, I don't follow. Could you say a bit more about this?

Quoted 3 lines

For serialization, could a kernel "gcc -E checkpoint-headers.h > this-kernel-checkpoint-file.h" build rule be enough?

Again, I don't follow. Do you mean to turn something like kernel_cap_t into something like struct ckpt_capabilities?

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 16 lines · Serge E. Hallyn

Quoting Andrew G. Morgan ([email protected]):
...

Quoted 14 lines

Quoted 13 lines

Now you mention using kernel_cap_t's...  we can go partway down that road, but the inherent problem is serializing the relevant data so it can be streamed to some file.  So I think it's better if the subsystems are required to specify a useful format (like struct ckpt_capabilities) and define the checkpoint/restart helpers to serialize data into the struct.  We can try and get cute with dual mirrored struct defs, one which is defined in terms the caps code can understand, and one defined in more arch-independent terms (which is why we need __u64s and packed, for instance). But that seems more fragile than having clear and simple requirements for the $SUBYSTEM_checkpoint() and $SUBSYSTEM_restart() helpers.

Quoted 6 lines

Quoted 4 lines

I like this $SUBSYSTEM_checkpoint() etc. thing.

I like the ckpthdr.sed thing. I think a similar rule could be used to generate the calls to the list of $SUBSYSTEM_checkpoint() functions.

Sorry, I don't follow. Could you say a bit more about this?

See sketch below.

Quoted 6 lines

Quoted 3 lines

For serialization, could a kernel "gcc -E checkpoint-headers.h > this-kernel-checkpoint-file.h" build rule be enough?

Again, I don't follow. Do you mean to turn something like kernel_cap_t into something like struct ckpt_capabilities?

Yes, but without manual effort.

To return the the 1,2,3 breakdown. I think it should be possible for 1 to provide code like this:

$SUBSYSTEM_header.h:

  struct ckpt_$SUBSYSTEM_s {
     kernel_foo_t a, b, c;

};

$SUBSYSTEM_code.c:
void $SUBSYSTEM_checkpoint(struct *storage_ptr)
{
}

  int $SUBSYSTEM_restore(struct *storage_ptr)
  {
  }

Serge,

I'm not sure I'm too happy with hard coding the 64-bitness of capability sets. It may well be a very long time before we increase their size, but couldn't you prepare for that with some reference to the prevailing magic numbers for the current ABI representation?

Also, the use of 'error' as both a variable and a goto destination looks a little confusing.

Quoted 6 lines · Andrew G. Morgan

Serge,

I'm not sure I'm too happy with hard coding the 64-bitness of capability sets. It may well be a very long time before we increase their size, but couldn't you prepare for that with some reference to the prevailing magic numbers for the current ABI representation?

Is the appended, updated version of the patch ok?

Quoted 2 lines

Also, the use of 'error' as both a variable and a goto destination looks a little confusing.

Ok, but that was in the original code. I can send a separate patch for mainline to change that, but it's not hurting anything at the moment so not sure it's worth it?

-serge

From aa72f022fb5788ab46658e6eb94eaf18e8c6568a Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <[email protected]>
Date: Mon, 11 May 2009 09:44:42 -0400
Subject: [PATCH 5/9] cr: capabilities: define checkpoint and restore fns

An application checkpoint image will store capability sets (and the bounding set) as __u64s. Define checkpoint and restart functions to translate between those and kernel_cap_t's.

Define a common function do_capset_tocred() which applies capability set changes to a passed-in struct cred.

The restore function uses do_capset_tocred() to apply the restored capabilities to the struct cred being crafted, subject to the current task's (task executing sys_restart()) permissions.

Changelog:
Jun 01: Add commented BUILD_BUG_ON() to point out that the
current implementation depends on 64-bit capabilities. (Andrew Morgan and Alexey Dobriyan).
May 28: add helpers to c/r securebits

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

---
 include/linux/capability.h |    7 +++
 kernel/capability.c        |  102 ++++++++++++++++++++++++++++++++++++++------
 security/commoncap.c       |   51 +++++++++++++++-------
 3 files changed, 130 insertions(+), 30 deletions(-)

include/linux/capability.h +0 -0

index c302110..b3853ca 100644

Quoted 3 lines · Serge E. Hallyn

Create /proc/userns, which prints out all user namespaces. It prints the address of the user_ns itself, the uid and userns address of the user who created it, and the reference count.

kernel/user_namespace.c void free_user_ns(struct kref *kref) · line 127

Quoted 8 lines

kernel/user_namespace.c +8 -0

+static int proc_userns_show(struct seq_file *m, void *v)
+{
+	struct user_namespace *ns = v;
+	seq_printf(m, "userns %p creator (uid %d ns %p) count %d\n",
+		(void *)ns, ns->creator->uid, (void *) ns->creator->user_ns,
+		atomic_read(&ns->kref.refcount));
+	return 0;
+}

Kernel shouldn't expose location of kernel objects to userspace.