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 <[email protected]>
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;
}
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)
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)
>