[PATCH 1/3] libselinux: refactor wrapper in sestatus.c for safe shared memory access

[PATCH 1/3] libselinux: refactor wrapper in sestatus.c for safe shared memory access

[Christian Göttsche]

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))           \
+
 /*
  * 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;
 }
-- 
2.28.0

[PATCH 2/3] libselinux: safely access shared memory in selinux_status_updated()

[Christian Göttsche]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/sestatus.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 925e6079..62a864b1 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -99,7 +99,9 @@ static inline uint32_t read_sequence(struct selinux_status_t *status)
 int selinux_status_updated(void)
 {
 	uint32_t	curr_seqno;
-	int		result = 0;
+	uint32_t	tmp_seqno;
+	uint32_t	enforcing;
+	uint32_t	policyload;
 
 	if (selinux_status == NULL) {
 		errno = EINVAL;
@@ -125,21 +127,28 @@ int selinux_status_updated(void)
 	if (last_seqno & 0x0001)
 		last_seqno = curr_seqno;
 
-	if (last_seqno != curr_seqno)
-	{
-		if (avc_enforcing != (int) selinux_status->enforcing) {
-			if (avc_process_setenforce(selinux_status->enforcing) < 0)
-				return -1;
-		}
-		if (last_policyload != selinux_status->policyload) {
-			if (avc_process_policyload(selinux_status->policyload) < 0)
-				return -1;
-			last_policyload = selinux_status->policyload;
-		}
-		last_seqno = curr_seqno;
-		result = 1;
+	if (last_seqno == curr_seqno)
+		return 0;
+
+	do {
+		enforcing = selinux_status->enforcing;
+		policyload = selinux_status->policyload;
+		tmp_seqno = curr_seqno;
+		curr_seqno = read_sequence(selinux_status);
+	} while (tmp_seqno != curr_seqno);
+
+	if (avc_enforcing != (int) enforcing) {
+		if (avc_process_setenforce(enforcing) < 0)
+			return -1;
 	}
-	return result;
+	if (last_policyload != policyload) {
+		if (avc_process_policyload(policyload) < 0)
+			return -1;
+		last_policyload = policyload;
+	}
+	last_seqno = curr_seqno;
+
+	return 1;
 }
 
 /*
-- 
2.28.0

[PATCH 3/3] libselinux: initialize last_policyload in selinux_status_open()

[Christian Göttsche]

If not initialized to the current policyload count, an enforcing change
will trigger policyload-callbacks in selinux_status_updated().

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/sestatus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 62a864b1..dcd7f2dc 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -289,6 +289,7 @@ int selinux_status_open(int fallback)
 	}
 	selinux_status_fd = fd;
 	last_seqno = (uint32_t)(-1);
+	sestatus_save_access(policyload, last_policyload);
 
 	/* No need to use avc threads if the kernel status page is available */
 	avc_using_threads = 0;
-- 
2.28.0

Re: [PATCH 1/3] libselinux: refactor wrapper in sestatus.c for safe shared memory access

[Stephen Smalley]

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.

[PATCH v2 0/2] selinux_status_ changes

[Christian Göttsche]

Dropped first patch refactoring the access to the shared structure.

Christian Göttsche (2):
  libselinux: safely access shared memory in selinux_status_updated()
  libselinux: initialize last_policyload in selinux_status_open()

 libselinux/src/sestatus.c | 55 ++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 18 deletions(-)

-- 
2.28.0

[PATCH v2 1/2] libselinux: safely access shared memory in selinux_status_updated()

[Christian Göttsche]

