Re: [PATCH v2] selinux: remove unused initial SIDs and improve handling

[Stephen Smalley <sds@tycho.nsa.gov>]

On 1/29/20 11:42 AM, Stephen Smalley wrote:
> Remove initial SIDs that have never been used or are no longer
> used by the kernel from its string table, which is also used
> to generate the SECINITSID_* symbols referenced in code.
> Update the code to gracefully handle the fact that these can
> now be NULL. Stop treating it as an error if a policy defines
> additional initial SIDs unknown to the kernel.  Do not
> load unused initial SID contexts into the sidtab.
> Fix the incorrect usage of the name from the ocontext in error
> messages when loading initial SIDs since these are not presently
> written to the kernel policy and are therefore always NULL.
> 
> This is a first step toward enabling future evolution of
> initial SIDs. Further changes are required to both userspace
> and the kernel to fully address
> https://github.com/SELinuxProject/selinux-kernel/issues/12
> but this takes a small step toward that end.
> 
> Fully decoupling the policy and kernel initial SID values will
> require introducing a mapping between them and dyhamically
> mapping them at load time.
> 
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

Any objections, acks/reviews, or other questions/comments on this patch?
The GitHub issue has a more detailed discussion of how we can safely 
reuse and eventually increase the number of initial SIDs in the future.

