[Devel] Re: [PATCH 1/3] cr: add generic LSM c/r support (v4)
Oren Laadan
orenl at librato.com
Fri Oct 2 13:57:51 PDT 2009
Serge E. Hallyn wrote:
> (wasn't versioning the patchsets before, so randomly pick 4 as
> the version for this patchset...)
>
> Documentation/checkpoint/readme.txt begins:
> """
> Application checkpoint/restart is the ability to save the state
> of a running application so that it can later resume its execution
> from the time at which it was checkpointed.
> """
>
[...]
> Kernel interfaces
> =================
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index 1eeb557..6722035 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -25,6 +25,7 @@
> #include <linux/magic.h>
> #include <linux/hrtimer.h>
> #include <linux/deferqueue.h>
> +#include <linux/security.h>
> #include <linux/checkpoint.h>
> #include <linux/checkpoint_hdr.h>
>
> @@ -351,6 +352,16 @@ static int checkpoint_write_header(struct ckpt_ctx *ctx)
> if (ret < 0)
> return ret;
>
> + memset(ctx->lsm_name, 0, SECURITY_NAME_MAX + 1);
> + strlcpy(ctx->lsm_name, security_get_lsm_name(), SECURITY_NAME_MAX + 1);
> + ret = ckpt_write_buffer(ctx, ctx->lsm_name, SECURITY_NAME_MAX + 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = security_checkpoint_header(ctx);
> + if (ret < 0)
> + return ret;
> +
This is actually a case for a 'container-global' section that would
appear after the header and before the rest of the image. (Would be
useful also for network namespaces).
> return checkpoint_write_header_arch(ctx);
> }
>
> diff --git a/checkpoint/files.c b/checkpoint/files.c
> index 27e29a0..570facb 100644
> --- a/checkpoint/files.c
> +++ b/checkpoint/files.c
> @@ -145,6 +145,19 @@ static int scan_fds(struct files_struct *files, int **fdtable)
> return n;
> }
>
> +#ifdef CONFIG_SECURITY
> +int checkpoint_file_security(struct ckpt_ctx *ctx, struct file *file)
> +{
> + return security_checkpoint_obj(ctx, file->f_security,
> + CKPT_SECURITY_FILE);
> +}
> +#else
> +int checkpoint_file_security(struct ckpt_ctx *ctx, struct file *file)
> +{
> + return -EOPNOTSUPP;
> +}
> +#endif
> +
> int checkpoint_file_common(struct ckpt_ctx *ctx, struct file *file,
> struct ckpt_hdr_file *h)
> {
> @@ -159,6 +172,16 @@ int checkpoint_file_common(struct ckpt_ctx *ctx, struct file *file,
> if (h->f_credref < 0)
> return h->f_credref;
>
> + h->f_secref = checkpoint_file_security(ctx, file);
> + if (h->f_secref == -EOPNOTSUPP)
> + h->f_secref = -1;
#define CKPT_SECURITY_NONE -1 ??
> + else if (h->f_secref < 0)
> + return h->f_secref;
[...]
> +
> +static int do_checkpoint_security(struct ckpt_ctx *ctx,
> + const struct ckpt_stored_lsm *l)
> +{
> + int ret, len;
> + struct ckpt_hdr_lsm *h;
> +
> + len = strlen(l->string);
> + if (len > PAGE_SIZE - sizeof(*h))
> + return -E2BIG;
A call to ckpt_write_err() here may be useful.
> + h = ckpt_hdr_get_type(ctx, sizeof(*h)+len+1, CKPT_HDR_SEC);
> + if (!h)
> + return -ENOMEM;
> + h->len = len;
> + h->sectype = l->sectype;
> + strncpy(h->string, l->string, len);
> + h->string[len] = '\0';
> + ret = ckpt_write_obj(ctx, &h->h);
> + ckpt_hdr_put(ctx, h);
> + return ret;
> +}
> +
> +static int checkpoint_security(struct ckpt_ctx *ctx, void *ptr)
> +{
> + return do_checkpoint_security(ctx, (struct ckpt_stored_lsm *) ptr);
> +}
> +
> +static struct ckpt_stored_lsm *do_restore_security(struct ckpt_ctx *ctx)
> +{
> + struct ckpt_hdr_lsm *h;
> + struct ckpt_stored_lsm *l;
> +
> + h = ckpt_read_buf_type(ctx, PAGE_SIZE, CKPT_HDR_SEC);
> + if (IS_ERR(h))
> + return (struct ckpt_stored_lsm *)h;
> + if (h->len > h->h.len - sizeof(struct ckpt_hdr) ||
> + h->len > PAGE_SIZE) {
The second test (for h->len > PAGE_SIZE) is unnecessary - already
done in ckpt_read_buf_type()
> + ckpt_hdr_put(ctx, h);
> + return ERR_PTR(-EINVAL);
> + }
> + l = kzalloc(sizeof(*l), GFP_KERNEL);
> + if (!l) {
> + ckpt_hdr_put(ctx, h);
> + return ERR_PTR(-ENOMEM);
Nit: this part is common... set ret = -ENOMEM and use goto ?
[...]
> @@ -608,14 +613,21 @@ static int restore_task_objs(struct ckpt_ctx *ctx)
> * referenced. See comment in checkpoint_task_objs.
> */
> ret = restore_task_creds(ctx);
> - if (!ret)
> - ret = restore_task_ns(ctx);
> - if (ret < 0)
> + if (ret) {
Nit: (ret < 0) helps (me) read it.
> + ckpt_debug("restore_task_creds returned %d", ret);
> + return ret;
> + }
> + ret = restore_task_ns(ctx);
> + if (ret) {
> + ckpt_debug("restore_task_ns returned %d", ret);
> return ret;
> + }
>
> h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_OBJS);
> - if (IS_ERR(h))
> + if (IS_ERR(h)) {
> + ckpt_debug("Error fetching task obj");
> return PTR_ERR(h);
> + }
>
> ret = restore_obj_file_table(ctx, h->files_objref);
> ckpt_debug("file_table: ret %d (%p)\n", ret, current->files);
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index b12c8bd..bba2b45 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -457,6 +457,31 @@ static int restore_read_header(struct ckpt_ctx *ctx)
> if (ret < 0)
> goto out;
>
> + ret = _ckpt_read_buffer(ctx, ctx->lsm_name, SECURITY_NAME_MAX + 1);
> + if (ret < 0)
> + goto out;
> + if (ctx->uflags & RESTART_KEEP_LSM) {
> + char *cur = security_get_lsm_name();
> + if (strncmp(cur, ctx->lsm_name, SECURITY_NAME_MAX + 1) != 0) {
> + pr_warning("c/r: checkpointed LSM %s, current is %s.\n",
> + ctx->lsm_name, cur);
> + ret = -EINVAL;
EPERM ?
[...]
> @@ -277,6 +283,16 @@ struct ckpt_hdr_groupinfo {
> __u32 groups[0];
> } __attribute__((aligned(8)));
>
> +struct ckpt_hdr_lsm {
> + struct ckpt_hdr h;
> + __u8 sectype;
> + __u16 len;
Alignment ...
> + /*
> + * This is followed by a string of size len+1,
> + * null-terminated
> + */
> + __u8 string[0];
> +} __attribute__((aligned(8)));
> /*
> * todo - keyrings and LSM
> * These may be better done with userspace help though
> @@ -392,6 +408,8 @@ struct ckpt_hdr_file {
> __s32 f_credref;
> __u64 f_pos;
> __u64 f_version;
> + __s32 f_secref;
> + __u32 f_padding;
No need for padding here.
> } __attribute__((aligned(8)));
>
> struct ckpt_hdr_file_generic {
> @@ -634,6 +652,8 @@ struct ckpt_hdr_ipc_perms {
> __u32 mode;
> __u32 _padding;
> __u64 seq;
> + __s32 sec_ref;
> + __u32 padding;
Ditto.
[...]
> @@ -224,6 +234,16 @@ static struct msg_msg *restore_msg_contents_one(struct ckpt_ctx *ctx, int *clen)
> msg->next = NULL;
> pseg = &msg->next;
>
> + if ((ctx->uflags & RESTART_KEEP_LSM) && (h->sec_ref != -1)) {
> + struct ckpt_stored_lsm *l = ckpt_obj_fetch(ctx, h->sec_ref,
> + CKPT_OBJ_SEC);
> + if (IS_ERR(l))
> + return (struct msg_msg *)l;
That's a leak. Need to 'goto out' instead.
> + ret = security_msg_msg_restore(msg, l->string);
> + if (ret)
> + return ERR_PTR(ret);
Here too.
[...]
> @@ -806,6 +815,15 @@ static struct cred *do_restore_cred(struct ckpt_ctx *ctx)
> ret = cred_setfsgid(cred, h->fsgid, &oldgid);
> if (oldgid != h->fsgid && ret < 0)
> goto err_putcred;
> + if ((ctx->uflags & RESTART_KEEP_LSM) && (h->sec_ref != -1)) {
> + struct ckpt_stored_lsm *l = ckpt_obj_fetch(ctx, h->sec_ref,
> + CKPT_OBJ_SEC);
> + if (IS_ERR(l))
> + return (struct cred *)l;
And here too ?
> +/**
> + * security_checkpoint_obj - if first checkpoint of this void* security,
> + * then 1. ask the LSM for a string representing the context, 2. checkpoint
> + * that string
> + * @ctx: the checkpoint context
> + * @security: the void* security being checkpointed
> + * @sectype: indicates the type of object, because LSMs can (and do) store
> + * different types of data for different types of objects.
> + *
> + * Returns the objref of the checkpointed ckpt_stored_lsm representing the
> + * context, or -error on error.
> + *
> + * This is only used at checkpoint of course.
> + */
> +int security_checkpoint_obj(struct ckpt_ctx *ctx, void *security,
> + unsigned sectype)
> +{
> + int strref;
> + struct ckpt_stored_lsm *l;
> + char *str;
> + int len;
> +
> + /*
> + * to simplify the LSM code, short-cut a null security
> + * here.
> + */
> + if (!security)
> + return -EOPNOTSUPP;
> +
> + switch (sectype) {
> + case CKPT_SECURITY_MSG_MSG:
> + str = security_msg_msg_checkpoint(security);
> + break;
> + case CKPT_SECURITY_IPC:
> + str = security_ipc_checkpoint(security);
> + break;
> + case CKPT_SECURITY_FILE:
> + str = security_file_checkpoint(security);
> + break;
> + case CKPT_SECURITY_CRED:
> + str = security_cred_checkpoint(security);
> + break;
> + default:
> + str = ERR_PTR(-EINVAL);
> + break;
> + }
> + /* str will be alloc'ed for us by the LSM. We will free it when
> + * we clear out our hashtable */
> + if (IS_ERR(str))
> + return PTR_ERR(str);
> +
> + len = strlen(str);
> + l = kzalloc(sizeof(*l) + len + 1, GFP_KERNEL);
Probably a dumb question: why do you allocate with the size of
the string when it merely points to the string ?
> + if (!l) {
> + kfree(str);
> + return -ENOMEM;
> + }
> + kref_init(&l->kref);
> + l->sectype = sectype;
> + l->string = str;
> +
> + strref = checkpoint_obj(ctx, l, CKPT_OBJ_SEC);
> + /* do we need to free l if strref was err? */
> + return strref;
> +}
> +
> +#endif
Oren.
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
More information about the Devel
mailing list