On Thu, Feb 13, 2020 at 09:13:09AM -0500, Stephen Smalley wrote:
> 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.

I encourage this initiative from a user perspective. Having to be aware of/address legacy when writing policy is a distraction (just like having to be aware of ordering for that matter)
Even removing the requirement to define sidcons for unused sids is a step in a good direction. In documenting my policy I noticed how often my explanation boils down to:  "This is a historical artifact".

> 
> > ---
> > 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;
> >   			}
> > 
> 

-- 
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift