* [PATCH v2] selinux: Add helper functions to get and set checkreqprot
@ 2020-09-09 22:28 Lakshmi Ramasubramanian
2020-09-10 12:41 ` Stephen Smalley
2020-09-11 14:07 ` Paul Moore
0 siblings, 2 replies; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-09-09 22:28 UTC (permalink / raw)
To: stephen.smalley.work, paul; +Cc: sashal, jmorris, nramas, selinux
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot
2020-09-09 22:28 [PATCH v2] selinux: Add helper functions to get and set checkreqprot Lakshmi Ramasubramanian
@ 2020-09-10 12:41 ` Stephen Smalley
2020-09-11 14:07 ` Paul Moore
1 sibling, 0 replies; 7+ messages in thread
From: Stephen Smalley @ 2020-09-10 12:41 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Paul Moore, Sasha Levin, James Morris, SElinux list
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot
2020-09-09 22:28 [PATCH v2] selinux: Add helper functions to get and set checkreqprot Lakshmi Ramasubramanian
2020-09-10 12:41 ` Stephen Smalley
@ 2020-09-11 14:07 ` Paul Moore
2020-09-11 14:20 ` Lakshmi Ramasubramanian
2020-09-11 14:22 ` Stephen Smalley
1 sibling, 2 replies; 7+ messages in thread
From: Paul Moore @ 2020-09-11 14:07 UTC (permalink / raw)
To: Lakshmi Ramasubramanian; +Cc: Stephen Smalley, sashal, James Morris, selinux
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot
2020-09-11 14:07 ` Paul Moore
@ 2020-09-11 14:20 ` Lakshmi Ramasubramanian
2020-09-11 14:42 ` Paul Moore
2020-09-11 14:22 ` Stephen Smalley
1 sibling, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-09-11 14:20 UTC (permalink / raw)
To: Paul Moore; +Cc: Stephen Smalley, sashal, James Morris, selinux
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot
2020-09-11 14:20 ` Lakshmi Ramasubramanian
@ 2020-09-11 14:42 ` Paul Moore
0 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2020-09-11 14:42 UTC (permalink / raw)
To: Lakshmi Ramasubramanian; +Cc: Stephen Smalley, sashal, James Morris, selinux
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot
2020-09-11 14:07 ` Paul Moore
2020-09-11 14:20 ` Lakshmi Ramasubramanian
@ 2020-09-11 14:22 ` Stephen Smalley
2020-09-11 14:52 ` Paul Moore
1 sibling, 1 reply; 7+ messages in thread
From: Stephen Smalley @ 2020-09-11 14:22 UTC (permalink / raw)
To: Paul Moore
Cc: Lakshmi Ramasubramanian, Sasha Levin, James Morris, SElinux list
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?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot
2020-09-11 14:22 ` Stephen Smalley
@ 2020-09-11 14:52 ` Paul Moore
0 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2020-09-11 14:52 UTC (permalink / raw)
To: Stephen Smalley
Cc: Lakshmi Ramasubramanian, Sasha Levin, James Morris, SElinux list
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-09-11 16:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09 22:28 [PATCH v2] selinux: Add helper functions to get and set checkreqprot Lakshmi Ramasubramanian
2020-09-10 12:41 ` Stephen Smalley
2020-09-11 14:07 ` Paul Moore
2020-09-11 14:20 ` Lakshmi Ramasubramanian
2020-09-11 14:42 ` Paul Moore
2020-09-11 14:22 ` Stephen Smalley
2020-09-11 14:52 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.