Access the shared nenory safe in regard to consistent view of the SELinux
kernel status page - not in regard to thread-safety.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/sestatus.c | 40 ++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 814e86ee..ca2d3bbf 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -91,7 +91,9 @@ static inline uint32_t read_sequence(struct selinux_status_t *status)
 int selinux_status_updated(void)
 {
 	uint32_t	curr_seqno;
-	int		result = 0;
+	uint32_t	tmp_seqno;
+	uint32_t	enforcing;
+	uint32_t	policyload;
 
 	if (selinux_status == NULL) {
 		errno = EINVAL;
@@ -117,21 +119,29 @@ int selinux_status_updated(void)
 	if (last_seqno & 0x0001)
 		last_seqno = curr_seqno;
 
-	if (last_seqno != curr_seqno)
-	{
-		if (avc_enforcing != (int) selinux_status->enforcing) {
-			if (avc_process_setenforce(selinux_status->enforcing) < 0)
-				return -1;
-		}
-		if (last_policyload != selinux_status->policyload) {
-			if (avc_process_policyload(selinux_status->policyload) < 0)
-				return -1;
-			last_policyload = selinux_status->policyload;
-		}
-		last_seqno = curr_seqno;
-		result = 1;
+	if (last_seqno == curr_seqno)
+		return 0;
+
+	/* sequence must not be changed during references */
+	do {
+		enforcing = selinux_status->enforcing;
+		policyload = selinux_status->policyload;
+		tmp_seqno = curr_seqno;
+		curr_seqno = read_sequence(selinux_status);
+	} while (tmp_seqno != curr_seqno);
+
+	if (avc_enforcing != (int) enforcing) {
+		if (avc_process_setenforce(enforcing) < 0)
+			return -1;
+	}
+	if (last_policyload != policyload) {
+		if (avc_process_policyload(policyload) < 0)
+			return -1;
+		last_policyload = policyload;
 	}
-	return result;
+	last_seqno = curr_seqno;
+
+	return 1;
 }
 
 /*
-- 
2.28.0

[PATCH v2 2/2] libselinux: initialize last_policyload in selinux_status_open()

[Christian Göttsche]

If not initialized to the current policyload count, an enforcing change
will trigger policyload-callbacks in selinux_status_updated().

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/sestatus.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index ca2d3bbf..9ff2785d 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -278,9 +278,10 @@ static int fallback_cb_policyload(int policyload)
  */
 int selinux_status_open(int fallback)
 {
-	int	fd;
-	char	path[PATH_MAX];
-	long	pagesize;
+	int		fd;
+	char		path[PATH_MAX];
+	long		pagesize;
+	uint32_t	seqno;
 
 	if (!selinux_mnt) {
 		errno = ENOENT;
@@ -304,6 +305,14 @@ int selinux_status_open(int fallback)
 	selinux_status_fd = fd;
 	last_seqno = (uint32_t)(-1);
 
+	/* sequence must not be changed during references */
+	do {
+		seqno = read_sequence(selinux_status);
+
+		last_policyload = selinux_status->policyload;
+
+	} while (seqno != read_sequence(selinux_status));
+
 	/* No need to use avc threads if the kernel status page is available */
 	avc_using_threads = 0;
 
-- 
2.28.0

Re: [PATCH v2 1/2] libselinux: safely access shared memory in selinux_status_updated()

[Stephen Smalley]

On Tue, Aug 25, 2020 at 11:32 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Access the shared nenory safe in regard to consistent view of the SELinux

s/nenory/memory/

> kernel status page - not in regard to thread-safety.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

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

Re: [PATCH v2 2/2] libselinux: initialize last_policyload in selinux_status_open()

[Stephen Smalley]

On Tue, Aug 25, 2020 at 11:32 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> If not initialized to the current policyload count, an enforcing change
> will trigger policyload-callbacks in selinux_status_updated().
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

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

Re: [PATCH v2 2/2] libselinux: initialize last_policyload in selinux_status_open()

[Stephen Smalley]

On Wed, Aug 26, 2020 at 8:53 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Aug 25, 2020 at 11:32 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > If not initialized to the current policyload count, an enforcing change
> > will trigger policyload-callbacks in selinux_status_updated().
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Both applied.