Quoting Oren Laadan (orenl@xxxxxxxxxxx):
>
>
> 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).
But LSM's are specifically not containerized, so this is a host
property, not a container one.
> > 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 ??
Yeah, that'd be cleaner.
> > + 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.
ok
> > + 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()
Huh. I had no idea :)
> > + 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 ?
ok
> [...]
>
> > @@ -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.
Really? Whenever I review something like that it forces me to
go through and make sure no >0 codes will be returned for failure...
But ok.
> > + 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 ?
Hmm... I think of this not as "you're not allowed" but rather "it doesn't
make sense."
> [...]
>
> > @@ -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.
ok
> > + 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 ?
No just dumb code - I used to strcpy it in but don't anymore.
> > + 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.
Thanks! Will send a new version.
-serge
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
