[PATCH] arm64: fix strnlen_user when count <= strlen

[PATCH] arm64: fix strnlen_user when count <= strlen

[Kyle McMartin]

I received a bug report about the ruby test-suite failing on AArch64 when
attempting to pass MAX_ARG_STRLEN sized args to execv[1]. It was
expecting an E2BIG returned, but instead was receiving ENOMEM, and
concatenating the argument strings in funky ways.

The problem appeared to be in __strnlen_user on arm64, as when
instrumenting fs/exec.c to compare the results of the asm-generic
strnlen_user, I noticed an off-by-one on the result:
long-param-test: optimized strnlen_user (131072) and naive (131073) disagree!

As a result, fix strnlen_user to match expected behaviour as documented
in lib/strnlen_user.c, and return count+1 when count would be exceeded.

I didn't feel comfortable prodding the assembler, so I just worked
around it in the wrapper.

Signed-off-by: Kyle McMartin <kyle@redhat.com>

1. https://bugzilla.redhat.com/show_bug.cgi?id=1038676

---

I tested that this behaves the same as the lib/strnlen_user.c version
with some hacked together code here:
http://kyle.fedorapeople.org/strnlen_user-test.tar.xz

{master}kmcmarti ~/strnlen_user-test $ ./test
=== count = 0 ===
strnlen_user = 0
__strnlen_user = 0
fixed__strnlen_user = 0
=== count < strlen ===
strnlen_user = 7
__strnlen_user = 6
fixed__strnlen_user = 7
=== count = strlen ===
strnlen_user = 13
__strnlen_user = 12
fixed__strnlen_user = 13
=== count > strlen ===
strnlen_user = 13
__strnlen_user = 13
fixed__strnlen_user = 13

--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -290,9 +290,15 @@ static inline long __must_check strnlen_user(const char __user *s, long n)
 {
 	unsigned long res = 0;
 
+	if (unlikely(n <= 0))
+		return 0;
+
 	if (__addr_ok(s))
 		res = __strnlen_user(s, n);
 
+	if (unlikely(res >= n))
+		return n + 1;
+
 	return res;
 }
 

Re: [PATCH] arm64: fix strnlen_user when count <= strlen

[Will Deacon]

Hi Kyle,

On Thu, Jan 16, 2014 at 11:48:17PM +0000, Kyle McMartin wrote:
> I received a bug report about the ruby test-suite failing on AArch64 when
> attempting to pass MAX_ARG_STRLEN sized args to execv[1]. It was
> expecting an E2BIG returned, but instead was receiving ENOMEM, and
> concatenating the argument strings in funky ways.
> 
> The problem appeared to be in __strnlen_user on arm64, as when
> instrumenting fs/exec.c to compare the results of the asm-generic
> strnlen_user, I noticed an off-by-one on the result:
> long-param-test: optimized strnlen_user (131072) and naive (131073) disagree!
> 
> As a result, fix strnlen_user to match expected behaviour as documented
> in lib/strnlen_user.c, and return count+1 when count would be exceeded.
> 
> I didn't feel comfortable prodding the assembler, so I just worked
> around it in the wrapper.

Actually, I have removed strnlen_user for 3.14. Could you try your test case
with our for-next branch please?

  git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core

As for the issue you spotted, we probably need a fix for that to go into
stable kernels. Does the following (smaller patch) work for you?

Will

--->8

diff --git a/arch/arm64/lib/strnlen_user.S b/arch/arm64/lib/strnlen_user.S
index 7f7b176a5646..73f3335a2a45 100644
--- a/arch/arm64/lib/strnlen_user.S
+++ b/arch/arm64/lib/strnlen_user.S
@@ -37,6 +37,7 @@ ENTRY(__strnlen_user)
 USER(9f, ldrb	w3, [x0], #1	)
 	cbnz	w3, 1b
 2:	sub	x0, x0, x2
+	cinc	x0, x0, mi
 	ret
 ENDPROC(__strnlen_user)
 

Re: [PATCH] arm64: fix strnlen_user when count <= strlen

[Kyle McMartin]

On Fri, Jan 17, 2014 at 10:51:07AM +0000, Will Deacon wrote:
> Actually, I have removed strnlen_user for 3.14. Could you try your test case
> with our for-next branch please?
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> 

This will work fine, I believe (I can't easily test on HW since we're
dependent on $vendor drivers that aren't upstream yet) but in my testing
yesterday I parroted the x86 word-at-a-time.h and lib/strnlen_user.c and
that seemed to work.

> As for the issue you spotted, we probably need a fix for that to go into
> stable kernels. Does the following (smaller patch) work for you?
> 

This gets the count == 0 case wrong still, I believe.

=== count = 0 ===
strnlen_user = 0
__strnlen_user = 1

(Where strnlen_user is the lib/strnlen_user.c implementation.)

Testing if (n <= 0) return 0; before __strnlen_user fixes it though.

Given this is called in, I think, only two places in the kernel, maybe
we should just backport the switch to generic for stable? I can't
imagine it conflicting with anything.

regards, Kyle

> Will
> 
> --->8
> 
> diff --git a/arch/arm64/lib/strnlen_user.S b/arch/arm64/lib/strnlen_user.S
> index 7f7b176a5646..73f3335a2a45 100644
> --- a/arch/arm64/lib/strnlen_user.S
> +++ b/arch/arm64/lib/strnlen_user.S
> @@ -37,6 +37,7 @@ ENTRY(__strnlen_user)
>  USER(9f, ldrb	w3, [x0], #1	)
>  	cbnz	w3, 1b
>  2:	sub	x0, x0, x2
> +	cinc	x0, x0, mi
>  	ret
>  ENDPROC(__strnlen_user)
>