Skip to contentSkip to conversation

arm64: fix strnlen_user when count <= strlen

PATCH In review Kyle McMartin 2 participants 3 messages Jan 16, 2014 – Jan 17, 2014 arm64

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64: fix strnlen_user when count <= strlen

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.

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

arch/arm64/include/asm/uaccess.h +6 -0
@@ -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;
 }
 
#1 lore raw

Hi Kyle,

Quoted 10 lines · 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!

Quoted 5 lines

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

arch/arm64/lib/strnlen_user.S +1 -0
index 7f7b176a5646..73f3335a2a45 100644
@@ -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)
 
#2 lore raw
Quoted 5 lines · Will Deacon

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.

Quoted 3 lines

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

Quoted 16 lines

Will

--->8

arch/arm64/lib/strnlen_user.S +1 -0
index 7f7b176a5646..73f3335a2a45 100644
@@ -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)