linux-kernel - Re: [PATCH 3/3] selinux: Use an other error code for an input validation failure in sidtab

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:   Tue, 16 May 2017 14:41:05 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:     Casey Schaufler <casey@...aufler-ca.com>,
        Eric Paris <eparis@...isplace.org>,
        James Morris <james.l.morris@...cle.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        William Roberts <william.c.roberts@...el.com>,
        linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 3/3] selinux: Use an other error code for an input
 validation failure in sidtab_insert()

On Tue, Apr 4, 2017 at 7:16 AM, SF Markus Elfring
<elfring@...rs.sourceforge.net> wrote:
> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Tue, 4 Apr 2017 12:23:41 +0200
>
> The error code "-ENOMEM" was also returned so far when the parameter "s"
> of this function contained a null pointer.
> Now I find that the code "-EINVAL" is more appropriate in this case.
>
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  security/selinux/ss/sidtab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Have you tested this to determine any impact it may have on the
SELinux userspace?  I would agree that EINVAL is probably more
appropriate in this case, but changing this return code has very
little value and may disrupt userspace if it assumes EINVAL means
something else when the policy load fails.  Without a demonstration
that all the code paths have been tested I'm not inclined to merge
this patch.

> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index c5f436b15d19..2eb2a54b88d2 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -36,7 +36,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>         struct sidtab_node *prev, *cur, *newnode;
>
>         if (!s)
> -               return -ENOMEM;
> +               return -EINVAL;
>
>         hvalue = SIDTAB_HASH(sid);
>         prev = NULL;
> --
> 2.12.2

-- 
paul moore
www.paul-moore.com