| Message ID | 20200129164256.3190-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] selinux: remove unused initial SIDs and improve handling | expand |
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; > } >
On Thu, Feb 13, 2020 at 9:12 AM Stephen Smalley <sds@tycho.nsa.gov> 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. First let me climb up on my current favorite soapbox ... This is a perfect example of an email where you could have trimmed the bulk of it in your reply to the original patch posting. ;) Yes, I've been somewhat avoiding this patch simply because I'm not yet sure what I think of all this yet, and since it affects the kernel-userspace API it needs some careful thought. In other words, yes, I see this patch and the associated GH issue, but no I don't have any real comments yet. Sorry.
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; > > } > > >
On Wed, Jan 29, 2020 at 5:42 PM Stephen Smalley <sds@tycho.nsa.gov> 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 Nit: s/dyhamically/dynamically/ > mapping them at load time. > > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > 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(-) <snip> > 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; > } Your sentence "Stop treating it as an error if a policy defines additional initial SIDs unknown to the kernel." and the removed check for > SECINITSID_NUM suggest that you intend to not treat this condition as an error, but sidtab_set_initial() called bellow will reject such SID with -ENIVAL. Or am I misreading it and you just wanted to remove the duplicate check? > + > + /* 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; > } <snip>
On 2/13/20 5:34 PM, Paul Moore wrote: > First let me climb up on my current favorite soapbox ... This is a > perfect example of an email where you could have trimmed the bulk of > it in your reply to the original patch posting. ;) Ah, sorry about that. On the positive side, I have definitely increased how often and how much I am trimming. I just don't always remember still. > Yes, I've been somewhat avoiding this patch simply because I'm not yet > sure what I think of all this yet, and since it affects the > kernel-userspace API it needs some careful thought. In other words, > yes, I see this patch and the associated GH issue, but no I don't have > any real comments yet. > > Sorry. FWIW, this patch alone doesn't truly change the userspace API (aside from making it possible to add new initial SIDs in the future without errors, which in fact is just restoring to the behavior we had originally). It just removes things that are never used and lays the groundwork for future evolution.
On 2/14/20 7:46 AM, Ondrej Mosnacek wrote: > On Wed, Jan 29, 2020 at 5:42 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> Fully decoupling the policy and kernel initial SID values will >> require introducing a mapping between them and dyhamically > > Nit: s/dyhamically/dynamically/ Ah, thanks; will fix if I need to re-spin. >> - 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; >> } > > Your sentence "Stop treating it as an error if a policy defines > additional initial SIDs unknown to the kernel." and the removed check > for > SECINITSID_NUM suggest that you intend to not treat this > condition as an error, but sidtab_set_initial() called bellow will > reject such SID with -ENIVAL. Or am I misreading it and you just > wanted to remove the duplicate check? The comment and if statement below will cause it to ignore any initial SIDs unused by the kernel, whether they are ones <= SECINITSID_NUM whose names have been dropped and replaced by NULL or ones > SECINITSID_NUM. security_get_initial_sid_context() returns NULL for anything > SECINITSID_NUM. > >> + >> + /* Ignore initial SIDs unused by this kernel. */ >> + if (!name) >> + continue; >> +
On Fri, Feb 14, 2020 at 2:22 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 2/14/20 7:46 AM, Ondrej Mosnacek wrote: > > On Wed, Jan 29, 2020 at 5:42 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> Fully decoupling the policy and kernel initial SID values will > >> require introducing a mapping between them and dyhamically > > > > Nit: s/dyhamically/dynamically/ > > Ah, thanks; will fix if I need to re-spin. > > >> - 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; > >> } > > > > Your sentence "Stop treating it as an error if a policy defines > > additional initial SIDs unknown to the kernel." and the removed check > > for > SECINITSID_NUM suggest that you intend to not treat this > > condition as an error, but sidtab_set_initial() called bellow will > > reject such SID with -ENIVAL. Or am I misreading it and you just > > wanted to remove the duplicate check? > > The comment and if statement below will cause it to ignore any initial > SIDs unused by the kernel, whether they are ones <= SECINITSID_NUM whose > names have been dropped and replaced by NULL or ones > SECINITSID_NUM. > security_get_initial_sid_context() returns NULL for anything > > SECINITSID_NUM. Ah yes, it hits the "if (!name) continue;" check, of course... Never mind then. > > > > >> + > >> + /* Ignore initial SIDs unused by this kernel. */ > >> + if (!name) > >> + continue; > >> + >
On Fri, Feb 14, 2020 at 8:22 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 2/14/20 7:46 AM, Ondrej Mosnacek wrote: > > On Wed, Jan 29, 2020 at 5:42 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> Fully decoupling the policy and kernel initial SID values will > >> require introducing a mapping between them and dyhamically > > > > Nit: s/dyhamically/dynamically/ > > Ah, thanks; will fix if I need to re-spin. Normally this would fall under the category of something I could fix-up during a merge, but I think there are a few changes, mostly documentation, that we should add to this patch. First off, I know MLS is the policy everyone wants to forget, but it *is* used so let's not cause them any more pain then they are already feeling. That should add a few initial SIDs back into the list, but I think it still frees up plenty. Second, when we load the initial SIDs, in policydb_load_isids(), you show an error if one of these isid's is assigned a context: + if (sid == SECSID_NULL) { + pr_err("SELinux: SID null was assigned a context.\n"); ... I would suggest that we also display the SID number like below so that policy devs have a better idea of which isid is causing the problem: + if (sid == SECSID_NULL) { + pr_err("SELinux: SID null(%u) was assigned a context.\n", sid); Lastly, and most importantly, there is a lot of good discussion, including a "roadmap" in the GH issue, let's try to capture that in this patch description (maybe minus your retirement plans Stephen <g>). I have no idea where GH may be in a few years, but the git log is FOREVER ;)
On Mon, Feb 24, 2020 at 8:27 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 2/22/20 4:33 PM, Paul Moore wrote: > > On Fri, Feb 14, 2020 at 8:22 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> On 2/14/20 7:46 AM, Ondrej Mosnacek wrote: > >>> On Wed, Jan 29, 2020 at 5:42 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >>>> Fully decoupling the policy and kernel initial SID values will > >>>> require introducing a mapping between them and dyhamically > >>> > >>> Nit: s/dyhamically/dynamically/ > >> > >> Ah, thanks; will fix if I need to re-spin. > > > > Normally this would fall under the category of something I could > > fix-up during a merge, but I think there are a few changes, mostly > > documentation, that we should add to this patch. > > > > First off, I know MLS is the policy everyone wants to forget, but it > > *is* used so let's not cause them any more pain then they are already > > feeling. That should add a few initial SIDs back into the list, but I > > think it still frees up plenty. > > Not sure exactly what you mean here. The patch should still remove all > the unused initial SIDs (i.e. all initial SIDs for which there are no > hardcoded references to SECINITSID_name). The MLS discussion (which was > only in the GH issue, not in the patch description) is about which > initial SIDs can we start reusing in the near term without any > compatibility implications. So at most this affects what we say in the > revised patch description when we pull in the GH issue information, not > the patch itself. Yes, I was thinking of isid reuse, but I wasn't very clear. > I'd also question whether any MLS users would ever try to use a new > kernel without also having a correspondingly updated policy; MLS users > seems unlikely to run the latest kernels on existing distros. That's a good point, but I see no reason why we should not preserve that ability. Especially since I don't see us adding a lot of new initial SIDs in the near future. > > Second, when we load the initial SIDs, in policydb_load_isids(), you > > show an error if one of these isid's is assigned a context: > > > > + if (sid == SECSID_NULL) { > > + pr_err("SELinux: SID null was assigned a context.\n"); > > > > ... I would suggest that we also display the SID number like below so > > that policy devs have a better idea of which isid is causing the > > problem: > > > > + if (sid == SECSID_NULL) { > > + pr_err("SELinux: SID null(%u) was assigned a context.\n", sid); > > This isn't an error check for unused initial SIDs; it is retaining the > existing test for the NULL (0) SID, which isn't supposed to ever be > assigned a context, while dropping the restriction on adding initial > SIDs > SECINITSID_NUM. sid can only be 0 here, and the message already > says "null". If you'd prefer it say "SID 0 was assigned a context" I > can do that if/when I can ever post to vger again. Sorry, I misread the patch; it's fine the way it is. I've kept your reply intact (no trimming), with the exception of my own inline comments, since your mail didn't hit the list. Until you find a way to workaround your mailing list problems Stephen I would encourage others to do the same. > > Lastly, and most importantly, there is a lot of good discussion, > > including a "roadmap" in the GH issue, let's try to capture that in > > this patch description (maybe minus your retirement plans Stephen > > <g>). I have no idea where GH may be in a few years, but the git log > > is FOREVER ;)
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; }
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> --- 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(-)