[CRIU] [RFC] lsm: add support for c/ring LSM profiles

[Pavel Emelyanov]

On 04/24/2015 10:11 PM, Tycho Andersen wrote:
> Back from the dead of last year, this patch is finally working! 

Wow!

> It is an RFC, since I think there are a few things that are wrong with this patch:
> 
> 1. lsmtype probably shouldn't live in inventory, but I didn't know where else
>    to put it. When we figure out where to put this the lsmtype enum can move,
>    as well as the random extern in lsm.c

Can you describe what this type means so we could pick proper place for it?
A zdtm test would be a good example of how this thing works too.

> 2. lsm_profile maybe should live on pstree_item instead of ->core, but ->core
>    is the only thing passed to sigreturn_restore().

That's because variable named "current" is accessible all over the restorer
code and points to the pstree_item being restored :)

> 3. I don't know how to restore selinux policies since we don't actually exec()

Can you shed more light on this too? I see the lsm_set_lable() performed even
for selinux, so what's the policies are and what the problem with their restore is?

> I tried originally to get this to work using libraries, but I could _not_ get
> things to work correctly (I assume I was doing something wrong with all the
> static linking, you can see my draft attempts here:
> https://github.com/tych0/criu/commits/apparmor-using-libraries ). I can try to
> resurrect this if it makes more sense, to do it that way, though.
> 
> This patch adds support for checkpoint and restore of two linux security
> modules (apparmor and selinux). The actual checkpoint or restore code isn't
> that interesting, other than that we have to do the LSM restore in the restorer
> blob since it may block any number of things that we want to do as part of the
> restore process.

More comments inline.

> +static void get_host_lsm()
> +{
> +	if (access("/sys/kernel/security/apparmor", F_OK) == 0) {
> +		get_label = apparmor_get_label;
> +		lsmtype = LSMTYPE__APPARMOR;
> +		name = "apparmor";
> +		return;
> +	}
> +
> +#ifdef CONFIG_HAS_SELINUX
> +	if (access("/sys/kernel/security/selinux", F_OK) == 0) {
> +		get_label = selinux_get_label;
> +		lsmtype = LSMTYPE__SELINUX;
> +		name = "selinux";
> +		return;
> +	}
> +#endif

Can these two be present at the same time?

> +
> +	get_label = NULL;
> +	lsmtype = LSMTYPE__NO_LSM;
> +	name = "none";
> +}

> +int validate_lsm()
> +{
> +	struct pstree_item *it;
> +
> +	if (name == NULL)
> +		get_host_lsm();
> +
> +	if (image_lsm == LSMTYPE__NO_LSM || image_lsm == lsmtype)
> +		return 0;
> +
> +	/*
> +	 * This is really only a problem if the processes have actually
> +	 * specified an LSM profile. If not, we won't restore anything anyway,

But what does the lsm_set_label do?

> +	 * so it's fine.
> +	 */
> +	pr_warn("lsm types do not match: host %d migratee %d\n", lsmtype, image_lsm);
> +
> +	for_each_pstree_item(it) {
> +		if (it->core[0]->lsm_profile) {
> +			pr_err("mismatched lsm types and lsm profile specified\n");
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}

> @@ -80,4 +80,6 @@ message core_entry {
>  	optional task_core_entry	tc		= 3;
>  	optional task_kobj_ids_entry	ids		= 4;
>  	optional thread_core_entry	thread_core	= 5;
> +
> +	optional string			lsm_profile	= 9;

This is per-task thing, so the task_core_entry seem to be better place.

>  }

-- Pavel