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

[Tycho Andersen]

Hi Pavel,

Thanks for the feedback.

On Mon, Apr 27, 2015 at 03:29:47PM +0300, Pavel Emelyanov wrote:
> 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?

Yes, sorry. This is the type of LSM the host system running. You ask
below, but a host can only be running one LSM.

> A zdtm test would be a good example of how this thing works too.

Yes, I've attached one so you can see, and I'll re send it when I get
time to respin the series from our discussion here.

> > 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?

Earlier in the patch it dies if you have an selinux label:

+                       pr_err("I don't know how to set an selinux context without exec.");

although we could/should probably error out here too just to be
defensive.

As far as I can see (mostly just based on the initial mail [1], maybe
this has changed more recently), SELinux contexts can only change on
exec. Since we don't actually exec at the end of a process, I'm not
sure how to restore them (maybe it's not possible without a kernel
patch?).

[1]: https://lwn.net/Articles/28222/

> > 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?

No, you can only have one LSM at a time. (You can have libselinux
installed, of course, but /sys/kernel/security/selinux won't be there
if you have apparmor.)

> > +
> > +	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?

We catch the null (i.e. no security) profiles and set lsm_profile to
NULL in that case, and lsm_set_label restores things only when there
is a value of lsm_profile, so it should be a no-op in this case.

> > +	 * 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.

Ok, or on pstree directly, since it is actually visible thanks to your
comment above? (That makes the most sense to me, since that's where
the pid info lives too, and the profiles are accessed via pid in most
cases.)

Tycho

> >  }
> 
> -- Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lsm-add-a-test-for-apparmor.patch
Type: text/x-diff
Size: 3761 bytes
Desc: not available
URL: <http://lists.openvz.org/pipermail/criu/attachments/20150430/e9ce83f5/attachment.bin>