From: Stephen Smalley <sds@tycho.nsa.gov>
To: Ondrej Mosnacek <omosnace@redhat.com>,
linux-security-module@vger.kernel.org,
James Morris <jmorris@namei.org>,
"Serge E. Hallyn" <serge@hallyn.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>,
selinux@vger.kernel.org, Paul Moore <paul@paul-moore.com>,
John Johansen <john.johansen@canonical.com>,
Kees Cook <keescook@chromium.org>,
Micah Morton <mortonm@chromium.org>,
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: Re: [PATCH 1/2] selinux: treat atomic flags more carefully
Date: Tue, 7 Jan 2020 09:45:13 -0500 [thread overview]
Message-ID: <3cd598e0-4e47-3839-ab8d-c35ed141f2cf@tycho.nsa.gov> (raw)
In-Reply-To: <20200107133154.588958-2-omosnace@redhat.com>
On 1/7/20 8:31 AM, Ondrej Mosnacek wrote:
> The disabled/enforcing/initialized flags are all accessed concurrently
> by threads so use the appropriate accessors that ensure atomicity and
> document that it is expected.
>
> Use smp_load/acquire...() helpers (with memory barriers) for the
> initialized flag, since it gates access to the rest of the state
> structures.
>
> Note that the disabled flag is currently not used for anything other
> than avoiding double disable, but it will be used for bailing out of
> hooks once security_delete_hooks() is removed.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
This looks good to me as a cleanup regardless of whether the 2nd patch
is merged. Similar treatment is likely needed for state->checkreqprot
(although I'd like to obsolete and get rid of that entirely at some
point), maybe state->policycap[], isec->initialized, and others, but
that can all be done separately.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/hooks.c | 21 ++++++++--------
> security/selinux/include/security.h | 33 +++++++++++++++++++++++--
> security/selinux/ss/services.c | 38 ++++++++++++++---------------
> 3 files changed, 61 insertions(+), 31 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 659c4a81e897..47ad4db925cf 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -272,7 +272,7 @@ static int __inode_security_revalidate(struct inode *inode,
>
> might_sleep_if(may_sleep);
>
> - if (selinux_state.initialized &&
> + if (selinux_initialized(&selinux_state) &&
> isec->initialized != LABEL_INITIALIZED) {
> if (!may_sleep)
> return -ECHILD;
> @@ -659,7 +659,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>
> mutex_lock(&sbsec->lock);
>
> - if (!selinux_state.initialized) {
> + if (!selinux_initialized(&selinux_state)) {
> if (!opts) {
> /* Defer initialization until selinux_complete_init,
> after the initial policy is loaded and the security
> @@ -928,7 +928,7 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
> * if the parent was able to be mounted it clearly had no special lsm
> * mount options. thus we can safely deal with this superblock later
> */
> - if (!selinux_state.initialized)
> + if (!selinux_initialized(&selinux_state))
> return 0;
>
> /*
> @@ -1103,7 +1103,7 @@ static int selinux_sb_show_options(struct seq_file *m, struct super_block *sb)
> if (!(sbsec->flags & SE_SBINITIALIZED))
> return 0;
>
> - if (!selinux_state.initialized)
> + if (!selinux_initialized(&selinux_state))
> return 0;
>
> if (sbsec->flags & FSCONTEXT_MNT) {
> @@ -2920,7 +2920,8 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> isec->initialized = LABEL_INITIALIZED;
> }
>
> - if (!selinux_state.initialized || !(sbsec->flags & SBLABEL_MNT))
> + if (!selinux_initialized(&selinux_state) ||
> + !(sbsec->flags & SBLABEL_MNT))
> return -EOPNOTSUPP;
>
> if (name)
> @@ -3143,7 +3144,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
> return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> }
>
> - if (!selinux_state.initialized)
> + if (!selinux_initialized(&selinux_state))
> return (inode_owner_or_capable(inode) ? 0 : -EPERM);
>
> sbsec = inode->i_sb->s_security;
> @@ -3229,7 +3230,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
> return;
> }
>
> - if (!selinux_state.initialized) {
> + if (!selinux_initialized(&selinux_state)) {
> /* If we haven't even been initialized, then we can't validate
> * against a policy, so leave the label as invalid. It may
> * resolve to a valid label on the next revalidation try if
> @@ -7304,17 +7305,17 @@ static void selinux_nf_ip_exit(void)
> #ifdef CONFIG_SECURITY_SELINUX_DISABLE
> int selinux_disable(struct selinux_state *state)
> {
> - if (state->initialized) {
> + if (selinux_initialized(state)) {
> /* Not permitted after initial policy load. */
> return -EINVAL;
> }
>
> - if (state->disabled) {
> + if (selinux_disabled(state)) {
> /* Only do this once. */
> return -EINVAL;
> }
>
> - state->disabled = 1;
> + selinux_mark_disabled(state);
>
> pr_info("SELinux: Disabled at runtime.\n");
>
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ecdd610e6449..a39f9565d80b 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -117,15 +117,27 @@ void selinux_avc_init(struct selinux_avc **avc);
>
> extern struct selinux_state selinux_state;
>
> +static inline bool selinux_initialized(const struct selinux_state *state)
> +{
> + /* do a synchronized load to avoid race conditions */
> + return smp_load_acquire(&state->initialized);
> +}
> +
> +static inline void selinux_mark_initialized(struct selinux_state *state)
> +{
> + /* do a synchronized write to avoid race conditions */
> + smp_store_release(&state->initialized, true);
> +}
> +
> #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
> static inline bool enforcing_enabled(struct selinux_state *state)
> {
> - return state->enforcing;
> + return READ_ONCE(state->enforcing);
> }
>
> static inline void enforcing_set(struct selinux_state *state, bool value)
> {
> - state->enforcing = value;
> + WRITE_ONCE(state->enforcing, value);
> }
> #else
> static inline bool enforcing_enabled(struct selinux_state *state)
> @@ -138,6 +150,23 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
> }
> #endif
>
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +static inline bool selinux_disabled(struct selinux_state *state)
> +{
> + return READ_ONCE(state->disabled);
> +}
> +
> +static inline void selinux_mark_disabled(struct selinux_state *state)
> +{
> + WRITE_ONCE(state->disabled, true);
> +}
> +#else
> +static inline bool selinux_disabled(struct selinux_state *state)
> +{
> + return false;
> +}
> +#endif
> +
> static inline bool selinux_policycap_netpeer(void)
> {
> struct selinux_state *state = &selinux_state;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 55cf42945cba..0e8b94e8e156 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -767,7 +767,7 @@ static int security_compute_validatetrans(struct selinux_state *state,
> int rc = 0;
>
>
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> return 0;
>
> read_lock(&state->ss->policy_rwlock);
> @@ -868,7 +868,7 @@ int security_bounded_transition(struct selinux_state *state,
> int index;
> int rc;
>
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> return 0;
>
> read_lock(&state->ss->policy_rwlock);
> @@ -1027,7 +1027,7 @@ void security_compute_xperms_decision(struct selinux_state *state,
> memset(xpermd->dontaudit->p, 0, sizeof(xpermd->dontaudit->p));
>
> read_lock(&state->ss->policy_rwlock);
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> goto allow;
>
> policydb = &state->ss->policydb;
> @@ -1112,7 +1112,7 @@ void security_compute_av(struct selinux_state *state,
> read_lock(&state->ss->policy_rwlock);
> avd_init(state, avd);
> xperms->len = 0;
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> goto allow;
>
> policydb = &state->ss->policydb;
> @@ -1166,7 +1166,7 @@ void security_compute_av_user(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
> avd_init(state, avd);
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> goto allow;
>
> policydb = &state->ss->policydb;
> @@ -1286,7 +1286,7 @@ int security_sidtab_hash_stats(struct selinux_state *state, char *page)
> {
> int rc;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> pr_err("SELinux: %s: called before initial load_policy\n",
> __func__);
> return -EINVAL;
> @@ -1320,7 +1320,7 @@ static int security_sid_to_context_core(struct selinux_state *state,
> *scontext = NULL;
> *scontext_len = 0;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> if (sid <= SECINITSID_NUM) {
> char *scontextp;
>
> @@ -1549,7 +1549,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
> if (!scontext2)
> return -ENOMEM;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> int i;
>
> for (i = 1; i < SECINITSID_NUM; i++) {
> @@ -1736,7 +1736,7 @@ static int security_compute_sid(struct selinux_state *state,
> int rc = 0;
> bool sock;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> switch (orig_tclass) {
> case SECCLASS_PROCESS: /* kernel value */
> *out_sid = ssid;
> @@ -2198,7 +2198,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> goto out;
> }
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> rc = policydb_read(policydb, fp);
> if (rc) {
> kfree(newsidtab);
> @@ -2223,7 +2223,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
>
> state->ss->sidtab = newsidtab;
> security_load_policycaps(state);
> - state->initialized = 1;
> + selinux_mark_initialized(state);
> seqno = ++state->ss->latest_granting;
> selinux_complete_init();
> avc_ss_reset(state->avc, seqno);
> @@ -2639,7 +2639,7 @@ int security_get_user_sids(struct selinux_state *state,
> *sids = NULL;
> *nel = 0;
>
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> goto out;
>
> read_lock(&state->ss->policy_rwlock);
> @@ -2875,7 +2875,7 @@ int security_get_bools(struct selinux_state *state,
> struct policydb *policydb;
> int i, rc;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> *len = 0;
> *names = NULL;
> *values = NULL;
> @@ -3050,7 +3050,7 @@ int security_sid_mls_copy(struct selinux_state *state,
> int rc;
>
> rc = 0;
> - if (!state->initialized || !policydb->mls_enabled) {
> + if (!selinux_initialized(state) || !policydb->mls_enabled) {
> *new_sid = sid;
> goto out;
> }
> @@ -3217,7 +3217,7 @@ int security_get_classes(struct selinux_state *state,
> struct policydb *policydb = &state->ss->policydb;
> int rc;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> *nclasses = 0;
> *classes = NULL;
> return 0;
> @@ -3366,7 +3366,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
>
> *rule = NULL;
>
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> return -EOPNOTSUPP;
>
> switch (field) {
> @@ -3665,7 +3665,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
> struct context *ctx;
> struct context ctx_new;
>
> - if (!state->initialized) {
> + if (!selinux_initialized(state)) {
> *sid = SECSID_NULL;
> return 0;
> }
> @@ -3732,7 +3732,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> int rc;
> struct context *ctx;
>
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> return 0;
>
> read_lock(&state->ss->policy_rwlock);
> @@ -3771,7 +3771,7 @@ int security_read_policy(struct selinux_state *state,
> int rc;
> struct policy_file fp;
>
> - if (!state->initialized)
> + if (!selinux_initialized(state))
> return -EINVAL;
>
> *len = security_policydb_len(state);
>
next prev parent reply other threads:[~2020-01-07 14:44 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-07 13:31 [PATCH 0/2] LSM: Drop security_delete_hooks() Ondrej Mosnacek
2020-01-07 13:31 ` [PATCH 1/2] selinux: treat atomic flags more carefully Ondrej Mosnacek
2020-01-07 14:45 ` Stephen Smalley [this message]
2020-01-07 18:09 ` Kees Cook
2020-01-07 19:45 ` James Morris
2020-01-10 20:22 ` Paul Moore
2020-01-10 20:21 ` Paul Moore
2020-01-07 13:31 ` [PATCH 2/2] security,selinux: get rid of security_delete_hooks() Ondrej Mosnacek
2020-01-07 14:47 ` [PATCH 2/2] security, selinux: " Stephen Smalley
2020-01-08 5:31 ` Paul Moore
2020-01-08 8:15 ` Ondrej Mosnacek
2020-01-08 13:45 ` Paul Moore
2020-01-08 14:49 ` Stephen Smalley
2020-01-07 16:46 ` [PATCH 2/2] security,selinux: " Casey Schaufler
2020-01-07 18:10 ` Kees Cook
2020-01-07 19:59 ` James Morris
2020-01-08 8:21 ` Ondrej Mosnacek
2020-01-08 18:47 ` James Morris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3cd598e0-4e47-3839-ab8d-c35ed141f2cf@tycho.nsa.gov \
--to=sds@tycho.nsa.gov \
--cc=casey@schaufler-ca.com \
--cc=jmorris@namei.org \
--cc=john.johansen@canonical.com \
--cc=keescook@chromium.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mortonm@chromium.org \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
--cc=selinux@vger.kernel.org \
--cc=serge@hallyn.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox