[PATCH] selinux: Add helper functions to get and set checkreqprot

[PATCH] selinux: Add helper functions to get and set checkreqprot

[Lakshmi Ramasubramanian]

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(-)

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);
-- 
2.28.0

Re: [PATCH] selinux: Add helper functions to get and set checkreqprot

[Stephen Smalley]

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().