| Message ID | 20200911164009.21926-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] selinux: Add helper functions to get and set checkreqprot | expand |
On Fri, Sep 11, 2020 at 12:40 PM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > > checkreqprot data member in selinux_state struct is accessed directly by > SELinux functions to get and set. This could cause unexpected read or > write access to this data member due to compiler optimizations and/or > compiler's reordering of access to this field. > > Add helper functions to get and set checkreqprot data member in > selinux_state struct. These helper functions use READ_ONCE and > WRITE_ONCE macros to ensure atomic read or write of memory for > this data member. > > Rename enforcing_enabled() to enforcing_get() to be consistent > with the corresponding set function name. I thought Paul said to only use the new names for checkreqprot_*() and not to touch enforcing_*()? I don't really care either way about the names but usually we wouldn't mix renaming of something else with the introduction of these new helpers in a single patch. FWIW, looking at the history, the enforcing functions were originally named is_enforcing() and set_enforcing() in aa8e712cee93d520e96a2ca8 ("selinux: wrap global selinux state") . Then Paul renamed them to enforcing_enabled() and enforcing_set() in e5a5ca96a42ca7eee19cf869 ("selinux: rename the {is,set}_enforcing() functions").
On Mon, Sep 14, 2020 at 9:25 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Fri, Sep 11, 2020 at 12:40 PM Lakshmi Ramasubramanian > <nramas@linux.microsoft.com> wrote: > > > > checkreqprot data member in selinux_state struct is accessed directly by > > SELinux functions to get and set. This could cause unexpected read or > > write access to this data member due to compiler optimizations and/or > > compiler's reordering of access to this field. > > > > Add helper functions to get and set checkreqprot data member in > > selinux_state struct. These helper functions use READ_ONCE and > > WRITE_ONCE macros to ensure atomic read or write of memory for > > this data member. > > > > Rename enforcing_enabled() to enforcing_get() to be consistent > > with the corresponding set function name. > > I thought Paul said to only use the new names for checkreqprot_*() and > not to touch enforcing_*()? I don't really care either way about the > names but usually we wouldn't mix renaming of something else with the > introduction of these new helpers in a single patch. It's generally a good idea when someone has provided feedback on a patch to limit the changes in the next revision to just those changes which have been suggested. It helps to keep the patch focused on the original issue, makes the follow-up reviews easier, and shortens the develop/reverge/merge cycle. It generally isn't too bad of a problem in the SELinux code, but there are other subsystems where several large patch revisions have been wasted because the patch author lost focus and started making additional changes outside the scope of the comments. If you want to submit the additional changes anyway, my recommendation would be to split them out into a second patch in a patch series (make sure the original patch is first!). Of course the best solution is always to ask if you are unsure :) While I don't check my upstream email quite as often as some folks here, I promise to respond to any follow-up questions if no one answers first. ... and please don't let our small nitpicks discourage you from submitting patches, we really do appreciate help and contributions :) > FWIW, looking at the history, the enforcing functions were originally > named is_enforcing() and set_enforcing() in aa8e712cee93d520e96a2ca8 > ("selinux: wrap global selinux state") . Then Paul renamed them to > enforcing_enabled() and enforcing_set() in e5a5ca96a42ca7eee19cf869 > ("selinux: rename the {is,set}_enforcing() functions").
On 9/14/20 8:04 AM, Paul Moore wrote: > On Mon, Sep 14, 2020 at 9:25 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> On Fri, Sep 11, 2020 at 12:40 PM Lakshmi Ramasubramanian >> <nramas@linux.microsoft.com> wrote: >>> >>> checkreqprot data member in selinux_state struct is accessed directly by >>> SELinux functions to get and set. This could cause unexpected read or >>> write access to this data member due to compiler optimizations and/or >>> compiler's reordering of access to this field. >>> >>> Add helper functions to get and set checkreqprot data member in >>> selinux_state struct. These helper functions use READ_ONCE and >>> WRITE_ONCE macros to ensure atomic read or write of memory for >>> this data member. >>> >>> Rename enforcing_enabled() to enforcing_get() to be consistent >>> with the corresponding set function name. >> >> I thought Paul said to only use the new names for checkreqprot_*() and >> not to touch enforcing_*()? I don't really care either way about the >> names but usually we wouldn't mix renaming of something else with the >> introduction of these new helpers in a single patch. > > It's generally a good idea when someone has provided feedback on a > patch to limit the changes in the next revision to just those changes > which have been suggested. It helps to keep the patch focused on the > original issue, makes the follow-up reviews easier, and shortens the > develop/reverge/merge cycle. It generally isn't too bad of a problem > in the SELinux code, but there are other subsystems where several > large patch revisions have been wasted because the patch author lost > focus and started making additional changes outside the scope of the > comments. If you want to submit the additional changes anyway, my > recommendation would be to split them out into a second patch in a > patch series (make sure the original patch is first!). Sorry about that - I will resubmit the patch with the change to rename only checkreqprot helper function. -lakshmi > > Of course the best solution is always to ask if you are unsure :) > While I don't check my upstream email quite as often as some folks > here, I promise to respond to any follow-up questions if no one > answers first. > > ... and please don't let our small nitpicks discourage you from > submitting patches, we really do appreciate help and contributions :) > >> FWIW, looking at the history, the enforcing functions were originally >> named is_enforcing() and set_enforcing() in aa8e712cee93d520e96a2ca8 >> ("selinux: wrap global selinux state") . Then Paul renamed them to >> enforcing_enabled() and enforcing_set() in e5a5ca96a42ca7eee19cf869 >> ("selinux: rename the {is,set}_enforcing() functions"). >
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 3c05827608b6..9d0cd7054b08 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -1020,7 +1020,7 @@ static noinline int avc_denied(struct selinux_state *state, if (flags & AVC_STRICT) return -EACCES; - if (enforcing_enabled(state) && + if (enforcing_get(state) && !(avd->flags & AVD_FLAGS_PERMISSIVE)) return -EACCES; diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 6210e98219a5..2bbfbb722e95 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3718,7 +3718,7 @@ static int selinux_mmap_file(struct file *file, unsigned long reqprot, return rc; } - if (selinux_state.checkreqprot) + if (checkreqprot_get(&selinux_state)) prot = reqprot; return file_map_prot_check(file, prot, @@ -3732,7 +3732,7 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, const struct cred *cred = current_cred(); u32 sid = cred_sid(cred); - if (selinux_state.checkreqprot) + if (checkreqprot_get(&selinux_state)) prot = reqprot; if (default_noexec && @@ -5882,7 +5882,7 @@ static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) sk->sk_protocol, nlh->nlmsg_type, secclass_map[sclass - 1].name, task_pid_nr(current), current->comm); - if (enforcing_enabled(&selinux_state) && + if (enforcing_get(&selinux_state) && !security_get_allow_unknown(&selinux_state)) return rc; rc = 0; @@ -7234,7 +7234,7 @@ static __init int selinux_init(void) memset(&selinux_state, 0, sizeof(selinux_state)); enforcing_set(&selinux_state, selinux_enforcing_boot); - selinux_state.checkreqprot = selinux_checkreqprot_boot; + checkreqprot_set(&selinux_state, selinux_checkreqprot_boot); selinux_avc_init(&selinux_state.avc); mutex_init(&selinux_state.status_lock); mutex_init(&selinux_state.policy_mutex); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 0ce2ef684ed0..845079045e62 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -123,7 +123,7 @@ static inline void selinux_mark_initialized(struct selinux_state *state) } #ifdef CONFIG_SECURITY_SELINUX_DEVELOP -static inline bool enforcing_enabled(struct selinux_state *state) +static inline bool enforcing_get(struct selinux_state *state) { return READ_ONCE(state->enforcing); } @@ -133,7 +133,7 @@ static inline void enforcing_set(struct selinux_state *state, bool value) WRITE_ONCE(state->enforcing, value); } #else -static inline bool enforcing_enabled(struct selinux_state *state) +static inline bool enforcing_get(struct selinux_state *state) { return true; } @@ -143,6 +143,16 @@ static inline void enforcing_set(struct selinux_state *state, bool value) } #endif +static inline bool checkreqprot_get(const struct selinux_state *state) +{ + return READ_ONCE(state->checkreqprot); +} + +static inline void checkreqprot_set(struct selinux_state *state, bool value) +{ + WRITE_ONCE(state->checkreqprot, value); +} + #ifdef CONFIG_SECURITY_SELINUX_DISABLE static inline bool selinux_disabled(struct selinux_state *state) { diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 45e9efa9bf5b..ad1bc4f57313 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -129,7 +129,7 @@ static ssize_t sel_read_enforce(struct file *filp, char __user *buf, ssize_t length; length = scnprintf(tmpbuf, TMPBUFLEN, "%d", - enforcing_enabled(fsi->state)); + enforcing_get(fsi->state)); return simple_read_from_buffer(buf, count, ppos, tmpbuf, length); } @@ -161,7 +161,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, new_value = !!new_value; - old_value = enforcing_enabled(state); + old_value = enforcing_get(state); if (new_value != old_value) { length = avc_has_perm(&selinux_state, current_sid(), SECINITSID_SECURITY, @@ -307,7 +307,7 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, goto out; if (new_value) { - enforcing = enforcing_enabled(fsi->state); + enforcing = enforcing_get(fsi->state); length = selinux_disable(fsi->state); if (length) goto out; @@ -717,7 +717,8 @@ static ssize_t sel_read_checkreqprot(struct file *filp, char __user *buf, char tmpbuf[TMPBUFLEN]; ssize_t length; - length = scnprintf(tmpbuf, TMPBUFLEN, "%u", fsi->state->checkreqprot); + length = scnprintf(tmpbuf, TMPBUFLEN, "%u", + checkreqprot_get(fsi->state)); return simple_read_from_buffer(buf, count, ppos, tmpbuf, length); } @@ -759,7 +760,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, comm, current->pid); } - fsi->state->checkreqprot = new_value ? 1 : 0; + checkreqprot_set(fsi->state, (new_value ? 1 : 0)); length = count; out: kfree(page); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 9704c8a32303..62792e026096 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -737,7 +737,7 @@ static int security_validtrans_handle_fail(struct selinux_state *state, kfree(n); kfree(t); - if (!enforcing_enabled(state)) + if (!enforcing_get(state)) return 0; return -EPERM; } @@ -1657,7 +1657,7 @@ static int compute_sid_handle_invalid_context( kfree(s); kfree(t); kfree(n); - if (!enforcing_enabled(state)) + if (!enforcing_get(state)) return 0; return -EACCES; } @@ -1964,7 +1964,7 @@ static inline int convert_context_handle_invalid_context( char *s; u32 len; - if (enforcing_enabled(state)) + if (enforcing_get(state)) return -EINVAL; if (!context_struct_to_string(policydb, context, &s, &len)) { diff --git a/security/selinux/status.c b/security/selinux/status.c index 4bc8f809934c..88990d381374 100644 --- a/security/selinux/status.c +++ b/security/selinux/status.c @@ -53,7 +53,7 @@ struct page *selinux_kernel_status_page(struct selinux_state *state) status->version = SELINUX_KERNEL_STATUS_VERSION; status->sequence = 0; - status->enforcing = enforcing_enabled(state); + status->enforcing = enforcing_get(state); /* * NOTE: the next policyload event shall set * a positive value on the status->policyload,
checkreqprot data member in selinux_state struct is accessed directly by SELinux functions to get and set. This could cause unexpected read or write access to this data member due to compiler optimizations and/or compiler's reordering of access to this field. Add helper functions to get and set checkreqprot data member in selinux_state struct. These helper functions use READ_ONCE and WRITE_ONCE macros to ensure atomic read or write of memory for this data member. Rename enforcing_enabled() to enforcing_get() to be consistent with the corresponding set function name. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> Suggested-by: Paul Moore <paul@paul-moore.com> --- security/selinux/avc.c | 2 +- security/selinux/hooks.c | 8 ++++---- security/selinux/include/security.h | 14 ++++++++++++-- security/selinux/selinuxfs.c | 11 ++++++----- security/selinux/ss/services.c | 6 +++--- security/selinux/status.c | 2 +- 6 files changed, 27 insertions(+), 16 deletions(-)