| Message ID | 20200909222822.23198-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] selinux: Add helper functions to get and set checkreqprot | expand |
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>
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.
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
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?
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.
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.
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);
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(-)