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

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

[Steven Rostedt]

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
 00000000 00000000 f0817eb4 c0cd118d c10224d4 f0817ee4 c0440fbe c1011b30
 00000001 00000667 c10224d4 00000401 c04ba081 c04ba081 f18e3c00 f1cd5080^M
 00000000 f0817ef4 c0440ff7 00000009 00000000 f0817f84 c04ba081 f0817f60
Call Trace:
 [<c0cd118d>] dump_stack+0x41/0x52
 [<c0440fbe>] warn_slowpath_common+0x9d/0xb4
 [<c04ba081>] ? audit_log_exit+0x734/0xb0d
 [<c04ba081>] ? audit_log_exit+0x734/0xb0d
 [<c0440ff7>] warn_slowpath_null+0x22/0x24
 [<c04ba081>] audit_log_exit+0x734/0xb0d
 [<c04bb7f4>] __audit_syscall_exit+0x43/0xf6
 [<c040ce27>] syscall_trace_leave+0x30/0xe5
 [<c0cdbced>] 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 <rostedt@goodmis.org>
---
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;

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@goodmis.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

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

[Steven Rostedt]

On Wed, 8 Jul 2015 08:53:34 -0700
Linus Torvalds <torvalds@linux-foundation.org> 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

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

[Steven Rostedt]

On Wed, 8 Jul 2015 11:59:39 -0400
Steven Rostedt <rostedt@goodmis.org> 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

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

[Steven Rostedt]

On Wed, 8 Jul 2015 12:02:49 -0400
Steven Rostedt <rostedt@goodmis.org> 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;
 	}

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

[Paul Moore]

On Wednesday, July 08, 2015 12:29:43 PM Steven Rostedt wrote:
> On Wed, 8 Jul 2015 12:02:49 -0400
> 
> Steven Rostedt <rostedt@goodmis.org> 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 <pmoore@redhat.com>

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

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

[Steven Rostedt]

On Wed, 8 Jul 2015 12:29:43 -0400
Steven Rostedt <rostedt@goodmis.org> 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

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

[Jan Kara]

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
>  00000000 00000000 f0817eb4 c0cd118d c10224d4 f0817ee4 c0440fbe c1011b30
>  00000001 00000667 c10224d4 00000401 c04ba081 c04ba081 f18e3c00 f1cd5080^M
>  00000000 f0817ef4 c0440ff7 00000009 00000000 f0817f84 c04ba081 f0817f60
> Call Trace:
>  [<c0cd118d>] dump_stack+0x41/0x52
>  [<c0440fbe>] warn_slowpath_common+0x9d/0xb4
>  [<c04ba081>] ? audit_log_exit+0x734/0xb0d
>  [<c04ba081>] ? audit_log_exit+0x734/0xb0d
>  [<c0440ff7>] warn_slowpath_null+0x22/0x24
>  [<c04ba081>] audit_log_exit+0x734/0xb0d
>  [<c04bb7f4>] __audit_syscall_exit+0x43/0xf6
>  [<c040ce27>] syscall_trace_leave+0x30/0xe5
>  [<c0cdbced>] 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 <rostedt@goodmis.org>
> ---
> 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 <jack@suse.cz>
SUSE Labs, CR