[lxc-devel] [PATCH 1/1] seccomp: fix 32-bit rules

Stéphane Graber stgraber at ubuntu.com
Fri Jun 20 20:33:27 UTC 2014


On Fri, Jun 20, 2014 at 02:58:41PM -0500, Serge Hallyn wrote:
> When calling seccomp_rule_add(), you must pass the native syscall number
> even if the context is a 32-bit context.  So use resolve_name rather
> than resolve_name_arch.
> 
> Enhance the check of /proc/self/status for Seccomp: so that we do not
> enable seccomp policies if seccomp is not built into the kernel.  This
> is needed before we can enable by-default seccomp policies (which we
> want to do next)
> 
> Fix wrong return value check from seccomp_arch_exist, and remove
> needless abstraction in arch handling.
> 
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>

Acked-by: Stéphane Graber <stgraber at ubuntu.com>

> ---
>  src/lxc/seccomp.c | 72 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 40 insertions(+), 32 deletions(-)
> 
> diff --git a/src/lxc/seccomp.c b/src/lxc/seccomp.c
> index 5df37bc..dfdedf2 100644
> --- a/src/lxc/seccomp.c
> +++ b/src/lxc/seccomp.c
> @@ -182,11 +182,11 @@ bool do_resolve_add_rule(uint32_t arch, char *line, scmp_filter_ctx ctx,
>  {
>  	int nr, ret;
>  
> -	if (arch && !seccomp_arch_exist(ctx, arch)) {
> +	if (arch && seccomp_arch_exist(ctx, arch) != 0) {
>  		ERROR("BUG: seccomp: rule and context arch do not match (arch %d)", arch);
>  		return false;
>  	}
> -	nr = seccomp_syscall_resolve_name_arch(arch, line);
> +	nr = seccomp_syscall_resolve_name(line);
>  	if (nr == __NR_SCMP_ERROR) {
>  		WARN("Seccomp: failed to resolve syscall: %s", line);
>  		WARN("This syscall will NOT be blacklisted");
> @@ -226,7 +226,6 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf)
>  	scmp_filter_ctx compat_ctx = NULL;
>  	bool blacklist = false;
>  	uint32_t default_policy_action = -1, default_rule_action = -1, action;
> -	uint32_t arch = SCMP_ARCH_NATIVE;
>  	enum lxc_hostarch_t native_arch = get_hostarch(),
>  			    cur_rule_arch = native_arch;
>  
> @@ -294,7 +293,6 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf)
>  					continue;
>  				}
>  				cur_rule_arch = lxc_seccomp_arch_i386;
> -				arch = SCMP_ARCH_X86;
>  				if (native_arch == lxc_seccomp_arch_amd64) {
>  					if (compat_ctx)
>  						continue;
> @@ -310,7 +308,6 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf)
>  					continue;
>  				}
>  				cur_rule_arch = lxc_seccomp_arch_amd64;
> -				arch = SCMP_ARCH_X86_64;
>  			} else if (strcmp(line, "[all]") == 0 ||
>  					strcmp(line, "[ALL]") == 0) {
>  				cur_rule_arch = lxc_seccomp_arch_all;
> @@ -331,7 +328,6 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf)
>  					continue;
>  				}
>  				cur_rule_arch = lxc_seccomp_arch_arm;
> -				arch = SCMP_ARCH_ARM;
>  			}
>  #endif
>  			else
> @@ -351,16 +347,16 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf)
>  			goto bad_rule;
>  		}
>  
> -		if (cur_rule_arch == lxc_seccomp_arch_all)
> -			arch = SCMP_ARCH_NATIVE;
> -
>  		/*
>  		 * TODO generalize - if !is_compat_only(native_arch, cur_rule_arch)
> +		 *
> +		 * in other words, the rule is 32-bit only, on 64-bit host;  don't run
> +		 * the rule against the native arch.
>  		 */
>  		if (!(cur_rule_arch == lxc_seccomp_arch_i386 &&
>  			native_arch == lxc_seccomp_arch_amd64)) {
>  			INFO("Adding non-compat rule for %s action %d", line, action);
> -			if (!do_resolve_add_rule(arch, line, conf->seccomp_ctx, action))
> +			if (!do_resolve_add_rule(SCMP_ARCH_NATIVE, line, conf->seccomp_ctx, action))
>  				goto bad_rule;
>  		}
>  
> @@ -371,17 +367,19 @@ static int parse_config_v2(FILE *f, char *line, struct lxc_conf *conf)
>  			cur_rule_arch != lxc_seccomp_arch_amd64) {
>  			int nr1, nr2;
>  			INFO("Adding compat rule for %s action %d", line, action);
> -			nr1 = seccomp_syscall_resolve_name_arch(lxc_seccomp_arch_amd64, line);
> -			nr2 = seccomp_syscall_resolve_name_arch(lxc_seccomp_arch_i386, line);
> +			nr1 = seccomp_syscall_resolve_name_arch(SCMP_ARCH_X86, line);
> +			nr2 = seccomp_syscall_resolve_name_arch(SCMP_ARCH_NATIVE, line);
>  			if (nr1 == nr2) {
> -				/* sigh - if the syscall # is the same for 32- and 64-bit, then
> -				 * we get EFAULT if we try to aplly to compat_ctx.  So apply to
> -				 * the noncompat ctx.  We may already have done so, but that's ok
> +				/* If the syscall # is the same for 32- and 64-bit, then we cannot
> +				 * apply it to the compat_ctx.  So apply it to the noncompat ctx.
> +				 * We may already have done so, but that's ok
>  				 */
> -				if (!do_resolve_add_rule(arch, line, conf->seccomp_ctx, action))
> +				INFO("Adding non-compat rule bc nr1 == nr2 (%d, %d)", nr1, nr2);
> +				if (!do_resolve_add_rule(SCMP_ARCH_NATIVE, line, conf->seccomp_ctx, action))
>  					goto bad_rule;
>  				continue;
>  			}
> +			INFO("Really adding compat rule bc nr1 == nr2 (%d, %d)", nr1, nr2);
>  			if (!do_resolve_add_rule(SCMP_ARCH_X86, line,
>  						compat_ctx, action))
>  				goto bad_rule;
> @@ -449,30 +447,44 @@ static int parse_config(FILE *f, struct lxc_conf *conf)
>  	return parse_config_v2(f, line, conf);
>  }
>  
> -static bool seccomp_already_confined(void)
> +/*
> + * use_seccomp: return true if we should try and apply a seccomp policy
> + * if defined for the container.
> + * This will return false if
> + *   1. seccomp is not enabled in the kernel
> + *   2. a seccomp policy is already enabled for this task
> + */
> +static bool use_seccomp(void)
>  {
>  	FILE *f = fopen("/proc/self/status", "r");
>  	char line[1024];
> -	bool bret = false;
> +	bool already_enabled = false;
> +	bool found = false;
>  	int ret, v;
>  
>  	if (!f)
> -		return false;
> +		return true;
>  
>  	while (fgets(line, 1024, f)) {
>  		if (strncmp(line, "Seccomp:", 8) == 0) {
> +			found = true;
>  			ret = sscanf(line+8, "%d", &v);
> -			if (ret != 1)
> -				continue;
> -			if (v != 0)
> -				bret = true;
> -			goto out;
> +			if (ret == 1 && v != 0)
> +				already_enabled = true;
> +			break;
>  		}
>  	}
>  
> -out:
>  	fclose(f);
> -	return bret;
> +	if (!found) {  /* no Seccomp line, no seccomp in kernel */
> +		INFO("Seccomp is not enabled in the kernel");
> +		return false;
> +	}
> +	if (already_enabled) {  /* already seccomp-confined */
> +		INFO("Already seccomp-confined, not loading new policy");
> +		return false;
> +	}
> +	return true;
>  }
>  
>  int lxc_read_seccomp_config(struct lxc_conf *conf)
> @@ -483,10 +495,8 @@ int lxc_read_seccomp_config(struct lxc_conf *conf)
>  	if (!conf->seccomp)
>  		return 0;
>  
> -	if (seccomp_already_confined()) {
> -		INFO("Already confined by seccomp;  not loading a new policy");
> +	if (!use_seccomp())
>  		return 0;
> -	}
>  #if HAVE_SCMP_FILTER_CTX
>  	/* XXX for debug, pass in SCMP_ACT_TRAP */
>  	conf->seccomp_ctx = seccomp_init(SCMP_ACT_KILL);
> @@ -525,10 +535,8 @@ int lxc_seccomp_load(struct lxc_conf *conf)
>  	int ret;
>  	if (!conf->seccomp)
>  		return 0;
> -	if (seccomp_already_confined()) {
> -		INFO("Already confined by seccomp;  not loading a new policy");
> +	if (!use_seccomp())
>  		return 0;
> -	}
>  	ret = seccomp_load(
>  #if HAVE_SCMP_FILTER_CTX
>  			conf->seccomp_ctx
> -- 
> 2.0.0
> 
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel

-- 
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140620/15880ba2/attachment-0001.sig>


More information about the lxc-devel mailing list