Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: circumvent bug in uClibc-ng syscall() on x86_64 systems #175

Closed
wants to merge 1 commit into from
Closed

RFE: circumvent bug in uClibc-ng syscall() on x86_64 systems #175

wants to merge 1 commit into from

Conversation

casantos
Copy link

On uClibc up to v1.0.31, syscall() for x86_64 is defined in
libc/sysdeps/linux/x86_64/syscall.S as

syscall:
movq %rdi, %rax /* Syscall number -> rax. /
movq %rsi, %rdi /
shift arg1 - arg5. /
movq %rdx, %rsi
movq %rcx, %rdx
movq %r8, %r10
movq %r9, %r8
movq 8(%rsp),%r9 /
arg6 is on the stack. /
syscall /
Do the system call. /
cmpq $-4095, %rax /
Check %rax for error. /
jae __syscall_error /
Branch forward if it failed. /
ret /
Return to caller. */

And __syscall_error is defined in
libc/sysdeps/linux/x86_64/__syscall_error.c as

int __syscall_error(void) attribute_hidden;
int __syscall_error(void)
{
register int err_no asm ("%rcx");
asm ("mov %rax, %rcx\n\t"
"neg %rcx");
__set_errno(err_no);
return -1;
}

Notice that __syscall_error returns -1 as a 32-bit int in %rax, a 64-bit
register i.e. 0x00000000ffffffff (decimal 4294967295). When this value
is compared to -1 in _sys_chk_seccomp_flag_kernel() the result is false,
leading the function to always return 0.

Prevent the error by coercing the return value of syscall() to int in a
temporary variable before comparing it to -1. We could use just an (int)
cast but the variable makes the code more readable and the machine code
generated by the compiler is the same in both cases.

All other syscall() invocations were inspected and they either already
coerce the result to int or do not compare it to -1.

The same problem probably occurs on other 64-bit systems but so far only
x86_64 was tested.

A bug report is being submitted to uClibc.

Signed-off-by: Carlos Santos casantos@redhat.com

On uClibc up to v1.0.31, syscall() for x86_64 is defined in
libc/sysdeps/linux/x86_64/syscall.S as

syscall:
        movq %rdi, %rax         /* Syscall number -> rax.  */
        movq %rsi, %rdi         /* shift arg1 - arg5.  */
        movq %rdx, %rsi
        movq %rcx, %rdx
        movq %r8, %r10
        movq %r9, %r8
        movq 8(%rsp),%r9        /* arg6 is on the stack.  */
        syscall                 /* Do the system call.  */
        cmpq $-4095, %rax       /* Check %rax for error.  */
        jae __syscall_error     /* Branch forward if it failed.  */
        ret                     /* Return to caller.  */

And __syscall_error is defined in
libc/sysdeps/linux/x86_64/__syscall_error.c as

int __syscall_error(void) attribute_hidden;
int __syscall_error(void)
{
	register int err_no __asm__ ("%rcx");
	__asm__ ("mov %rax, %rcx\n\t"
	         "neg %rcx");
	__set_errno(err_no);
	return -1;
}

Notice that __syscall_error returns -1 as a 32-bit int in %rax, a 64-bit
register i.e. 0x00000000ffffffff (decimal 4294967295). When this value
is compared to -1 in _sys_chk_seccomp_flag_kernel() the result is false,
leading the function to always return 0.

Prevent the error by coercing the return value of syscall() to int in a
temporary variable before comparing it to -1. We could use just an (int)
cast but the variable makes the code more readable and the machine code
generated by the compiler is the same in both cases.

All other syscall() invocations were inspected and they either already
coerce the result to int or do not compare it to -1.

The same problem probably occurs on other 64-bit systems but so far only
x86_64 was tested.

A bug report is being submitted to uClibc.

Signed-off-by: Carlos Santos <casantos@redhat.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.794% when pulling 613e601 on casantos:master into c95cdad on seccomp:master.

buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request Oct 19, 2019
On uClibc up to at least v1.0.32, syscall() for x86_64 is defined in
libc/sysdeps/linux/x86_64/syscall.S as

syscall:
        movq %rdi, %rax         /* Syscall number -> rax.  */
        movq %rsi, %rdi         /* shift arg1 - arg5.  */
        movq %rdx, %rsi
        movq %rcx, %rdx
        movq %r8, %r10
        movq %r9, %r8
        movq 8(%rsp),%r9        /* arg6 is on the stack.  */
        syscall                 /* Do the system call.  */
        cmpq $-4095, %rax       /* Check %rax for error.  */
        jae __syscall_error     /* Branch forward if it failed.  */
        ret                     /* Return to caller.  */

And __syscall_error is defined in
libc/sysdeps/linux/x86_64/__syscall_error.c as

int __syscall_error(void) attribute_hidden;
int __syscall_error(void)
{
        register int err_no __asm__ ("%rcx");
        __asm__ ("mov %rax, %rcx\n\t"
                 "neg %rcx");
        __set_errno(err_no);
        return -1;
}

Notice that __syscall_error returns -1 as a 32-bit int in %rax, a 64-bit
register i.e. 0x00000000ffffffff (decimal 4294967295). When this value
is compared to -1 in _sys_chk_seccomp_flag_kernel() the result is false,
leading the function to always return 0.

Prevent the error by coercing the return value of syscall() to int in a
temporary variable before comparing it to -1. We could use just an (int)
cast but the variable makes the code more readable and the machine code
generated by the compiler is the same in both cases.

All other syscall() invocations were inspected and they either already
coerce the result to int or do not compare it to -1.

The same problem probably occurs on other 64-bit systems but so far only
x86_64 was tested.

A bug report is being submitted to uClibc.

Upstream status: seccomp/libseccomp#175

Signed-off-by: Carlos Santos <unixmania@gmail.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
@pcmoore pcmoore changed the title Circumvent bug in uClibc-ng syscall() on x86_64 systems RFE: circumvent bug in uClibc-ng syscall() on x86_64 systems Oct 21, 2019
@pcmoore
Copy link
Member

pcmoore commented Oct 21, 2019

Hi @casantos, thanks for the report and the patch. If I understand you correctly, it sounds like the real issue is with uClibc and this is just a workaround for libseccomp?

@casantos
Copy link
Author

Hi @casantos, thanks for the report and the patch. If I understand you correctly, it sounds like the real issue is with uClibc and this is just a workaround for libseccomp?

Right. The workaround is necessary because uClibc-ng still has the bug and even after it gets fixed the user may not be able to update immediately (e.g. user uses a prebuilt toolchain).

@pcmoore
Copy link
Member

pcmoore commented Oct 22, 2019

Hmm. I'm not a big fan of adding kludges to libseccomp to workaround problems in other libraries; it deteriorates the code over time and makes it harder to maintain. Have the uClibc-ng maintainers been responsive to the bug? If there is no hope of it getting fixed, maybe we can do something, but I would much prefer that uClibc-ng fix their code.

@casantos
Copy link
Author

I sent a patch a few days ago but so far I didn't receive any response from uClibc-ng maintainers.

Even if it gets fixed upstream the existing toolchains (e.g. from Bootlin and DENX) still contain earlier uClibc-ng versions and will unlikely be updated in a short period.

@pcmoore
Copy link
Member

pcmoore commented Oct 22, 2019

Even if it gets fixed upstream the existing toolchains (e.g. from Bootlin and DENX) still contain earlier uClibc-ng versions and will unlikely be updated in a short period.

I understand, but I consider this to be similar to distros which ship old and/or problematic libraries; it is a downstream problem and I don't want to pollute upstream unless there is no other way. In this case the Bootlin and DENX projects need to realize their toolchains are broken and fix them.

@casantos casantos closed this Oct 22, 2019
@pcmoore
Copy link
Member

pcmoore commented Oct 22, 2019

