| Message ID | 20200909182351.10740-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selinux: Add helper functions to get and set checkreqprot | expand |
On Wed, Sep 9, 2020 at 2:23 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 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 explicit read or write of memory for > this data member. s/explicit/atomic/ > This patch is based on commit 66ccd2560aff > ("selinux: simplify away security_policydb_len()") in "next" branch > in https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git Don't include this kind of information in a commit message, if needed it can go after the --- or in brackets in the subject line ala [-next] but it isn't necessary when sending against the next branch because that's the default expectation for submitted patches for selinux. No need to cc lsm list on selinux-only patches. > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index cbdd3c7aff8b..b19d919f01e7 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -209,6 +209,16 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) > return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]; > } > > +static inline bool selinux_checkreqprot(const struct selinux_state *state) > +{ > + return READ_ONCE(state->checkreqprot); > +} > +static inline void selinux_checkreqprot_set(struct selinux_state *state, > + bool value) > +{ > + WRITE_ONCE(state->checkreqprot, value); > +} Move these up with the enforcing accessor functions in this header and use a consistent naming, e.g. checkreqprot_enabled(), checkreqprot_set().
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 6210e98219a5..520c7b78bcd8 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 (selinux_checkreqprot(&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 (selinux_checkreqprot(&selinux_state)) prot = reqprot; if (default_noexec && @@ -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; + selinux_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 cbdd3c7aff8b..b19d919f01e7 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -209,6 +209,16 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]; } +static inline bool selinux_checkreqprot(const struct selinux_state *state) +{ + return READ_ONCE(state->checkreqprot); +} +static inline void selinux_checkreqprot_set(struct selinux_state *state, + bool value) +{ + WRITE_ONCE(state->checkreqprot, value); +} + int security_mls_enabled(struct selinux_state *state); int security_load_policy(struct selinux_state *state, void *data, size_t len, diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 45e9efa9bf5b..ba636d3ea89d 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -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", + selinux_checkreqprot(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; + selinux_checkreqprot_set(fsi->state, (new_value ? 1 : 0)); length = count; out: kfree(page);
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 explicit read or write of memory for this data member. This patch is based on commit 66ccd2560aff ("selinux: simplify away security_policydb_len()") in "next" branch in https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- security/selinux/hooks.c | 6 +++--- security/selinux/include/security.h | 10 ++++++++++ security/selinux/selinuxfs.c | 5 +++-- 3 files changed, 16 insertions(+), 5 deletions(-)