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@xxxxxxxxxxxxx> > > 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
Attachment:
signature.asc
Description: PGP signature
