[CRIU] [PATCH] lsm: add a --lsm-profile flag

[Tycho Andersen]

Hi Pavel,

On Fri, Dec 04, 2015 at 03:35:22PM +0300, Pavel Emelyanov wrote:
> On 12/03/2015 08:58 PM, Tycho Andersen wrote:
> > In LXD, we use the container name in the LSM profile. If the container name
> > is changed on migrate (on the host side), we want to use a different LSM
> > profile name (a. la. --cgroup-root). This flag adds that support.
> 
> Hi, Tycho.
> 
> Hopefully you don't plan to have it in 1.8 :) The release is in 3 days
> 2 of which are weekends.

Nope :). I just had this patch sitting around, and Cyrill mentioned he
was working in this area and said I should just send it and we can
wait to apply it until after 1.8.

> And some more comments inline.
> 
> > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > ---
> >  cr-restore.c         | 36 +++++++++++++++++++++---------------
> >  crtools.c            |  6 ++++++
> >  include/cr_options.h |  5 +++++
> >  include/lsm.h        |  3 ++-
> >  lsm.c                | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  5 files changed, 81 insertions(+), 18 deletions(-)
> > 
> > diff --git a/cr-restore.c b/cr-restore.c
> > index 1c0c641..3c636b9 100644
> > --- a/cr-restore.c
> > +++ b/cr-restore.c
> > @@ -2827,29 +2827,35 @@ static int sigreturn_restore(pid_t pid, CoreEntry *core)
> >  	if (!creds)
> >  		goto err;
> >  
> > -	if (creds->lsm_profile) {
> > -		char *rendered;
> > +	if (creds->lsm_profile || opts.lsm_supplied) {
> > +		char *rendered, *profile;
> >  		int ret;
> >  
> > -		if (validate_lsm(creds) < 0)
> > +		profile = creds->lsm_profile;
> > +		if (opts.lsm_supplied)
> > +			profile = opts.lsm_profile;
> > +
> > +		if (validate_lsm(profile) < 0)
> >  			return -1;
> >  
> > -		ret = render_lsm_profile(creds->lsm_profile, &rendered);
> > -		if (ret < 0) {
> > -			goto err_nv;
> > -		}
> > +		if (profile) {
> 
> Is this if required? I cannot find the combination of creds->lsm_profile and
> opts.lsm_profile which can lead to NULL here :(

If you pass --lsm-profile none: (i.e. don't set an lsm profile), you
can get NULL here. (Admittedly the syntax is a little ugly, we could
do a special case for --lsm-profile none if you want.)

> > +			ret = render_lsm_profile(profile, &rendered);
> > +			if (ret < 0) {
> > +				goto err_nv;
> > +			}
> > +
> > +			lsm_pos = rst_mem_cpos(RM_PRIVATE);
> > +			lsm_profile_len = strlen(rendered);
> > +			lsm = rst_mem_alloc(lsm_profile_len + 1, RM_PRIVATE);
> > +			if (!lsm) {
> > +				xfree(rendered);
> > +				goto err_nv;
> > +			}
> >  
> > -		lsm_pos = rst_mem_cpos(RM_PRIVATE);
> > -		lsm_profile_len = strlen(rendered);
> > -		lsm = rst_mem_alloc(lsm_profile_len + 1, RM_PRIVATE);
> > -		if (!lsm) {
> > +			strncpy(lsm, rendered, lsm_profile_len);
> >  			xfree(rendered);
> > -			goto err_nv;
> >  		}
> >  
> > -		strncpy(lsm, rendered, lsm_profile_len);
> > -		xfree(rendered);
> > -
> >  	}
> >  
> >  	if (seccomp_filters_get_rst_pos(core, &n_seccomp_filters, &seccomp_filter_pos) < 0)
> > diff --git a/crtools.c b/crtools.c
> > index d3812a1..68756a0 100644
> > --- a/crtools.c
> > +++ b/crtools.c
> > @@ -41,6 +41,7 @@
> >  #include "security.h"
> >  #include "irmap.h"
> >  #include "fault-injection.h"
> > +#include "lsm.h"
> >  
> >  #include "setproctitle.h"
> >  
> > @@ -253,6 +254,7 @@ int main(int argc, char *argv[], char *envp[])
> >  		{ "freeze-cgroup",		required_argument,	0, 1068 },
> >  		{ "ghost-limit",		required_argument,	0, 1069 },
> >  		{ "irmap-scan-path",		required_argument,	0, 1070 },
> > +		{ "lsm-profile",		required_argument,	0, 1071 },
> >  		{ },
> >  	};
> >  
> > @@ -498,6 +500,10 @@ int main(int argc, char *argv[], char *envp[])
> >  			if (irmap_scan_path_add(optarg))
> >  				return -1;
> >  			break;
> > +		case 1071:
> > +			if (parse_lsm_arg(optarg) < 0)
> > +				return -1;
> > +			break;
> >  		case 'M':
> >  			{
> >  				char *aux;
> > diff --git a/include/cr_options.h b/include/cr_options.h
> > index eac7283..f31078a 100644
> > --- a/include/cr_options.h
> > +++ b/include/cr_options.h
> > @@ -5,6 +5,8 @@
> >  
> >  #include "list.h"
> >  
> > +#include "protobuf/inventory.pb-c.h"
> > +
> >  /*
> >   * CPU capability options.
> >   */
> > @@ -95,6 +97,9 @@ struct cr_options {
> >  	bool			overlayfs;
> >  	size_t			ghost_limit;
> >  	struct list_head	irmap_scan_paths;
> > +	bool			lsm_supplied;
> > +	Lsmtype			force_lsm;
> 
> This field is unused in the patch.

Bah, it's from a previous version of this patch. I'll remove and
resend.

> > +	char			*lsm_profile;
> >  };
> >  
> >  extern struct cr_options opts;
> > diff --git a/include/lsm.h b/include/lsm.h
> > index 1409acd..bd13ef7 100644
> > --- a/include/lsm.h
> > +++ b/include/lsm.h
> > @@ -23,7 +23,7 @@ extern int collect_lsm_profile(pid_t, CredsEntry *);
> >   * Validate that the LSM profiles can be correctly applied (must happen after
> >   * pstree is set up).
> >   */
> > -int validate_lsm(CredsEntry *ce);
> > +int validate_lsm(char *profile);
> >  
> >  /*
> >   * Render the profile name in the way that the LSM wants it written to
> > @@ -31,4 +31,5 @@ int validate_lsm(CredsEntry *ce);
> >   */
> >  int render_lsm_profile(char *profile, char **val);
> >  
> > +extern int parse_lsm_arg(char *arg);
> >  #endif /* __CR_LSM_H__ */
> > diff --git a/lsm.c b/lsm.c
> > index c960611..dac1094 100644
> > --- a/lsm.c
> > +++ b/lsm.c
> > @@ -7,6 +7,7 @@
> >  #include "config.h"
> >  #include "pstree.h"
> >  #include "util.h"
> > +#include "cr_options.h"
> >  
> >  #include "protobuf.h"
> >  #include "protobuf/inventory.pb-c.h"
> > @@ -106,6 +107,9 @@ static int selinux_get_label(pid_t pid, char **output)
> >  
> >  void kerndat_lsm(void)
> >  {
> > +	if (name)
> > +		return;
> 
> Please, explain why this if is required.

We can now call this function twice on restore: once if the user
passes --lsm-profile and once in kerndat_lsm_rst(). We don't need to
do this detection twice, though. We could delete this if you want.

Tycho