| Message ID | 20151228010929.GA1819@altlinux.org |
|---|---|
| State | New |
| Headers | show |
On 28 Dec 2015 04:09, Dmitry V. Levin wrote: > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/personality.c > > + INTERNAL_SYSCALL_DECL (err); > + long ret = INTERNAL_SYSCALL (personality, err, 1, persona); > + > + /* Starting with kernel commit v2.6.29-6609-g11d06b2, personality syscall > + never fails to set the personality. However, due to architecture > + limitations of 32-bit kernels, personality syscall still may return > + an "error". */ > + if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (ret, err))) > + ret = -INTERNAL_SYSCALL_ERRNO (ret, err); > + return ret; > +} INTERNAL_SYSCALL returns the raw value from the kernel, so why do you need to test it directly ? > + if (personality (test_persona) != saved_persona || > + personality (0xffffffff) == -1 || > + personality (PER_LINUX) == -1 || > + personality (0xffffffff) != PER_LINUX) > + rc = 1; should this also verify errno is not changed ? > + (void) personality (saved_persona); we don't have __wur on this func, so the (void) is kind of pointless -mike
On Tue, Dec 29, 2015 at 04:42:18PM -0500, Mike Frysinger wrote: > On 28 Dec 2015 04:09, Dmitry V. Levin wrote: > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/personality.c > > > > + INTERNAL_SYSCALL_DECL (err); > > + long ret = INTERNAL_SYSCALL (personality, err, 1, persona); > > + > > + /* Starting with kernel commit v2.6.29-6609-g11d06b2, personality syscall > > + never fails to set the personality. However, due to architecture > > + limitations of 32-bit kernels, personality syscall still may return > > + an "error". */ > > + if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (ret, err))) > > + ret = -INTERNAL_SYSCALL_ERRNO (ret, err); > > + return ret; > > +} > > INTERNAL_SYSCALL returns the raw value from the kernel, so why do you > need to test it directly ? While some architectures (including x86/x86_64) use negated errno semantics, others (e.g. mips, nios2, powerpc, and sparc) use dedicated registers to return an error condition. In the latter case, the kernel syscall exit code negates the value returned by sys_personality and returns it to userspace as an "errno". It has to be negated again to recover the value returned by sys_personality. > > + if (personality (test_persona) != saved_persona || > > + personality (0xffffffff) == -1 || > > + personality (PER_LINUX) == -1 || > > + personality (0xffffffff) != PER_LINUX) > > + rc = 1; > > should this also verify errno is not changed ? Like this? errno = 0xdefaced; if (personality (test_persona) != saved_persona || personality (0xffffffff) == -1 || personality (PER_LINUX) == -1 || personality (0xffffffff) != PER_LINUX || 0xdefaced != errno) rc = 1; > > + (void) personality (saved_persona); > > we don't have __wur on this func, so the (void) is kind of pointless This is my way to show that in this case, unlike others, the return code is irrelevant. If you or somebody else don't like this style, I have no problem removing the (void).
On 30 Dec 2015 02:27, Dmitry V. Levin wrote: > On Tue, Dec 29, 2015 at 04:42:18PM -0500, Mike Frysinger wrote: > > On 28 Dec 2015 04:09, Dmitry V. Levin wrote: > > > --- /dev/null > > > +++ b/sysdeps/unix/sysv/linux/personality.c > > > > > > + INTERNAL_SYSCALL_DECL (err); > > > + long ret = INTERNAL_SYSCALL (personality, err, 1, persona); > > > + > > > + /* Starting with kernel commit v2.6.29-6609-g11d06b2, personality syscall > > > + never fails to set the personality. However, due to architecture > > > + limitations of 32-bit kernels, personality syscall still may return > > > + an "error". */ > > > + if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (ret, err))) > > > + ret = -INTERNAL_SYSCALL_ERRNO (ret, err); > > > + return ret; > > > +} > > > > INTERNAL_SYSCALL returns the raw value from the kernel, so why do you > > need to test it directly ? > > While some architectures (including x86/x86_64) use negated errno semantics, > others (e.g. mips, nios2, powerpc, and sparc) use dedicated registers to return > an error condition. In the latter case, the kernel syscall exit code negates > the value returned by sys_personality and returns it to userspace as an > "errno". It has to be negated again to recover the value returned by > sys_personality. i'm aware of some arches doing split error paths, but doesn't the kernel already handle this ? i'm not seeing this sort of logic for ptrace or mmap calls where any value is valid. i guess the commit could use clarification here. -mike
On Tue, Dec 29, 2015 at 06:50:07PM -0500, Mike Frysinger wrote: > On 30 Dec 2015 02:27, Dmitry V. Levin wrote: > > On Tue, Dec 29, 2015 at 04:42:18PM -0500, Mike Frysinger wrote: > > > On 28 Dec 2015 04:09, Dmitry V. Levin wrote: > > > > --- /dev/null > > > > +++ b/sysdeps/unix/sysv/linux/personality.c > > > > > > > > + INTERNAL_SYSCALL_DECL (err); > > > > + long ret = INTERNAL_SYSCALL (personality, err, 1, persona); > > > > + > > > > + /* Starting with kernel commit v2.6.29-6609-g11d06b2, personality syscall > > > > + never fails to set the personality. However, due to architecture > > > > + limitations of 32-bit kernels, personality syscall still may return > > > > + an "error". */ > > > > + if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (ret, err))) > > > > + ret = -INTERNAL_SYSCALL_ERRNO (ret, err); > > > > + return ret; > > > > +} > > > > > > INTERNAL_SYSCALL returns the raw value from the kernel, so why do you > > > need to test it directly ? > > > > While some architectures (including x86/x86_64) use negated errno semantics, > > others (e.g. mips, nios2, powerpc, and sparc) use dedicated registers to return > > an error condition. In the latter case, the kernel syscall exit code negates > > the value returned by sys_personality and returns it to userspace as an > > "errno". It has to be negated again to recover the value returned by > > sys_personality. > > i'm aware of some arches doing split error paths, but doesn't the kernel > already handle this ? Unfortunately, it doesn't. > i'm not seeing this sort of logic for ptrace or > mmap calls where any value is valid. mmap doesn't have this issue because you cannot trick it to return an address >= -4095. ptrace doesn't have this issue because it doesn't return arbitrary values, PTRACE_PEEK{TEXT,DATA,USER) return value is stored in memory, see sysdeps/unix/sysv/linux/ptrace.c. > i guess the commit could use clarification here. The subject is tricky and the commit is quite lengthy already. What would you like to be clarified?
On 30 Dec 2015 03:26, Dmitry V. Levin wrote: > On Tue, Dec 29, 2015 at 06:50:07PM -0500, Mike Frysinger wrote: > > i guess the commit could use clarification here. > > The subject is tricky and the commit is quite lengthy already. > What would you like to be clarified? /* Starting with kernel commit v2.6.29-6609-g11d06b2, the personality syscall never fails. However, 32-bit kernels might flag valid values as errors, so we need to reverse the error setting. We can't use the raw result as some arches split the return/error values. */ -mike
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index f6269ea..9999600 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -16,7 +16,8 @@ include $(firstword $(wildcard $(sysdirs:=/sysctl.mk))) sysdep_routines += clone llseek umount umount2 readahead \ setfsuid setfsgid makedev epoll_pwait signalfd \ - eventfd eventfd_read eventfd_write prlimit + eventfd eventfd_read eventfd_write prlimit \ + personality CFLAGS-gethostid.c = -fexceptions CFLAGS-tst-writev.c += "-DARTIFICIAL_LIMIT=0x80000000-__getpagesize()" @@ -41,7 +42,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \ bits/socket_type.h bits/syscall.h bits/sysctl.h \ bits/mman-linux.h -tests += tst-clone tst-fanotify +tests += tst-clone tst-fanotify tst-personality # Generate the list of SYS_* macros for the system calls (__NR_* macros). diff --git a/sysdeps/unix/sysv/linux/arm/syscalls.list b/sysdeps/unix/sysv/linux/arm/syscalls.list index 911ed7d..c06954f 100644 --- a/sysdeps/unix/sysv/linux/arm/syscalls.list +++ b/sysdeps/unix/sysv/linux/arm/syscalls.list @@ -19,6 +19,8 @@ prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiiis fanotify_mark +personality EXTRA personality Ei:i __personality personality + # Semaphore and shm system calls. msgctl, shmctl, and semctl have C # wrappers (to set __IPC_64). msgget - msgget i:ii __msgget msgget diff --git a/sysdeps/unix/sysv/linux/hppa/syscalls.list b/sysdeps/unix/sysv/linux/hppa/syscalls.list index 2cb8d02..d29c358 100644 --- a/sysdeps/unix/sysv/linux/hppa/syscalls.list +++ b/sysdeps/unix/sysv/linux/hppa/syscalls.list @@ -37,3 +37,4 @@ setrlimit - setrlimit i:ip __setrlimit setrlimit getrlimit - getrlimit i:ip __getrlimit getrlimit prlimit64 EXTRA prlimit64 i:iipp __prlimit64 prlimit64@@GLIBC_2.17 fanotify_mark EXTRA fanotify_mark i:iiiiis __fanotify_mark fanotify_mark@@GLIBC_2.19 +personality EXTRA personality Ei:i __personality personality diff --git a/sysdeps/unix/sysv/linux/i386/syscalls.list b/sysdeps/unix/sysv/linux/i386/syscalls.list index 1cebd6a..6282ff8 100644 --- a/sysdeps/unix/sysv/linux/i386/syscalls.list +++ b/sysdeps/unix/sysv/linux/i386/syscalls.list @@ -25,3 +25,5 @@ waitpid - waitpid Ci:ipi __waitpid waitpid prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiiis fanotify_mark + +personality EXTRA personality Ei:i __personality personality diff --git a/sysdeps/unix/sysv/linux/m68k/syscalls.list b/sysdeps/unix/sysv/linux/m68k/syscalls.list index ad4ca46..4260f3e 100644 --- a/sysdeps/unix/sysv/linux/m68k/syscalls.list +++ b/sysdeps/unix/sysv/linux/m68k/syscalls.list @@ -19,3 +19,4 @@ setfsuid - setfsuid32 Ei:i setfsuid cacheflush EXTRA cacheflush i:iiii __cacheflush cacheflush prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiiis fanotify_mark +personality EXTRA personality Ei:i __personality personality diff --git a/sysdeps/unix/sysv/linux/microblaze/syscalls.list b/sysdeps/unix/sysv/linux/microblaze/syscalls.list index 86fd40b..da0fd4e 100644 --- a/sysdeps/unix/sysv/linux/microblaze/syscalls.list +++ b/sysdeps/unix/sysv/linux/microblaze/syscalls.list @@ -4,6 +4,7 @@ cacheflush EXTRA cacheflush i:iiii __cacheflush cacheflush prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiiis fanotify_mark +personality EXTRA personality Ei:i __personality personality # Semaphore and shm system calls. msgctl, shmctl, and semctl have C # wrappers (to set __IPC_64). diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list b/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list index 7ad5523..584ad1f 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/syscalls.list @@ -6,3 +6,5 @@ sync_file_range - sync_file_range Ci:iiii sync_file_range prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiis fanotify_mark + +personality EXTRA personality Ei:i __personality personality diff --git a/sysdeps/unix/sysv/linux/personality.c b/sysdeps/unix/sysv/linux/personality.c new file mode 100644 index 0000000..be5ada7 --- /dev/null +++ b/sysdeps/unix/sysv/linux/personality.c @@ -0,0 +1,49 @@ +/* Copyright (C) 2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <sys/personality.h> +#include <sysdep.h> + +extern __typeof (personality) __personality; + +int +__personality (unsigned long persona) +{ +#ifdef PERSONALITY_TRUNCATE_ARGUMENT + /* Starting with kernel commit v2.6.21-3117-g97dc32c, the type of + task_struct->pesonality is "unsigned int". + Starting with kernel commit v2.6.35-rc1-372-g485d527, personality + syscall accepts "unsigned int" instead of "long unsigned int". + Inbetween, a personality value that does not fit into "unsigned int" + would result to system call returning -EINVAL. + We explicitly truncate the personality value to "unsigned int" + to eliminate the uncertainty. */ + persona = (unsigned int) persona; +#endif + + INTERNAL_SYSCALL_DECL (err); + long ret = INTERNAL_SYSCALL (personality, err, 1, persona); + + /* Starting with kernel commit v2.6.29-6609-g11d06b2, personality syscall + never fails to set the personality. However, due to architecture + limitations of 32-bit kernels, personality syscall still may return + an "error". */ + if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (ret, err))) + ret = -INTERNAL_SYSCALL_ERRNO (ret, err); + return ret; +} +weak_alias (__personality, personality) diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscalls.list b/sysdeps/unix/sysv/linux/s390/s390-32/syscalls.list index 82baf9c..141b165 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/syscalls.list +++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscalls.list @@ -20,3 +20,4 @@ setrlimit - setrlimit i:ip __setrlimit setrlimit@GLIBC_2.0 setrlimit@@GLIBC_2.2 prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiiis fanotify_mark +personality EXTRA personality Ei:i __personality personality diff --git a/sysdeps/unix/sysv/linux/sh/syscalls.list b/sysdeps/unix/sysv/linux/sh/syscalls.list index 2f4ac65..169d40f 100644 --- a/sysdeps/unix/sysv/linux/sh/syscalls.list +++ b/sysdeps/unix/sysv/linux/sh/syscalls.list @@ -20,3 +20,5 @@ waitpid - waitpid Ci:ipi __waitpid waitpid prlimit64 EXTRA prlimit64 i:iipp prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiiis __fanotify_mark fanotify_mark@@GLIBC_2.16 + +personality EXTRA personality Ei:i __personality personality diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/personality.c b/sysdeps/unix/sysv/linux/sparc/sparc64/personality.c new file mode 100644 index 0000000..250e501 --- /dev/null +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/personality.c @@ -0,0 +1,3 @@ +/* Work around sign extension bug in the kernel. */ +#define PERSONALITY_TRUNCATE_ARGUMENT +#include <sysdeps/unix/sysv/linux/personality.c> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list index caa6ccf..afaf033 100644 --- a/sysdeps/unix/sysv/linux/syscalls.list +++ b/sysdeps/unix/sysv/linux/syscalls.list @@ -46,7 +46,6 @@ munlockall - munlockall i: munlockall nanosleep - nanosleep Ci:pp __nanosleep nanosleep nfsservctl EXTRA nfsservctl i:ipp nfsservctl pause - pause Ci: __libc_pause pause -personality EXTRA personality i:i __personality personality pipe - pipe i:f __pipe pipe pipe2 - pipe2 i:fi __pipe2 pipe2 pivot_root EXTRA pivot_root i:ss pivot_root diff --git a/sysdeps/unix/sysv/linux/tst-personality.c b/sysdeps/unix/sysv/linux/tst-personality.c new file mode 100644 index 0000000..2763db2 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-personality.c @@ -0,0 +1,41 @@ +/* BZ #19408 linux personality syscall wrapper test. + + Copyright (C) 2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <sys/personality.h> + +static int +do_test (void) +{ + int rc = 0; + unsigned int test_persona = -EINVAL; + unsigned int saved_persona = personality (0xffffffff); + + if (personality (test_persona) != saved_persona || + personality (0xffffffff) == -1 || + personality (PER_LINUX) == -1 || + personality (0xffffffff) != PER_LINUX) + rc = 1; + + (void) personality (saved_persona); + return rc; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list index 51ee8d8..19cc6d9 100644 --- a/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list +++ b/sysdeps/unix/sysv/linux/wordsize-64/syscalls.list @@ -20,3 +20,4 @@ open - open Ci:siv __libc_open __open open __open64 open64 prlimit EXTRA prlimit64 i:iipp prlimit prlimit64 fanotify_mark EXTRA fanotify_mark i:iiiis fanotify_mark +personality EXTRA personality i:i __personality personality diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list b/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list index 2cc58af..c98ac74 100644 --- a/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list +++ b/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list @@ -2,6 +2,7 @@ fallocate - fallocate Ci:iiii fallocate fallocate64 gettimeofday - gettimeofday:__vdso_gettimeofday@LINUX_2.6 i:pP __gettimeofday gettimeofday +personality EXTRA personality Ei:i __personality personality posix_fadvise - fadvise64 Vi:iiii posix_fadvise posix_fadvise64 preadv - preadv Ci:ipii preadv preadv64 pwritev - pwritev Ci:ipii pwritev pwritev64