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

[Stephen Smalley <stephen.smalley.work@gmail.com>]

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.