@casantos if this doesn't get resolved upstream within in a month or two, please reopen this issue.

buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request Oct 30, 2019
On uClibc up to at least v1.0.32, syscall() for x86_64 is defined in
libc/sysdeps/linux/x86_64/syscall.S as

syscall:
        movq %rdi, %rax         /* Syscall number -> rax.  */
        movq %rsi, %rdi         /* shift arg1 - arg5.  */
        movq %rdx, %rsi
        movq %rcx, %rdx
        movq %r8, %r10
        movq %r9, %r8
        movq 8(%rsp),%r9        /* arg6 is on the stack.  */
        syscall                 /* Do the system call.  */
        cmpq $-4095, %rax       /* Check %rax for error.  */
        jae __syscall_error     /* Branch forward if it failed.  */
        ret                     /* Return to caller.  */

And __syscall_error is defined in
libc/sysdeps/linux/x86_64/__syscall_error.c as

int __syscall_error(void) attribute_hidden;
int __syscall_error(void)
{
        register int err_no __asm__ ("%rcx");
        __asm__ ("mov %rax, %rcx\n\t"
                 "neg %rcx");
        __set_errno(err_no);
        return -1;
}

Notice that __syscall_error returns -1 as a 32-bit int in %rax, a 64-bit
register i.e. 0x00000000ffffffff (decimal 4294967295). When this value
is compared to -1 in _sys_chk_seccomp_flag_kernel() the result is false,
leading the function to always return 0.

Prevent the error by coercing the return value of syscall() to int in a
temporary variable before comparing it to -1. We could use just an (int)
cast but the variable makes the code more readable and the machine code
generated by the compiler is the same in both cases.

All other syscall() invocations were inspected and they either already
coerce the result to int or do not compare it to -1.

The same problem probably occurs on other 64-bit systems but so far only
x86_64 was tested.

A bug report is being submitted to uClibc.

Upstream status: seccomp/libseccomp#175

Signed-off-by: Carlos Santos <unixmania@gmail.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
(cherry picked from commit 440c7a9)
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request Oct 30, 2019
On uClibc up to at least v1.0.32, syscall() for x86_64 is defined in
libc/sysdeps/linux/x86_64/syscall.S as

syscall:
        movq %rdi, %rax         /* Syscall number -> rax.  */
        movq %rsi, %rdi         /* shift arg1 - arg5.  */
        movq %rdx, %rsi
        movq %rcx, %rdx
        movq %r8, %r10
        movq %r9, %r8
        movq 8(%rsp),%r9        /* arg6 is on the stack.  */
        syscall                 /* Do the system call.  */
        cmpq $-4095, %rax       /* Check %rax for error.  */
        jae __syscall_error     /* Branch forward if it failed.  */
        ret                     /* Return to caller.  */

And __syscall_error is defined in
libc/sysdeps/linux/x86_64/__syscall_error.c as

int __syscall_error(void) attribute_hidden;
int __syscall_error(void)
{
        register int err_no __asm__ ("%rcx");
        __asm__ ("mov %rax, %rcx\n\t"
                 "neg %rcx");
        __set_errno(err_no);
        return -1;
}

Notice that __syscall_error returns -1 as a 32-bit int in %rax, a 64-bit
register i.e. 0x00000000ffffffff (decimal 4294967295). When this value
is compared to -1 in _sys_chk_seccomp_flag_kernel() the result is false,
leading the function to always return 0.

Prevent the error by coercing the return value of syscall() to int in a
temporary variable before comparing it to -1. We could use just an (int)
cast but the variable makes the code more readable and the machine code
generated by the compiler is the same in both cases.

All other syscall() invocations were inspected and they either already
coerce the result to int or do not compare it to -1.

The same problem probably occurs on other 64-bit systems but so far only
x86_64 was tested.

A bug report is being submitted to uClibc.

Upstream status: seccomp/libseccomp#175

Signed-off-by: Carlos Santos <unixmania@gmail.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
(cherry picked from commit 440c7a9)
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants