Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()

[Linus Torvalds]

On Wed, Jul 8, 2015 at 8:26 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/