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

[PATCH v2] 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 atomic read or write of memory for
this data member.

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..25a36a3e6bed 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_enabled(&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_enabled(&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;
+	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..cc29177c8858 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -143,6 +143,16 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
 }
 #endif
 
+static inline bool checkreqprot_enabled(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..540bfa078877 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",
+			   checkreqprot_enabled(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);
-- 
2.28.0

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

[Stephen Smalley]

On Wed, Sep 9, 2020 at 6:28 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.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

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

[Paul Moore]

On Wed, Sep 9, 2020 at 6:28 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.
>
> 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/include/security.h b/security/selinux/include/security.h
> index cbdd3c7aff8b..cc29177c8858 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -143,6 +143,16 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
>  }
>  #endif
>
> +static inline bool checkreqprot_enabled(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);
> +}

This is a nitpick, and I recognize that Stephen already suggested the
use of "*_set()" and "*_enabled()" for names, but if we are going to
name the setter "*_set()" let's also name the getter "*_get()".

Other than that, it looks fine to me.

-- 
paul moore
www.paul-moore.com

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

[Lakshmi Ramasubramanian]

On 9/11/20 7:07 AM, Paul Moore wrote:

>>
>> +static inline bool checkreqprot_enabled(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);
>> +}
> 
> This is a nitpick, and I recognize that Stephen already suggested the
> use of "*_set()" and "*_enabled()" for names, but if we are going to
> name the setter "*_set()" let's also name the getter "*_get()".
> 
> Other than that, it looks fine to me.
> 

Sure - I can do that.

Are you expecting something like below (for checkreqprot and enforcing)?

s/checkreqprot_enabled/checkreqprot_get/

s/enforcing_enabled/enforcing_get/

  -lakshmi

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

[Stephen Smalley]

On Fri, Sep 11, 2020 at 10:07 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Sep 9, 2020 at 6:28 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.
> >
> > 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/include/security.h b/security/selinux/include/security.h
> > index cbdd3c7aff8b..cc29177c8858 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -143,6 +143,16 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
> >  }
> >  #endif
> >
> > +static inline bool checkreqprot_enabled(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);
> > +}
>
> This is a nitpick, and I recognize that Stephen already suggested the
> use of "*_set()" and "*_enabled()" for names, but if we are going to
> name the setter "*_set()" let's also name the getter "*_get()".

I just suggested that we be consistent with the existing naming for
enforcing_*(), which I thought came from you?

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

[Paul Moore]

On Fri, Sep 11, 2020 at 10:20 AM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
> On 9/11/20 7:07 AM, Paul Moore wrote:
> >>
> >> +static inline bool checkreqprot_enabled(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);
> >> +}
> >
> > This is a nitpick, and I recognize that Stephen already suggested the
> > use of "*_set()" and "*_enabled()" for names, but if we are going to
> > name the setter "*_set()" let's also name the getter "*_get()".
> >
> > Other than that, it looks fine to me.
> >
>
> Sure - I can do that.
>
> Are you expecting something like below (for checkreqprot and enforcing)?
>
> s/checkreqprot_enabled/checkreqprot_get/
>
> s/enforcing_enabled/enforcing_get/

Sorry for the confusion, I should have been more clear.  I was
thinking that the names "checkreqprot_set()" and "checkreqprot_get()"
would be nice.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Fri, Sep 11, 2020 at 10:23 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Fri, Sep 11, 2020 at 10:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Sep 9, 2020 at 6:28 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.
> > >
> > > 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/include/security.h b/security/selinux/include/security.h
> > > index cbdd3c7aff8b..cc29177c8858 100644
> > > --- a/security/selinux/include/security.h
> > > +++ b/security/selinux/include/security.h
> > > @@ -143,6 +143,16 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
> > >  }
> > >  #endif
> > >
> > > +static inline bool checkreqprot_enabled(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);
> > > +}
> >
> > This is a nitpick, and I recognize that Stephen already suggested the
> > use of "*_set()" and "*_enabled()" for names, but if we are going to
> > name the setter "*_set()" let's also name the getter "*_get()".
>
> I just suggested that we be consistent with the existing naming for
> enforcing_*(), which I thought came from you?

I vaguely remember having a discussion around the naming back then,
but I can't recall who preferred what and when.  I'm sure it's all in
the archives for anyone who cares to look.

Regardless, I think _set() and _get() make the most sense here.  I can
also see an argument being made for "_enabled()" on some bigger flags.
My personal opinion is that the SELinux kernel code, and kernel code
in general, is a bit of a mess when it comes to naming consistency.
Considering the age, size, and number of contributors the current
state really shouldn't be too surprising (and honestly it isn't that
bad considering everything).  Perhaps someday we can look at that, but
that is so far down the priority list it isn't worth discussing; I'd
tackle the coding style inconsistencies before this.

-- 
paul moore
www.paul-moore.com