Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220217142133.72205-3-cgzones@googlemail.com> <20220308165527.45456-1-cgzones@googlemail.com>
In-Reply-To: <20220308165527.45456-1-cgzones@googlemail.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 4 Apr 2022 16:03:21 -0400
Message-ID: <CAHC9VhSGggUV2po0mj0qqMBBX1n56BzR99khcYfhjv4jZprEiQ@mail.gmail.com>
Subject: Re: [PATCH v2 4/5] selinux: declare data arrays const
To:     =?UTF-8?Q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>
Cc:     selinux@vger.kernel.org,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>,
        Ondrej Mosnacek <omosnace@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jeremy Kerr <jk@codeconstruct.com.au>,
        Richard Haines <richard_c_haines@btinternet.com>,
        Lakshmi Ramasubramanian <nramas@linux.microsoft.com>,
        Austin Kim <austin.kim@lge.com>,
        Yang Li <yang.lee@linux.alibaba.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 8, 2022 at 11:55 AM Christian G=C3=B6ttsche
<cgzones@googlemail.com> 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=C3=B6ttsche <cgzones@googlemail.com>
> ---
> 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/ge=
nheaders/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 =3D strdup(s);
> -       char *p;
> +       static char buffer[256];
> +       unsigned int i;
> +       char *p =3D buffer;
>
> -       if (!s2) {
> -               fprintf(stderr, "%s:  out of memory\n", progname);
> +       for (i =3D 0; i < (sizeof(buffer) - 1) && *s; i++)
> +               *p++ =3D toupper(*s++);
> +
> +       if (*s) {
> +               fprintf(stderr, "%s:  buffer too small\n", progname);
>                 exit(3);
>         }
>
> -       for (p =3D s2; *p; p++)
> -               *p =3D toupper(*p);
> -       return s2;
> +       *p =3D '\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?

--=20
paul-moore.com