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
| ||
|
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/
Powered by blists - more mailing lists