Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wed 08-07-15 11:26:07, Steven Rostedt wrote:
> When testing 4.2-rc1 I hit this WARNING:
>
> [ cut here ]
> WARNING: CPU: 1 PID: 1639 at
> /work/autotest/nobackup/linux-test.git/kernel/auditsc.c:1025
> audit_log_exit+0x734/0xb0d()
> Modules linked in:
> CPU: 1 PID: 1639 Comm: fstab-decode Not tainted 4.2.0-rc1-test+ #2
> Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
> f0817eb4 c0cd118d c10224d4 f0817ee4 c0440fbe c1011b30
> 0001 0667 c10224d4 0401 c04ba081 c04ba081 f18e3c00 f1cd5080^M
> f0817ef4 c0440ff7 0009 f0817f84 c04ba081 f0817f60
> Call Trace:
> [] dump_stack+0x41/0x52
> [] warn_slowpath_common+0x9d/0xb4
> [] ? audit_log_exit+0x734/0xb0d
> [] ? audit_log_exit+0x734/0xb0d
> [] warn_slowpath_null+0x22/0x24
> [] audit_log_exit+0x734/0xb0d
> [] __audit_syscall_exit+0x43/0xf6
> [] syscall_trace_leave+0x30/0xe5
> [] syscall_exit_work+0x19/0x1e
> ---[ end trace 156b2a7afa592deb ]---
>
> Debugging it, I found that it was triggered by this commit:
>
> commit 0b08c5e5944 ("audit: Fix check of return value of strnlen_user()")
>
> 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.
You're right. Thanks for catching this. I don't know what I was thinking
when writing that "fix"...
Honza
> len = strnlen_user(p, MAX_ARG_STRLEN) - 1;
>
> Now the warning triggers on a string of size zero ("\0"), which is a
> legitimate entry.
>
> Signed-off-by: Steven Rostedt
> ---
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 09c65640cad6..ee097948b0a8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1021,7 +1021,7 @@ static int audit_log_single_execve_arg(struct
> audit_context *context,
>* for strings that are too long, we should not have created
>* any.
>*/
> - if (unlikely((len == 0) || len > MAX_ARG_STRLEN - 1)) {
> + if (unlikely((len == -1) || len > MAX_ARG_STRLEN - 1)) {
> WARN_ON(1);
> send_sig(SIGKILL, current, 0);
> return -1;
--
Jan Kara
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wed, 8 Jul 2015 12:29:43 -0400 Steven Rostedt wrote: > I tested the following patch. Feel free to author it yourself and just > add my reported/tested-by tags, or give it to me. Either way, I don't > care. I just want it fixed so that it doesn't make my own tests fail. > OK, you posted your version of the patch, and you just had to include the quote from me that happened to have a grammar mistake: As reported by Steven Rostedt: "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" Ug, my "than" stupidity is forever carved in the git stone logs. At least you didn't stick a "(sic)" in there. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wednesday, July 08, 2015 12:29:43 PM Steven Rostedt wrote:
> On Wed, 8 Jul 2015 12:02:49 -0400
>
> Steven Rostedt wrote:
> > Well, any testing will have to wait. The reason I found this is because
> > it caused my own tests to fail for a bug fix I'm testing (unrelated to
> > this) that I'm getting ready to send to you. My box to run this on is
> > back to running those tests, which can take several hours.
>
> Oh well, my tests just stumbled over another unrelated 4.2-rc1 bug (I
> need to dig into this one now :-( ). But that freed up my machine to
> test this.
>
> I tested the following patch. Feel free to author it yourself and just
> add my reported/tested-by tags, or give it to me. Either way, I don't
> care. I just want it fixed so that it doesn't make my own tests fail.
>
> Thanks!
Acked-by: Paul Moore
Sorry to be late in replying, and I see that this is already in Linus' tree so
the ack above it probably a bit pointless, but thanks for reporting this and
providing a fix.
> 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(struct
> audit_context *context, * 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;
> }
--
paul moore
security @ redhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wed, 8 Jul 2015 12:02:49 -0400
Steven Rostedt wrote:
>
> Well, any testing will have to wait. The reason I found this is because
> it caused my own tests to fail for a bug fix I'm testing (unrelated to
> this) that I'm getting ready to send to you. My box to run this on is
> back to running those tests, which can take several hours.
Oh well, my tests just stumbled over another unrelated 4.2-rc1 bug (I
need to dig into this one now :-( ). But that freed up my machine to
test this.
I tested the following patch. Feel free to author it yourself and just
add my reported/tested-by tags, or give it to me. Either way, I don't
care. I just want it fixed so that it doesn't make my own tests fail.
Thanks!
-- Steve
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(struct
audit_context *context,
* 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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wed, 8 Jul 2015 11:59:39 -0400 Steven Rostedt wrote: > I'll test it again, even though I already did (see below). Well, any testing will have to wait. The reason I found this is because it caused my own tests to fail for a bug fix I'm testing (unrelated to this) that I'm getting ready to send to you. My box to run this on is back to running those tests, which can take several hours. When its done, I'll retest the patch to make sure it works. I don't see why it wont. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wed, 8 Jul 2015 08:53:34 -0700
Linus Torvalds wrote:
> Anyway, to make a long rant more on-point, does this alternative
> version work for you?
I'll test it again, even though I already did (see below).
>
> 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.
>
That was what I originally did ;-) But then I figured I would just
revert the patch with a simple:
git show 0b08c5e59441d | patch -p1 -R
and submit that.
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user()
On Wed, Jul 8, 2015 at 8:26 AM, Steven Rostedt 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Revert: audit: Fix check of return value of strnlen_user()
When testing 4.2-rc1 I hit this WARNING:
[ cut here ]
WARNING: CPU: 1 PID: 1639 at
/work/autotest/nobackup/linux-test.git/kernel/auditsc.c:1025
audit_log_exit+0x734/0xb0d()
Modules linked in:
CPU: 1 PID: 1639 Comm: fstab-decode Not tainted 4.2.0-rc1-test+ #2
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
f0817eb4 c0cd118d c10224d4 f0817ee4 c0440fbe c1011b30
0001 0667 c10224d4 0401 c04ba081 c04ba081 f18e3c00 f1cd5080^M
f0817ef4 c0440ff7 0009 f0817f84 c04ba081 f0817f60
Call Trace:
[] dump_stack+0x41/0x52
[] warn_slowpath_common+0x9d/0xb4
[] ? audit_log_exit+0x734/0xb0d
[] ? audit_log_exit+0x734/0xb0d
[] warn_slowpath_null+0x22/0x24
[] audit_log_exit+0x734/0xb0d
[] __audit_syscall_exit+0x43/0xf6
[] syscall_trace_leave+0x30/0xe5
[] syscall_exit_work+0x19/0x1e
---[ end trace 156b2a7afa592deb ]---
Debugging it, I found that it was triggered by this commit:
commit 0b08c5e5944 ("audit: Fix check of return value of strnlen_user()")
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.
len = strnlen_user(p, MAX_ARG_STRLEN) - 1;
Now the warning triggers on a string of size zero ("\0"), which is a
legitimate entry.
Signed-off-by: Steven Rostedt
---
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 09c65640cad6..ee097948b0a8 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1021,7 +1021,7 @@ static int audit_log_single_execve_arg(struct
audit_context *context,
* for strings that are too long, we should not have created
* any.
*/
- if (unlikely((len == 0) || len > MAX_ARG_STRLEN - 1)) {
+ if (unlikely((len == -1) || len > MAX_ARG_STRLEN - 1)) {
WARN_ON(1);
send_sig(SIGKILL, current, 0);
return -1;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
