Re: [PATCH v2 4/5] selinux: declare data arrays const

[Paul Moore]

On Tue, Mar 8, 2022 at 11:55 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> The arrays for the policy capability names, the initial sid identifiers
> and the class and permission names are not changed at runtime.  Declare
> them const to avoid accidental modification.
>
> Do not override the classmap and the initial sid list in the build time
> script genheaders, by using a static buffer in the conversion function
> stoupperx().  In cases we need to compare or print more than one
> identifier allocate a temporary copy.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> v2:
>    Drop const exemption for genheaders script by rewriting stoupperx().
> ---
>  scripts/selinux/genheaders/genheaders.c       | 76 ++++++++++---------
>  scripts/selinux/mdp/mdp.c                     |  4 +-
>  security/selinux/avc.c                        |  2 +-
>  security/selinux/include/avc_ss.h             |  2 +-
>  security/selinux/include/classmap.h           |  2 +-
>  .../selinux/include/initial_sid_to_string.h   |  3 +-
>  security/selinux/include/policycap.h          |  2 +-
>  security/selinux/include/policycap_names.h    |  2 +-
>  security/selinux/ss/services.c                |  4 +-
>  9 files changed, 51 insertions(+), 46 deletions(-)
>
> diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
> index f355b3e0e968..a2caff3c997f 100644
> --- a/scripts/selinux/genheaders/genheaders.c
> +++ b/scripts/selinux/genheaders/genheaders.c
> @@ -26,19 +26,23 @@ static void usage(void)
>         exit(1);
>  }
>
> -static char *stoupperx(const char *s)
> +static const char *stoupperx(const char *s)
>  {
> -       char *s2 = strdup(s);
> -       char *p;
> +       static char buffer[256];
> +       unsigned int i;
> +       char *p = buffer;
>
> -       if (!s2) {
> -               fprintf(stderr, "%s:  out of memory\n", progname);
> +       for (i = 0; i < (sizeof(buffer) - 1) && *s; i++)
> +               *p++ = toupper(*s++);
> +
> +       if (*s) {
> +               fprintf(stderr, "%s:  buffer too small\n", progname);
>                 exit(3);
>         }
>
> -       for (p = s2; *p; p++)
> -               *p = toupper(*p);
> -       return s2;
> +       *p = '\0';
> +
> +       return buffer;
>  }

Hmmm.  I recognize this is just build time code so it's not as
critical, but I still don't like the idea of passing back a static
buffer to the caller; it just seems like we are asking for future
trouble.  I'm also curious as to why you made this choice in this
revision when the existing code should have worked (passed a const,
returned a non-const).  I'm sure I'm missing something obvious, but
can you help me understand why this is necessary?

-- 
paul-moore.com