lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 4 Apr 2022 16:03:21 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Christian Göttsche <cgzones@...glemail.com>
Cc:     selinux@...r.kernel.org,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>,
        Ondrej Mosnacek <omosnace@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jeremy Kerr <jk@...econstruct.com.au>,
        Richard Haines <richard_c_haines@...nternet.com>,
        Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        Austin Kim <austin.kim@....com>,
        Yang Li <yang.lee@...ux.alibaba.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] selinux: declare data arrays const

On Tue, Mar 8, 2022 at 11:55 AM Christian Göttsche
<cgzones@...glemail.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öttsche <cgzones@...glemail.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/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

Powered by blists - more mailing lists