> ---
> v2 avoids loading all unused initial SID contexts into the sidtab,
> not just ones beyond SECINITSID_NUM.  It also drops the unnecessary
> check for an undefined context because all contexts in the OCON_ISID
> list were already validated at load time via context_read_and_validate().
> 
>   scripts/selinux/genheaders/genheaders.c       | 11 +++-
>   .../selinux/include/initial_sid_to_string.h   | 57 +++++++++----------
>   security/selinux/selinuxfs.c                  |  6 +-
>   security/selinux/ss/policydb.c                | 25 ++++----
>   security/selinux/ss/services.c                | 26 ++++-----
>   5 files changed, 66 insertions(+), 59 deletions(-)
> 
> diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
> index 544ca126a8a8..f355b3e0e968 100644
> --- a/scripts/selinux/genheaders/genheaders.c
> +++ b/scripts/selinux/genheaders/genheaders.c
> @@ -67,8 +67,12 @@ int main(int argc, char *argv[])
>   	}
>   
>   	isids_len = sizeof(initial_sid_to_string) / sizeof (char *);
> -	for (i = 1; i < isids_len; i++)
> -		initial_sid_to_string[i] = stoupperx(initial_sid_to_string[i]);
> +	for (i = 1; i < isids_len; i++) {
> +		const char *s = initial_sid_to_string[i];
> +
> +		if (s)
> +			initial_sid_to_string[i] = stoupperx(s);
> +	}
>   
>   	fprintf(fout, "/* This file is automatically generated.  Do not edit. */\n");
>   	fprintf(fout, "#ifndef _SELINUX_FLASK_H_\n#define _SELINUX_FLASK_H_\n\n");
> @@ -82,7 +86,8 @@ int main(int argc, char *argv[])
>   
>   	for (i = 1; i < isids_len; i++) {
>   		const char *s = initial_sid_to_string[i];
> -		fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i);
> +		if (s)
> +			fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i);
>   	}
>   	fprintf(fout, "\n#define SECINITSID_NUM %d\n", i-1);
>   	fprintf(fout, "\nstatic inline bool security_is_socket_class(u16 kern_tclass)\n");
> diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
> index 4f93f697f71c..5d332aeb8b6c 100644
> --- a/security/selinux/include/initial_sid_to_string.h
> +++ b/security/selinux/include/initial_sid_to_string.h
> @@ -1,34 +1,33 @@
>   /* SPDX-License-Identifier: GPL-2.0 */
> -/* This file is automatically generated.  Do not edit. */
>   static const char *initial_sid_to_string[] =
>   {
> -    "null",
> -    "kernel",
> -    "security",
> -    "unlabeled",
> -    "fs",
> -    "file",
> -    "file_labels",
> -    "init",
> -    "any_socket",
> -    "port",
> -    "netif",
> -    "netmsg",
> -    "node",
> -    "igmp_packet",
> -    "icmp_socket",
> -    "tcp_socket",
> -    "sysctl_modprobe",
> -    "sysctl",
> -    "sysctl_fs",
> -    "sysctl_kernel",
> -    "sysctl_net",
> -    "sysctl_net_unix",
> -    "sysctl_vm",
> -    "sysctl_dev",
> -    "kmod",
> -    "policy",
> -    "scmp_packet",
> -    "devnull",
> +	NULL,
> +	"kernel",
> +	"security",
> +	"unlabeled",
> +	NULL,
> +	"file",
> +	NULL,
> +	NULL,
> +	"any_socket",
> +	"port",
> +	"netif",
> +	"netmsg",
> +	"node",
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	NULL,
> +	"devnull",
>   };
>   
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 79c710911a3c..daddc880ebfc 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1692,7 +1692,11 @@ static int sel_make_initcon_files(struct dentry *dir)
>   	for (i = 1; i <= SECINITSID_NUM; i++) {
>   		struct inode *inode;
>   		struct dentry *dentry;
> -		dentry = d_alloc_name(dir, security_get_initial_sid_context(i));
> +		const char *s = security_get_initial_sid_context(i);
> +
> +		if (!s)
> +			continue;
> +		dentry = d_alloc_name(dir, s);
>   		if (!dentry)
>   			return -ENOMEM;
>   
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 2aa7f2e1a8e7..768a9d4e0b86 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -865,29 +865,28 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
>   
>   	head = p->ocontexts[OCON_ISID];
>   	for (c = head; c; c = c->next) {
> -		rc = -EINVAL;
> -		if (!c->context[0].user) {
> -			pr_err("SELinux:  SID %s was never defined.\n",
> -				c->u.name);
> -			sidtab_destroy(s);
> -			goto out;
> -		}
> -		if (c->sid[0] == SECSID_NULL || c->sid[0] > SECINITSID_NUM) {
> -			pr_err("SELinux:  Initial SID %s out of range.\n",
> -				c->u.name);
> +		u32 sid = c->sid[0];
> +		const char *name = security_get_initial_sid_context(sid);
> +
> +		if (sid == SECSID_NULL) {
> +			pr_err("SELinux:  SID null was assigned a context.\n");
>   			sidtab_destroy(s);
>   			goto out;
>   		}
> +
> +		/* Ignore initial SIDs unused by this kernel. */
> +		if (!name)
> +			continue;
> +
>   		rc = context_add_hash(p, &c->context[0]);
>   		if (rc) {
>   			sidtab_destroy(s);
>   			goto out;
>   		}
> -
> -		rc = sidtab_set_initial(s, c->sid[0], &c->context[0]);
> +		rc = sidtab_set_initial(s, sid, &c->context[0]);
>   		if (rc) {
>   			pr_err("SELinux:  unable to load initial SID %s.\n",
> -				c->u.name);
> +			       name);
>   			sidtab_destroy(s);
>   			goto out;
>   		}
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 216ce602a2b5..bd924a9a6388 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1323,23 +1323,22 @@ static int security_sid_to_context_core(struct selinux_state *state,
>   	if (!selinux_initialized(state)) {
>   		if (sid <= SECINITSID_NUM) {
>   			char *scontextp;
> +			const char *s = initial_sid_to_string[sid];
>   
> -			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
> +			if (!s)
> +				return -EINVAL;
> +			*scontext_len = strlen(s) + 1;
>   			if (!scontext)
> -				goto out;
> -			scontextp = kmemdup(initial_sid_to_string[sid],
> -					    *scontext_len, GFP_ATOMIC);
> -			if (!scontextp) {
> -				rc = -ENOMEM;
> -				goto out;
> -			}
> +				return 0;
> +			scontextp = kmemdup(s, *scontext_len, GFP_ATOMIC);
> +			if (!scontextp)
> +				return -ENOMEM;
>   			*scontext = scontextp;
> -			goto out;
> +			return 0;
>   		}
>   		pr_err("SELinux: %s:  called before initial "
>   		       "load_policy on unknown SID %d\n", __func__, sid);
> -		rc = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}
>   	read_lock(&state->ss->policy_rwlock);
>   	policydb = &state->ss->policydb;
> @@ -1363,7 +1362,6 @@ static int security_sid_to_context_core(struct selinux_state *state,
>   
>   out_unlock:
>   	read_unlock(&state->ss->policy_rwlock);
> -out:
>   	return rc;
>   
>   }
> @@ -1553,7 +1551,9 @@ static int security_context_to_sid_core(struct selinux_state *state,
>   		int i;
>   
>   		for (i = 1; i < SECINITSID_NUM; i++) {
> -			if (!strcmp(initial_sid_to_string[i], scontext2)) {
> +			const char *s = initial_sid_to_string[i];
> +
> +			if (s && !strcmp(s, scontext2)) {
>   				*sid = i;
>   				goto out;
>   			}
>