linux-kernel - Re: [PATCH] Revert: audit: Fix check of return value of strnlen

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]

Message-ID: <CA+55aFyuQh9jbFaTFaZLBDaapiQUDZFYXe567oVLFk8fnsSzAQ@mail.gmail.com>
Date:	Wed, 8 Jul 2015 08:53:34 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>, Jan Kara <jack@...e.cz>,
	Paul Moore <pmoore@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()

On Wed, Jul 8, 2015 at 8:26 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> Yes, strnlen_user() returns 0 on fault, but if you look at what len is
> set to, than you would notice that on fault len would be -1.

Ugh. I hate that. It looks bad, but it's also pointless.

It turns out that "len" is unsigned (it's a "size_t"), so "len >
MAX_ARG_STRLEN - 1" already takes care of the error case. And people
arguing for clarity are clearly wrong, since comparing an unsigned
value against "-1" is sure as hell not "clarity".

I personally tend to like range comparisons, so "len < 0 || len >
MAX_ARG_STRLEN - 1" is both readable and correct (and trivial for the
compiler to generate the optimal code for).  Sadly I think gcc has
occasionally generated warnings for code like that (warning for the
"len < 0" test when "len" is unsigned). Compilers that warn for the
good kind of safe range tests should be taken out and shot.

But it looks like we've either disabled that warning, or gcc has
learnt its lesson, because at least the version I have on F22 doesn't
warn.

Also, the code should use

        if (WARN_ON_ONCE(..)) {

instead of

        if (unlikely(..)) {
               WARN_ON();

so I just detest that buggy piece of crap for *so* many reasons.

It's also sad that a one-liner commit that claims to "fix" something
was this broken to begin with. Grr. Honza, not good.

Anyway, to make a long rant more on-point, does this alternative
version work for you?

    diff --git a/kernel/auditsc.c b/kernel/auditsc.c
    index 09c65640cad6..e85bdfd15fed 100644
    --- a/kernel/auditsc.c
    +++ b/kernel/auditsc.c
    @@ -1021,8 +1021,7 @@ static int audit_log_single_execve_arg(
             * for strings that are too long, we should not have created
             * any.
             */
    -       if (unlikely((len == 0) || len > MAX_ARG_STRLEN - 1)) {
    -               WARN_ON(1);
    +       if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) {
                    send_sig(SIGKILL, current, 0);
                    return -1;
            }

because that really looks better to me.

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/