Re: [PATCH 2/4] cr: add generic LSM c/r support (v5)

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Serge E. Hallyn wrote:
> 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.
> """
> 
> This patch adds generic support for c/r of LSM credentials.  Support
> for Smack and SELinux (and TOMOYO if appropriate) will be added later.
> Capabilities is already supported through generic creds code.
> 
> This patch supports ipc_perm, msg_msg, cred (task) and file ->security
> fields.  Inodes, superblocks, netif, and xfrm currently are restored
> not through sys_restart() but through container creation, and so the
> security fields should be done then as well.  Network should be added
> when network c/r is added.
> 
> Briefly, all security fields must be exported by the LSM as a simple
> null-terminated string.  They are checkpointed through the
> security_checkpoint_obj() helper, because we must pass it an extra
> sectype field.  Splitting SECURITY_OBJ_SEC into one type per object
> type would not work because, in Smack, one void* security is used for
> all object types.  But we must pass the sectype field because in
> SELinux a different type of structure is stashed in each object type.
> 
> The RESTART_KEEP_LSM flag indicates that the LSM should
> attempt to reuse checkpointed security labels.  It is always
> invalid when the LSM at restart differs from that at checkpoint.
> It is currently only usable for capabilities.
> 
> (For capabilities, restart without RESTART_KEEP_LSM is technically
> not implemented.  There actually might be a use case for that,
> but the safety of it is dubious so for now we always re-create
> checkpointed capability sets whether RESTART_KEEP_LSM is
> specified or not)

[...]

Can you please pull this part, which adds a global "container" section
to the checkpoint image, into a separate patch ?

(It will be helpful for folding the patches into ckpt-v19 eventually).

And IIRC, this needs to be accompanied by a user-cr patch, right ?

> diff --git a/Documentation/checkpoint/readme.txt b/Documentation/checkpoint/readme.txt
> index 571c469..9c6422c 100644
> --- a/Documentation/checkpoint/readme.txt
> +++ b/Documentation/checkpoint/readme.txt
> @@ -161,9 +161,10 @@ in-userspace conversion tools.
>  
>  The general format of the checkpoint image is as follows:
>  1. Image header
> -2. Task hierarchy
> -3. Tasks' state
> -4. Image trailer
> +2. Container configuration
> +3. Task hierarchy
> +4. Tasks' state
> +5. Image trailer
>  
>  The image always begins with a general header that holds a magic
>  number, an architecture identifier (little endian format), a format
> @@ -172,6 +173,11 @@ version number (@rev), followed by information about the kernel
>  checkpoint and the flags given to sys_checkpoint(). This header is
>  followed by an arch-specific header.
>  
> +The container configuration section contains details about the
> +security (LSM) configuration.  Network configuration and
> +container-wide mounts may also go here, so that the userspace
> +restart coordinator can re-create a suitable environment.
> +
>  The task hierarchy comes next so that userspace tools can read it
>  early (even from a stream) and re-create the restarting tasks. This is
>  basically an array of all checkpointed tasks, and their relationships
> @@ -333,6 +339,31 @@ So that's why we don't want CAP_SYS_ADMIN required up-front. That way
>  we will be forced to more carefully review each of those features.
>  However, this can be controlled with a sysctl-variable.

[...]

> +/*
> + * We restore a security label only if
> + * 1. sys_restart specified RESTART_KEEP_LSM
> + * 2. the checkpointed security context was not SECURITY_CTX_NONE
> + */
> +static inline int ckpt_restore_lsm_label(struct ckpt_ctx *ctx, int sec_ref)

Maybe add 'may' somewhere inside tol make it more descriptive ?
(e.g. ckpt_may_restore_lsm_label)

> +{
> +	if ((ctx->uflags & RESTART_KEEP_LSM) && (sec_ref != SECURITY_CTX_NONE))
> +		return 1;
> +	return 0;
> +}
> +

[...]

>  
> +/* stored on hashtable */
> +struct ckpt_stored_lsm {
> +	struct kref kref;
> +	int sectype;
> +	char *string;
> +};

Nit: maybe ckpt_lsm_str(ing) ?

[...]

> +#ifdef CONFIG_CHECKPOINT
> +
> +/**
> + * 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 secref, len, new, ret, fulllen;
> +	char *str;
> +	struct ckpt_hdr_lsm *h;
> +
> +	if (!security)
> +		return -EOPNOTSUPP;
> +
> +	secref = ckpt_obj_lookup_add(ctx, security, CKPT_OBJ_CHECKPOINT_SEC,
> +					&new);
> +	if (!new)
> +		return secref;
> +
> +	ret = ckpt_write_objref(ctx, CKPT_OBJ_SEC, secref);
> +	if (ret < 0)
> +		return ret;
> +
> +	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;
> +	}

Let me suggest a different scheme (also last night's IRC); I think it's
less hackish and uses better the existing {checkpoint,restore}_obj().

* Define one obj type CKPT_OBJ_SEC_{IPC, MSG_MSG, FILE, CRED}, with
matching c/r functions security_{c,r}_{ipc,msg_msg,file,cred}_obj()

* Define one obj type for the string representation CKPT_OBJ_SEC_STR
with matchin c/r functions security_{c,r}_string_obj()

* The helper will now:

	security_checkpoint_obj()
	{
		switch (type) {
		case CKPT_OBJ_SEC_IPC:
			ret = checkpoint_obj(ctx, sec, CKPT_OBJ_SEC_IPC);
			break;
		case CKPT_OBJ_SEC_CRED:
			ret = checkpoint_obj(ctx, sec, CKPT_OBJ_SEC_CRED);
		...
	}

	security_checkpoint_ipc_obj()
	{
		...
		ckpt_lsm_str = str_from_sec_ipc();   /* like you do now */
		objref = checkpoint_obj(ctx, ckpt_lsm_str, CKPT_OBJ_SEC_STR);
		...
		h->objref = objref;
		ckpt_write_obj();
	}

Perhaps a variation on this where the string is checkpoint_obj()'ed
first would also work.

I haven't looked at all the details, but I hope something along these
lines would help untangle the current mess.

Oren.