| Message ID | 20200824131841.55687-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] libselinux: refactor wrapper in sestatus.c for safe shared memory access | expand |
On Mon, Aug 24, 2020 at 9:19 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > libselinux/src/sestatus.c | 35 +++++++++++------------------------ > 1 file changed, 11 insertions(+), 24 deletions(-) > > diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c > index 814e86ee..925e6079 100644 > --- a/libselinux/src/sestatus.c > +++ b/libselinux/src/sestatus.c > @@ -80,6 +80,14 @@ static inline uint32_t read_sequence(struct selinux_status_t *status) > return seqno; > } > > +/* sequence must not be changed during references */ > +#define sestatus_save_access(name, result) \ > + uint32_t _seqno; \ > + do { \ > + _seqno = read_sequence(selinux_status); \ > + (result) = selinux_status->name; \ > + } while (_seqno != read_sequence(selinux_status)) \ I'm not sure how much we gain from this macro versus losing in readability of the calling code. It should be clear at the call site that we are setting result to the value of selinux_status->name, either by having the macro "return" the value to the caller or passing the address of result. If we are going to use a macro with a local variable declaration, then it needs to be wrapped with do { ... } while (0) to ensure that the variable has its own scope/block. I'm also not clear on the naming - why "save_access" - is that supposed to be "safe_access"? It would be nice if the trailing backslashes were aligned. To be clear, this code is not currently thread-safe; the "safety" has to do with getting a consistent view of the SELinux kernel status page. > @@ -157,13 +164,7 @@ int selinux_status_getenforce(void) > return fallback_enforcing; > } > > - /* sequence must not be changed during references */ > - do { > - seqno = read_sequence(selinux_status); > - > - enforcing = selinux_status->enforcing; > - > - } while (seqno != read_sequence(selinux_status)); > + sestatus_save_access(enforcing, enforcing); Someone reading the above code snippet has no idea that we just set enforcing to selinux_status->enforcing.
diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c index 814e86ee..925e6079 100644 --- a/libselinux/src/sestatus.c +++ b/libselinux/src/sestatus.c @@ -80,6 +80,14 @@ static inline uint32_t read_sequence(struct selinux_status_t *status) return seqno; } +/* sequence must not be changed during references */ +#define sestatus_save_access(name, result) \ + uint32_t _seqno; \ + do { \ + _seqno = read_sequence(selinux_status); \ + (result) = selinux_status->name; \ + } while (_seqno != read_sequence(selinux_status)) \ + /* * selinux_status_updated * @@ -142,7 +150,6 @@ int selinux_status_updated(void) */ int selinux_status_getenforce(void) { - uint32_t seqno; uint32_t enforcing; if (selinux_status == NULL) { @@ -157,13 +164,7 @@ int selinux_status_getenforce(void) return fallback_enforcing; } - /* sequence must not be changed during references */ - do { - seqno = read_sequence(selinux_status); - - enforcing = selinux_status->enforcing; - - } while (seqno != read_sequence(selinux_status)); + sestatus_save_access(enforcing, enforcing); return enforcing ? 1 : 0; } @@ -179,7 +180,6 @@ int selinux_status_getenforce(void) */ int selinux_status_policyload(void) { - uint32_t seqno; uint32_t policyload; if (selinux_status == NULL) { @@ -194,13 +194,7 @@ int selinux_status_policyload(void) return fallback_policyload; } - /* sequence must not be changed during references */ - do { - seqno = read_sequence(selinux_status); - - policyload = selinux_status->policyload; - - } while (seqno != read_sequence(selinux_status)); + sestatus_save_access(policyload, policyload); return policyload; } @@ -214,7 +208,6 @@ int selinux_status_policyload(void) */ int selinux_status_deny_unknown(void) { - uint32_t seqno; uint32_t deny_unknown; if (selinux_status == NULL) { @@ -225,13 +218,7 @@ int selinux_status_deny_unknown(void) if (selinux_status == MAP_FAILED) return security_deny_unknown(); - /* sequence must not be changed during references */ - do { - seqno = read_sequence(selinux_status); - - deny_unknown = selinux_status->deny_unknown; - - } while (seqno != read_sequence(selinux_status)); + sestatus_save_access(deny_unknown, deny_unknown); return deny_unknown ? 1 : 0; }
Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- libselinux/src/sestatus.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-)