* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Jann Horn @ 2018-10-05 14:12 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <CACAyw99ar0Wokyg4jx8FPyr1Z=7Cf+=WVgF6sLL1FK9JtHROjw@mail.gmail.com>
On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> >
> > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > check in here, right? I doubt it matters, but I don't really like
> > seeing something like this exposed to unprivileged userspace just
> > because you need it for kernel testing.
>
> That would mean all tests have to run as root / with CAP_SYS_ADMIN
> which isn't ideal.
This patch basically means that it becomes easier for a local user to
construct a BPF hash table that has all of its values stuffed into a
single hash bucket, correct? Which makes it easier to create a BPF
program that generates unusually large RCU stalls by performing ~40000
BPF map lookups, each of which has to walk through the entire linked
list of the hash map bucket? I dislike exposing something like that to
unprivileged userspace.
And if you want to run the whole BPF test suite with all its tests,
don't you already need root privileges? Or is this a different test
suite?
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-05 7:42 UTC (permalink / raw)
To: jannh; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAG48ez0_BtXQ2KCgpRicQ8yfeNqV2mN8RKCA3nn8DXFTmsj6UA@mail.gmail.com>
On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
>
> If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> check in here, right? I doubt it matters, but I don't really like
> seeing something like this exposed to unprivileged userspace just
> because you need it for kernel testing.
That would mean all tests have to run as root / with CAP_SYS_ADMIN
which isn't ideal.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* [REVIEW][PATCH 7/6] signal: In sigqueueinfo prefer sig not si_signo
From: Eric W. Biederman @ 2018-10-05 7:10 UTC (permalink / raw)
To: Andrei Vagin
Cc: linux-kernel, linux-api, linux-arch, Oleg Nesterov,
Linus Torvalds
In-Reply-To: <20181005060611.GA19061@gmail.com>
Andrei Vagin <avagin@gmail.com> reported:
> Accoding to the man page, the user should not set si_signo, it has to be set
> by kernel.
>
> $ man 2 rt_sigqueueinfo
>
> The uinfo argument specifies the data to accompany the signal. This
> argument is a pointer to a structure of type siginfo_t, described in
> sigaction(2) (and defined by including <sigaction.h>). The caller
> should set the following fields in this structure:
>
> si_code
> This must be one of the SI_* codes in the Linux kernel source
> file include/asm-generic/siginfo.h, with the restriction that
> the code must be negative (i.e., cannot be SI_USER, which is
> used by the kernel to indicate a signal sent by kill(2)) and
> cannot (since Linux 2.6.39) be SI_TKILL (which is used by the
> kernel to indicate a signal sent using tgkill(2)).
>
> si_pid This should be set to a process ID, typically the process ID of
> the sender.
>
> si_uid This should be set to a user ID, typically the real user ID of
> the sender.
>
> si_value
> This field contains the user data to accompany the signal. For
> more information, see the description of the last (union sigval)
> argument of sigqueue(3).
>
> Internally, the kernel sets the si_signo field to the value specified
> in sig, so that the receiver of the signal can also obtain the signal
> number via that field.
>
> On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote:
>>
>> If there is some application that calls sigqueueinfo directly that has
>> a problem with this added sanity check we can revisit this when we see
>> what kind of crazy that application is doing.
>
>
> I already know two "applications" ;)
>
> https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c
> https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c
>
> Disclaimer: I'm the author of both of them.
Looking at the kernel code the historical behavior has alwasy been to prefer
the signal number passed in by the kernel.
So sigh. Implmenet __copy_siginfo_from_user and __copy_siginfo_from_user32 to
take that signal number and prefer it. The user of ptrace will still
use copy_siginfo_from_user and copy_siginfo_from_user32 as they do not and
never have had a signal number there.
Luckily this change has never made it farther than linux-next.
Fixes: e75dc036c445 ("signal: Fail sigqueueinfo if si_signo != sig")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
Andrei can you verify this fixes your programs?
Thank you very much,
Eric
kernel/signal.c | 141 ++++++++++++++++++++++++++++--------------------
1 file changed, 84 insertions(+), 57 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index 1c2dd117fee0..2bffc5a50183 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2925,11 +2925,10 @@ int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
return 0;
}
-int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from)
+static int post_copy_siginfo_from_user(kernel_siginfo_t *info,
+ const siginfo_t __user *from)
{
- if (copy_from_user(to, from, sizeof(struct kernel_siginfo)))
- return -EFAULT;
- if (unlikely(!known_siginfo_layout(to->si_signo, to->si_code))) {
+ if (unlikely(!known_siginfo_layout(info->si_signo, info->si_code))) {
char __user *expansion = si_expansion(from);
char buf[SI_EXPANSION_SIZE];
int i;
@@ -2949,6 +2948,22 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from)
return 0;
}
+static int __copy_siginfo_from_user(int signo, kernel_siginfo_t *to,
+ const siginfo_t __user *from)
+{
+ if (copy_from_user(to, from, sizeof(struct kernel_siginfo)))
+ return -EFAULT;
+ to->si_signo = signo;
+ return post_copy_siginfo_from_user(to, from);
+}
+
+int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from)
+{
+ if (copy_from_user(to, from, sizeof(struct kernel_siginfo)))
+ return -EFAULT;
+ return post_copy_siginfo_from_user(to, from);
+}
+
#ifdef CONFIG_COMPAT
int copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from)
@@ -3041,88 +3056,106 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
return 0;
}
-int copy_siginfo_from_user32(struct kernel_siginfo *to,
- const struct compat_siginfo __user *ufrom)
+static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
+ const struct compat_siginfo *from)
{
- struct compat_siginfo from;
-
- if (copy_from_user(&from, ufrom, sizeof(struct compat_siginfo)))
- return -EFAULT;
-
clear_siginfo(to);
- to->si_signo = from.si_signo;
- to->si_errno = from.si_errno;
- to->si_code = from.si_code;
- switch(siginfo_layout(from.si_signo, from.si_code)) {
+ to->si_signo = from->si_signo;
+ to->si_errno = from->si_errno;
+ to->si_code = from->si_code;
+ switch(siginfo_layout(from->si_signo, from->si_code)) {
case SIL_KILL:
- to->si_pid = from.si_pid;
- to->si_uid = from.si_uid;
+ to->si_pid = from->si_pid;
+ to->si_uid = from->si_uid;
break;
case SIL_TIMER:
- to->si_tid = from.si_tid;
- to->si_overrun = from.si_overrun;
- to->si_int = from.si_int;
+ to->si_tid = from->si_tid;
+ to->si_overrun = from->si_overrun;
+ to->si_int = from->si_int;
break;
case SIL_POLL:
- to->si_band = from.si_band;
- to->si_fd = from.si_fd;
+ to->si_band = from->si_band;
+ to->si_fd = from->si_fd;
break;
case SIL_FAULT:
- to->si_addr = compat_ptr(from.si_addr);
+ to->si_addr = compat_ptr(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- to->si_trapno = from.si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
break;
case SIL_FAULT_MCEERR:
- to->si_addr = compat_ptr(from.si_addr);
+ to->si_addr = compat_ptr(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- to->si_trapno = from.si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
- to->si_addr_lsb = from.si_addr_lsb;
+ to->si_addr_lsb = from->si_addr_lsb;
break;
case SIL_FAULT_BNDERR:
- to->si_addr = compat_ptr(from.si_addr);
+ to->si_addr = compat_ptr(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- to->si_trapno = from.si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
- to->si_lower = compat_ptr(from.si_lower);
- to->si_upper = compat_ptr(from.si_upper);
+ to->si_lower = compat_ptr(from->si_lower);
+ to->si_upper = compat_ptr(from->si_upper);
break;
case SIL_FAULT_PKUERR:
- to->si_addr = compat_ptr(from.si_addr);
+ to->si_addr = compat_ptr(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- to->si_trapno = from.si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
- to->si_pkey = from.si_pkey;
+ to->si_pkey = from->si_pkey;
break;
case SIL_CHLD:
- to->si_pid = from.si_pid;
- to->si_uid = from.si_uid;
- to->si_status = from.si_status;
+ to->si_pid = from->si_pid;
+ to->si_uid = from->si_uid;
+ to->si_status = from->si_status;
#ifdef CONFIG_X86_X32_ABI
if (in_x32_syscall()) {
- to->si_utime = from._sifields._sigchld_x32._utime;
- to->si_stime = from._sifields._sigchld_x32._stime;
+ to->si_utime = from->_sifields._sigchld_x32._utime;
+ to->si_stime = from->_sifields._sigchld_x32._stime;
} else
#endif
{
- to->si_utime = from.si_utime;
- to->si_stime = from.si_stime;
+ to->si_utime = from->si_utime;
+ to->si_stime = from->si_stime;
}
break;
case SIL_RT:
- to->si_pid = from.si_pid;
- to->si_uid = from.si_uid;
- to->si_int = from.si_int;
+ to->si_pid = from->si_pid;
+ to->si_uid = from->si_uid;
+ to->si_int = from->si_int;
break;
case SIL_SYS:
- to->si_call_addr = compat_ptr(from.si_call_addr);
- to->si_syscall = from.si_syscall;
- to->si_arch = from.si_arch;
+ to->si_call_addr = compat_ptr(from->si_call_addr);
+ to->si_syscall = from->si_syscall;
+ to->si_arch = from->si_arch;
break;
}
return 0;
}
+
+static int __copy_siginfo_from_user32(int signo, struct kernel_siginfo *to,
+ const struct compat_siginfo __user *ufrom)
+{
+ struct compat_siginfo from;
+
+ if (copy_from_user(&from, ufrom, sizeof(struct compat_siginfo)))
+ return -EFAULT;
+
+ from.si_signo = signo;
+ return post_copy_siginfo_from_user32(to, &from);
+}
+
+int copy_siginfo_from_user32(struct kernel_siginfo *to,
+ const struct compat_siginfo __user *ufrom)
+{
+ struct compat_siginfo from;
+
+ if (copy_from_user(&from, ufrom, sizeof(struct compat_siginfo)))
+ return -EFAULT;
+
+ return post_copy_siginfo_from_user32(to, &from);
+}
#endif /* CONFIG_COMPAT */
/**
@@ -3359,9 +3392,6 @@ static int do_rt_sigqueueinfo(pid_t pid, int sig, kernel_siginfo_t *info)
(task_pid_vnr(current) != pid))
return -EPERM;
- if (info->si_signo != sig)
- return -EINVAL;
-
/* POSIX.1b doesn't mention process groups. */
return kill_proc_info(sig, info, pid);
}
@@ -3376,7 +3406,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
siginfo_t __user *, uinfo)
{
kernel_siginfo_t info;
- int ret = copy_siginfo_from_user(&info, uinfo);
+ int ret = __copy_siginfo_from_user(sig, &info, uinfo);
if (unlikely(ret))
return ret;
return do_rt_sigqueueinfo(pid, sig, &info);
@@ -3389,7 +3419,7 @@ COMPAT_SYSCALL_DEFINE3(rt_sigqueueinfo,
struct compat_siginfo __user *, uinfo)
{
kernel_siginfo_t info;
- int ret = copy_siginfo_from_user32(&info, uinfo);
+ int ret = __copy_siginfo_from_user32(sig, &info, uinfo);
if (unlikely(ret))
return ret;
return do_rt_sigqueueinfo(pid, sig, &info);
@@ -3409,9 +3439,6 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, kernel_siginfo_t
(task_pid_vnr(current) != pid))
return -EPERM;
- if (info->si_signo != sig)
- return -EINVAL;
-
return do_send_specific(tgid, pid, sig, info);
}
@@ -3419,7 +3446,7 @@ SYSCALL_DEFINE4(rt_tgsigqueueinfo, pid_t, tgid, pid_t, pid, int, sig,
siginfo_t __user *, uinfo)
{
kernel_siginfo_t info;
- int ret = copy_siginfo_from_user(&info, uinfo);
+ int ret = __copy_siginfo_from_user(sig, &info, uinfo);
if (unlikely(ret))
return ret;
return do_rt_tgsigqueueinfo(tgid, pid, sig, &info);
@@ -3433,7 +3460,7 @@ COMPAT_SYSCALL_DEFINE4(rt_tgsigqueueinfo,
struct compat_siginfo __user *, uinfo)
{
kernel_siginfo_t info;
- int ret = copy_siginfo_from_user32(&info, uinfo);
+ int ret = __copy_siginfo_from_user32(sig, &info, uinfo);
if (unlikely(ret))
return ret;
return do_rt_tgsigqueueinfo(tgid, pid, sig, &info);
^ permalink raw reply related
* Re: [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig
From: Eric W. Biederman @ 2018-10-05 6:50 UTC (permalink / raw)
To: Andrei Vagin
Cc: linux-kernel, linux-api, linux-arch, Oleg Nesterov,
Linus Torvalds
In-Reply-To: <20181005060611.GA19061@gmail.com>
Andrei Vagin <avagin@gmail.com> writes:
> On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote:
>> The kernel needs to validate that the contents of struct siginfo make
>> sense as siginfo is copied into the kernel, so that the proper union
>> members can be put in the appropriate locations. The field si_signo
>> is a fundamental part of that validation. As such changing the
>> contents of si_signo after the validation make no sense and can result
>> in nonsense values in the kernel.
>
> Accoding to the man page, the user should not set si_signo, it has to be set
> by kernel.
I wanted to say look at copy_siginfo_from_user32 it uses si_signo and it
has always done so. But before I got to fixing copy_siginfo_from_user32
only looked at si_code. This is one of the areas where we deliberately
slightly changed the KABI. To start looking an signo instead of magic
kernel internal si_code values.
So yes. Looking at si_signo instead of the passed in signo appears to
be a regression all of the way around (except for ptrace) where that
value is not present. So I will see if I can figure out how to refactor
the code to accomplish that.
I am travelling for the next couple of days so I won't be able to get to
this for a bit.
I am thinking I will need two version of copy_siginfo_from user now.
One for ptrace and one for rt_sgiqueueinfo. Sigh.
> $ man 2 rt_sigqueueinfo
>
> The uinfo argument specifies the data to accompany the signal. This
> argument is a pointer to a structure of type siginfo_t, described in
> sigaction(2) (and defined by including <sigaction.h>). The caller
> should set the following fields in this structure:
>
> si_code
> This must be one of the SI_* codes in the Linux kernel source
> file include/asm-generic/siginfo.h, with the restriction that
> the code must be negative (i.e., cannot be SI_USER, which is
> used by the kernel to indicate a signal sent by kill(2)) and
> cannot (since Linux 2.6.39) be SI_TKILL (which is used by the
> kernel to indicate a signal sent using tgkill(2)).
>
> si_pid This should be set to a process ID, typically the process ID of
> the sender.
>
> si_uid This should be set to a user ID, typically the real user ID of
> the sender.
>
> si_value
> This field contains the user data to accompany the signal. For
> more information, see the description of the last (union sigval)
> argument of sigqueue(3).
>
> Internally, the kernel sets the si_signo field to the value specified
> in sig, so that the receiver of the signal can also obtain the signal
> number via that field.
>
>>
>> As such simply fail if someone is silly enough to set si_signo out of
>> sync with the signal number passed to sigqueueinfo.
>>
>> I don't expect a problem as glibc's sigqueue implementation sets
>> "si_signo = sig" and CRIU just returns to the kernel what the kernel
>> gave to it.
>>
>> If there is some application that calls sigqueueinfo directly that has
>> a problem with this added sanity check we can revisit this when we see
>> what kind of crazy that application is doing.
>
>
> I already know two "applications" ;)
>
> https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c
> https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c
>
> Disclaimer: I'm the author of both of them.
>
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>> kernel/signal.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index 7b49c31d3fdb..e445b0a63faa 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -3306,7 +3306,8 @@ static int do_rt_sigqueueinfo(pid_t pid, int sig, siginfo_t *info)
>> (task_pid_vnr(current) != pid))
>> return -EPERM;
>>
>> - info->si_signo = sig;
>> + if (info->si_signo != sig)
>> + return -EINVAL;
>>
>> /* POSIX.1b doesn't mention process groups. */
>> return kill_proc_info(sig, info, pid);
>> @@ -3354,7 +3355,8 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
>> (task_pid_vnr(current) != pid))
>> return -EPERM;
>>
>> - info->si_signo = sig;
>> + if (info->si_signo != sig)
>> + return -EINVAL;
>>
>> return do_send_specific(tgid, pid, sig, info);
>> }
^ permalink raw reply
* Re: [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig
From: Eric W. Biederman @ 2018-10-05 6:27 UTC (permalink / raw)
To: Andrei Vagin
Cc: linux-kernel, linux-api, linux-arch, Oleg Nesterov,
Linus Torvalds
In-Reply-To: <20181005060611.GA19061@gmail.com>
Andrei Vagin <avagin@gmail.com> writes:
> On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote:
>> The kernel needs to validate that the contents of struct siginfo make
>> sense as siginfo is copied into the kernel, so that the proper union
>> members can be put in the appropriate locations. The field si_signo
>> is a fundamental part of that validation. As such changing the
>> contents of si_signo after the validation make no sense and can result
>> in nonsense values in the kernel.
>
> Accoding to the man page, the user should not set si_signo, it has to be set
> by kernel.
>
> $ man 2 rt_sigqueueinfo
>
> The uinfo argument specifies the data to accompany the signal. This
> argument is a pointer to a structure of type siginfo_t, described in
> sigaction(2) (and defined by including <sigaction.h>). The caller
> should set the following fields in this structure:
>
> si_code
> This must be one of the SI_* codes in the Linux kernel source
> file include/asm-generic/siginfo.h, with the restriction that
> the code must be negative (i.e., cannot be SI_USER, which is
> used by the kernel to indicate a signal sent by kill(2)) and
> cannot (since Linux 2.6.39) be SI_TKILL (which is used by the
> kernel to indicate a signal sent using tgkill(2)).
>
> si_pid This should be set to a process ID, typically the process ID of
> the sender.
>
> si_uid This should be set to a user ID, typically the real user ID of
> the sender.
>
> si_value
> This field contains the user data to accompany the signal. For
> more information, see the description of the last (union sigval)
> argument of sigqueue(3).
>
> Internally, the kernel sets the si_signo field to the value specified
> in sig, so that the receiver of the signal can also obtain the signal
> number via that field.
>
>>
>> As such simply fail if someone is silly enough to set si_signo out of
>> sync with the signal number passed to sigqueueinfo.
>>
>> I don't expect a problem as glibc's sigqueue implementation sets
>> "si_signo = sig" and CRIU just returns to the kernel what the kernel
>> gave to it.
>>
>> If there is some application that calls sigqueueinfo directly that has
>> a problem with this added sanity check we can revisit this when we see
>> what kind of crazy that application is doing.
>
>
> I already know two "applications" ;)
>
> https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c
> https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c
>
> Disclaimer: I'm the author of both of them.
Fair enough. Then this counts as a regression. The setting in the
kernel happens in an awkward place and I will see if it can be moved
earlier.
Eric
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>> kernel/signal.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index 7b49c31d3fdb..e445b0a63faa 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -3306,7 +3306,8 @@ static int do_rt_sigqueueinfo(pid_t pid, int sig, siginfo_t *info)
>> (task_pid_vnr(current) != pid))
>> return -EPERM;
>>
>> - info->si_signo = sig;
>> + if (info->si_signo != sig)
>> + return -EINVAL;
>>
>> /* POSIX.1b doesn't mention process groups. */
>> return kill_proc_info(sig, info, pid);
>> @@ -3354,7 +3355,8 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
>> (task_pid_vnr(current) != pid))
>> return -EPERM;
>>
>> - info->si_signo = sig;
>> + if (info->si_signo != sig)
>> + return -EINVAL;
>>
>> return do_send_specific(tgid, pid, sig, info);
>> }
^ permalink raw reply
* Re: [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig
From: Andrei Vagin @ 2018-10-05 6:06 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, linux-api, linux-arch, Oleg Nesterov,
Linus Torvalds
In-Reply-To: <20180925171906.19683-2-ebiederm@xmission.com>
On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote:
> The kernel needs to validate that the contents of struct siginfo make
> sense as siginfo is copied into the kernel, so that the proper union
> members can be put in the appropriate locations. The field si_signo
> is a fundamental part of that validation. As such changing the
> contents of si_signo after the validation make no sense and can result
> in nonsense values in the kernel.
Accoding to the man page, the user should not set si_signo, it has to be set
by kernel.
$ man 2 rt_sigqueueinfo
The uinfo argument specifies the data to accompany the signal. This
argument is a pointer to a structure of type siginfo_t, described in
sigaction(2) (and defined by including <sigaction.h>). The caller
should set the following fields in this structure:
si_code
This must be one of the SI_* codes in the Linux kernel source
file include/asm-generic/siginfo.h, with the restriction that
the code must be negative (i.e., cannot be SI_USER, which is
used by the kernel to indicate a signal sent by kill(2)) and
cannot (since Linux 2.6.39) be SI_TKILL (which is used by the
kernel to indicate a signal sent using tgkill(2)).
si_pid This should be set to a process ID, typically the process ID of
the sender.
si_uid This should be set to a user ID, typically the real user ID of
the sender.
si_value
This field contains the user data to accompany the signal. For
more information, see the description of the last (union sigval)
argument of sigqueue(3).
Internally, the kernel sets the si_signo field to the value specified
in sig, so that the receiver of the signal can also obtain the signal
number via that field.
>
> As such simply fail if someone is silly enough to set si_signo out of
> sync with the signal number passed to sigqueueinfo.
>
> I don't expect a problem as glibc's sigqueue implementation sets
> "si_signo = sig" and CRIU just returns to the kernel what the kernel
> gave to it.
>
> If there is some application that calls sigqueueinfo directly that has
> a problem with this added sanity check we can revisit this when we see
> what kind of crazy that application is doing.
I already know two "applications" ;)
https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c
https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c
Disclaimer: I'm the author of both of them.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> kernel/signal.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7b49c31d3fdb..e445b0a63faa 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3306,7 +3306,8 @@ static int do_rt_sigqueueinfo(pid_t pid, int sig, siginfo_t *info)
> (task_pid_vnr(current) != pid))
> return -EPERM;
>
> - info->si_signo = sig;
> + if (info->si_signo != sig)
> + return -EINVAL;
>
> /* POSIX.1b doesn't mention process groups. */
> return kill_proc_info(sig, info, pid);
> @@ -3354,7 +3355,8 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
> (task_pid_vnr(current) != pid))
> return -EPERM;
>
> - info->si_signo = sig;
> + if (info->si_signo != sig)
> + return -EINVAL;
>
> return do_send_specific(tgid, pid, sig, info);
> }
^ permalink raw reply
* Re: [PATCH 00/34] VFS: Introduce filesystem context [ver #12]
From: Eric W. Biederman @ 2018-10-04 18:37 UTC (permalink / raw)
To: David Howells
Cc: viro, John Johansen, Tejun Heo, Eric W. Biederman, selinux,
Paul Moore, Li Zefan, linux-api, apparmor, Casey Schaufler,
fenghua.yu, Greg Kroah-Hartman, Eric Biggers,
linux-security-module, Tetsuo Handa, Johannes Weiner,
Stephen Smalley, tomoyo-dev-en, cgroups, torvalds, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <153754740781.17872.7869536526927736855.stgit@warthog.procyon.org.uk>
David,
I have been going through these and it is a wonderful proof of concept
patchset. There are a couple significant problems with it however.
- Many patches do more than one thing that could benefit from being
broken up into more patches so that there is only one logical change
per patch. I have attempted a little of that and have found several
significant bugs.
- There are many unnecessary changes in this patchset that just add
noise and make it difficult to review.
- There are many typos and thinkos in this patchset that while not hard
to correct keep this from being anywhere close to being ready for
prime time.
- Some of the bugs I have encountered.
* proc that isn't pid_ns_prepare_proc does not set fc->user_ns to
match the pid namespace.
* mqueue does not set fc->user_ns to match the ipc namespace.
* The cpuset filesystem always fails to mount
* Non-converted filesystems don't have the old security hooks
and only have a bit blob so don't call into the new security
hooks either.
* The changes to implement the new security hooks at least for
selinux are riddled with typos, and thinkos.
I was hoping to get into the semantic questions but I can't get
there until I get a good solid baseline patch to work with.
I have been able to hoist the permission check out of sget_fc for
converted filesystems. So progress is being made. That absolutely
requires fc->user_ns to be set properly before vfs_get_tree. Something
that still needs to be fixed.
I have also observed that by not allowing unconverted filesystems
to mount using the new api. The compatbitility code can be
significantly simplified, and the who data_size problem goes away.
I am going to be travelling for the next couple of days so I
don't expect I will be able to answer questions in a timely manner.
In the hopes that it might help below is my work in progress git
tree where I have cleaned up some of these issues.
https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git new-mount-api-testing
Eric
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-04 18:34 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20181004094637.GG22173@dhcp22.suse.cz>
On Thu, 4 Oct 2018, Michal Hocko wrote:
> > And prior to the offending commit, there were three ways to control thp
> > but two ways to determine if a mapping was eligible for thp based on the
> > implementation detail of one of those ways.
>
> Yes, it is really unfortunate that we have ever allowed to leak such an
> internal stuff like VMA flags to userspace.
>
Right, I don't like userspace dependencies on VmFlags in smaps myself, but
it's the only way we have available that shows whether a single mapping is
eligible to be backed by thp :/
> > If there are three ways to
> > control thp, userspace is still in the dark wrt which takes precedence
> > over the other: we have PR_SET_THP_DISABLE but globally sysfs has it set
> > to "always", or we have MADV_HUGEPAGE set per smaps but PR_SET_THP_DISABLE
> > shown in /proc/pid/status, etc.
> >
> > Which one is the ultimate authority?
>
> Isn't our documentation good enough? If not then we should document it
> properly.
>
No, because the offending commit actually changed the precedence itself:
PR_SET_THP_DISABLE used to be honored for future mappings and the commit
changed that for all current mappings. So as a result of the commit
itself we would have had to change the documentation and userspace can't
be expected to keep up with yet a fourth variable: kernel version. It
really needs to be simpler, just a per-mapping specifier.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Jann Horn @ 2018-10-04 18:26 UTC (permalink / raw)
To: cyphar
Cc: Eric W. Biederman, jlayton, Bruce Fields, Al Viro, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, Linux API
In-Reply-To: <20181004162611.vdlujbdguvagalpt@ryuk>
On Thu, Oct 4, 2018 at 6:26 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > Something else concurrently moves /A/B/C to /A/C. This can result in
> > the following:
> >
> > 1. You start the path walk and reach /A/B/C.
> > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > 3. Your path walk follows the first ".." up into /A. This is outside
> > the process root, but you never actually encountered the process root,
> > so you don't notice.
> > 4. Your path walk follows the second ".." up to /. Again, this is
> > outside the process root, but you don't notice.
> > 5. Your path walk walks down to /etc/passwd, and the open completes
> > successfully. You now have an fd pointing outside your chroot.
>
> I've been playing with this and I have the following patch, which
> according to my testing protects against attacks where ".." skips over
> nd->root. It abuses __d_path to figure out if nd->path can be resolved
> from nd->root (obviously a proper version of this patch would refactor
> __d_path so it could be used like this -- and would not return
> -EMULTIHOP).
>
> I've also attached my reproducer. With it, I was seeing fairly constant
> breakouts before this patch and after it I didn't see a single breakout
> after running it overnight. Obviously this is not conclusive, but I'm
> hoping that it can show what my idea for protecting against ".." was.
>
> Does this patch make sense? Or is there something wrong with it that I'm
> not seeing?
>
> --8<-------------------------------------------------------------------
>
> There is a fairly easy-to-exploit race condition with chroot(2) (and
> thus by extension AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a
> path can be used to "skip over" nd->root and thus escape to the
> filesystem above nd->root.
>
> thread1 [attacker]:
> for (;;)
> renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
> thread2 [victim]:
> for (;;)
> openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
>
> With fairly significant regularity, thread2 will resolve to
> "/etc/shadow" rather than "/a/b/etc/shadow". With this patch, such cases
> will be detected during ".." resolution (which is the weak point of
> chroot(2) -- since walking *into* a subdirectory tautologically cannot
> result in you walking *outside* nd->root).
>
> The use of __d_path here might seem suspect, however we don't mind if a
> path is moved from within the chroot to outside the chroot and we
> incorrectly decide it is safe (because at that point we are still within
> the set of files which were accessible at the beginning of resolution).
> However, we can fail resolution on the next path component if it remains
> outside of the root. A path which has always been outside nd->root
> during resolution will never be resolveable from nd->root and thus will
> always be blocked.
>
> DO NOT MERGE: Currently this code returns -EMULTIHOP in this case,
> purely as a debugging measure (so that you can see that
> the protection actually does something). Obviously in the
> proper patch this will return -EXDEV.
>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> fs/namei.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 6f995e6de6b1..c8349693d47b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
> * The new code replaces the old recursive symlink resolution with
> * an iterative one (in case of non-nested symlink chains). It does
> * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all
> * the special cases of the former code.
> *
> * With the new dcache, the pathname is stored at each inode, at least as
> @@ -1375,6 +1375,20 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> return -EXDEV;
> break;
> }
> + if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
> + char *pathbuf, *pathptr;
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> + if (!pathbuf)
> + return -ECHILD;
> + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> + kfree(pathbuf);
> + if (IS_ERR_OR_NULL(pathptr)) {
> + if (!pathptr)
> + pathptr = ERR_PTR(-EMULTIHOP);
> + return PTR_ERR(pathptr);
> + }
> + }
One somewhat problematic thing about this approach is that if someone
tries to lookup
"a/a/a/a/a/a/a/a/a/a/[...]/../../../../../../../../../.." for some
reason, you'll have quadratic runtime: For each "..", you'll have to
walk up to the root.
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Christian Brauner @ 2018-10-04 17:31 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jann Horn, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <20181004162611.vdlujbdguvagalpt@ryuk>
On Fri, Oct 05, 2018 at 02:26:11AM +1000, Aleksa Sarai wrote:
> On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> > You attempt to open "C/../../etc/passwd" under the root "/A/B".
> > Something else concurrently moves /A/B/C to /A/C. This can result in
> > the following:
> >
> > 1. You start the path walk and reach /A/B/C.
> > 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> > 3. Your path walk follows the first ".." up into /A. This is outside
> > the process root, but you never actually encountered the process root,
> > so you don't notice.
> > 4. Your path walk follows the second ".." up to /. Again, this is
> > outside the process root, but you don't notice.
> > 5. Your path walk walks down to /etc/passwd, and the open completes
> > successfully. You now have an fd pointing outside your chroot.
>
> I've been playing with this and I have the following patch, which
> according to my testing protects against attacks where ".." skips over
> nd->root. It abuses __d_path to figure out if nd->path can be resolved
> from nd->root (obviously a proper version of this patch would refactor
> __d_path so it could be used like this -- and would not return
> -EMULTIHOP).
>
> I've also attached my reproducer. With it, I was seeing fairly constant
> breakouts before this patch and after it I didn't see a single breakout
> after running it overnight. Obviously this is not conclusive, but I'm
> hoping that it can show what my idea for protecting against ".." was.
>
> Does this patch make sense? Or is there something wrong with it that I'm
> not seeing?
Interesting.
Apart from the abuse of __d_path() :) the question I'd have is whether
this just minimizes the race window or if you can provide a sound
argument that this actually can't happen anymore with this patch.
>
> --8<-------------------------------------------------------------------
>
> There is a fairly easy-to-exploit race condition with chroot(2) (and
> thus by extension AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a
> path can be used to "skip over" nd->root and thus escape to the
> filesystem above nd->root.
>
> thread1 [attacker]:
> for (;;)
> renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
> thread2 [victim]:
> for (;;)
> openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
>
> With fairly significant regularity, thread2 will resolve to
> "/etc/shadow" rather than "/a/b/etc/shadow". With this patch, such cases
> will be detected during ".." resolution (which is the weak point of
> chroot(2) -- since walking *into* a subdirectory tautologically cannot
> result in you walking *outside* nd->root).
>
> The use of __d_path here might seem suspect, however we don't mind if a
> path is moved from within the chroot to outside the chroot and we
> incorrectly decide it is safe (because at that point we are still within
> the set of files which were accessible at the beginning of resolution).
> However, we can fail resolution on the next path component if it remains
> outside of the root. A path which has always been outside nd->root
> during resolution will never be resolveable from nd->root and thus will
> always be blocked.
>
> DO NOT MERGE: Currently this code returns -EMULTIHOP in this case,
> purely as a debugging measure (so that you can see that
> the protection actually does something). Obviously in the
> proper patch this will return -EXDEV.
>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> fs/namei.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 6f995e6de6b1..c8349693d47b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
> * The new code replaces the old recursive symlink resolution with
> * an iterative one (in case of non-nested symlink chains). It does
> * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all
> * the special cases of the former code.
> *
> * With the new dcache, the pathname is stored at each inode, at least as
> @@ -1375,6 +1375,20 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> return -EXDEV;
> break;
> }
> + if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
> + char *pathbuf, *pathptr;
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
> + if (!pathbuf)
> + return -ECHILD;
> + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> + kfree(pathbuf);
> + if (IS_ERR_OR_NULL(pathptr)) {
> + if (!pathptr)
> + pathptr = ERR_PTR(-EMULTIHOP);
> + return PTR_ERR(pathptr);
> + }
> + }
> if (nd->path.dentry != nd->path.mnt->mnt_root) {
> struct dentry *old = nd->path.dentry;
> struct dentry *parent = old->d_parent;
> @@ -1510,6 +1524,20 @@ static int follow_dotdot(struct nameidata *nd)
> return -EXDEV;
> break;
> }
> + if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
> + char *pathbuf, *pathptr;
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> + if (!pathbuf)
> + return -ENOMEM;
> + pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
> + kfree(pathbuf);
> + if (IS_ERR_OR_NULL(pathptr)) {
> + if (!pathptr)
> + pathptr = ERR_PTR(-EMULTIHOP);
> + return PTR_ERR(pathptr);
> + }
> + }
> if (nd->path.dentry != nd->path.mnt->mnt_root) {
> int ret = path_parent_directory(&nd->path);
> if (ret)
> --
> 2.19.0
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Christian Brauner @ 2018-10-04 17:27 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jann Horn, Eric W. Biederman, Al Viro, jlayton, Bruce Fields,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <20181001161833.sg5iy6gk7n7crcvy@ryuk>
On Tue, Oct 02, 2018 at 02:18:33AM +1000, Aleksa Sarai wrote:
> On 2018-10-01, Jann Horn <jannh@google.com> wrote:
> > > If this is an issue for AT_THIS_ROOT, I believe this might also be an
> > > issue for AT_BENEATH since they are effectively both using the same
> > > nd->root trick (so you could similarly trick AT_BENEATH to not error
> > > out). So we'd need to figure out how to solve this problem in order for
> > > AT_BENEATH to be safe.
> >
> > Oh, wait, what? I think I didn't notice that the semantics of
> > AT_BENEATH changed like that since the original posting of David
> > Drysdale's O_BENEATH_ONLY patch
> > (https://lore.kernel.org/lkml/1439458366-8223-2-git-send-email-drysdale@google.com/).
> > David's patch had nice, straightforward semantics, blocking any form
> > of upwards traversal. Why was that changed? Does anyone actually want
> > to use paths that contain ".." with AT_BENEATH? I would strongly
> > prefer something that blocks any use of "..".
> >
> > @Al: It looks like this already changed back when you posted
> > https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk/
> > ?
>
> Yes, I copied the semantics from Al's patchset. I don't know why he felt
> strongly that this was the best idea, but in my opinion allowing paths
> like "a/../b/../c" seems like it's quite useful because otherwise you
> wouldn't be able to operate on most distribution root filesystems (many
> have symlinks that have ".." components). Looking at my own (openSUSE)
> machine there are something like 100k such symlinks (~37% of symlinks on
> my machine).
>
> While I do understand that the easiest way of solving this problem is to
> disallow ".." entirely with AT_BENEATH, given that support ".." has
> utility, I would like to know whether it's actually not possible to have
> this work safely.
>
> > > Speaking naively, doesn't it make sense to invalidate the walk if a path
> > > component was modified? Or is this something that would be far too
> > > costly with little benefit? What if we do more aggressive nd->root
> > > checks when resolving with AT_BENEATH or AT_THIS_ROOT (or if nd->root !=
> > > current->mnt_ns->root)?
> >
> > It seems to me like doing that would basically require looking at each
> > node in the path walk twice? And it'd be difficult to guarantee
> > forward progress unless you're willing to do some fairly heavy
> > locking.
>
> I had another idea since I wrote my previous mail -- since the issue (at
> least the way I understand it) is that ".." can "skip" over nd->root
> because of the rename, what if we had some sort of is_descendant() check
> within follow_dotdot()? (FWIW, ".." already has some pretty interesting
> "hand-over-hand" locking semantics.) This should be effectively similar
> to how prepend_path() deals with a path that is not reachable from @root
> (I'm not sure if the locking is acceptable for the namei path though).
>
> Since ".." with AT_THIS_ROOT (or AT_BENEATH) is not going to be the most
> common component type (and we only need to do these checks for those
> flags), I would think that the extra cost would not be _that_ awful.
>
> Would this work?
>
> > > You're right about this -- for C runtimes. In Go we cannot do a raw
> > > clone() or fork() (if you do it manually with RawSyscall you'll end with
> > > broken runtime state). So you're forced to do fork+exec (which then
> > > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> > > for CLONE_VFORK.
> >
> > If you insist on implementing every last bit of your code in Go, I guess.
>
> Fair enough, though I believe this would affect most multi-threaded
> programs as well (regardless of the language they're written in).
(Depends on whether you do any explicit locking and have atfork handlers
for your locks and so on. If you do a clone syscall directly to avoid
having libc running any additional atfork handlers (flushing streams
etc.) it's doable though not ideal.)
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-04 16:26 UTC (permalink / raw)
To: Jann Horn
Cc: Eric W. Biederman, jlayton, Bruce Fields, Al Viro, Arnd Bergmann,
shuah, David Howells, Andy Lutomirski, christian, Tycho Andersen,
kernel list, linux-fsdevel, linux-arch, linux-kselftest, dev,
containers, Linux API
In-Reply-To: <CAG48ez30WJhbsro2HOc_DR7V91M+hNFzBP5ogRMZaxbAORvqzg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 5425 bytes --]
On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> You attempt to open "C/../../etc/passwd" under the root "/A/B".
> Something else concurrently moves /A/B/C to /A/C. This can result in
> the following:
>
> 1. You start the path walk and reach /A/B/C.
> 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> 3. Your path walk follows the first ".." up into /A. This is outside
> the process root, but you never actually encountered the process root,
> so you don't notice.
> 4. Your path walk follows the second ".." up to /. Again, this is
> outside the process root, but you don't notice.
> 5. Your path walk walks down to /etc/passwd, and the open completes
> successfully. You now have an fd pointing outside your chroot.
I've been playing with this and I have the following patch, which
according to my testing protects against attacks where ".." skips over
nd->root. It abuses __d_path to figure out if nd->path can be resolved
from nd->root (obviously a proper version of this patch would refactor
__d_path so it could be used like this -- and would not return
-EMULTIHOP).
I've also attached my reproducer. With it, I was seeing fairly constant
breakouts before this patch and after it I didn't see a single breakout
after running it overnight. Obviously this is not conclusive, but I'm
hoping that it can show what my idea for protecting against ".." was.
Does this patch make sense? Or is there something wrong with it that I'm
not seeing?
--8<-------------------------------------------------------------------
There is a fairly easy-to-exploit race condition with chroot(2) (and
thus by extension AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a
path can be used to "skip over" nd->root and thus escape to the
filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". With this patch, such cases
will be detected during ".." resolution (which is the weak point of
chroot(2) -- since walking *into* a subdirectory tautologically cannot
result in you walking *outside* nd->root).
The use of __d_path here might seem suspect, however we don't mind if a
path is moved from within the chroot to outside the chroot and we
incorrectly decide it is safe (because at that point we are still within
the set of files which were accessible at the beginning of resolution).
However, we can fail resolution on the next path component if it remains
outside of the root. A path which has always been outside nd->root
during resolution will never be resolveable from nd->root and thus will
always be blocked.
DO NOT MERGE: Currently this code returns -EMULTIHOP in this case,
purely as a debugging measure (so that you can see that
the protection actually does something). Obviously in the
proper patch this will return -EXDEV.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 6f995e6de6b1..c8349693d47b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -53,8 +53,8 @@
* The new code replaces the old recursive symlink resolution with
* an iterative one (in case of non-nested symlink chains). It does
* this with calls to <fs>_follow_link().
- * As a side effect, dir_namei(), _namei() and follow_link() are now
- * replaced with a single function lookup_dentry() that can handle all
+ * As a side effect, dir_namei(), _namei() and follow_link() are now
+ * replaced with a single function lookup_dentry() that can handle all
* the special cases of the former code.
*
* With the new dcache, the pathname is stored at each inode, at least as
@@ -1375,6 +1375,20 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -EXDEV;
break;
}
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
+ char *pathbuf, *pathptr;
+
+ pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
+ if (!pathbuf)
+ return -ECHILD;
+ pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
+ kfree(pathbuf);
+ if (IS_ERR_OR_NULL(pathptr)) {
+ if (!pathptr)
+ pathptr = ERR_PTR(-EMULTIHOP);
+ return PTR_ERR(pathptr);
+ }
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1510,6 +1524,20 @@ static int follow_dotdot(struct nameidata *nd)
return -EXDEV;
break;
}
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
+ char *pathbuf, *pathptr;
+
+ pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (!pathbuf)
+ return -ENOMEM;
+ pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
+ kfree(pathbuf);
+ if (IS_ERR_OR_NULL(pathptr)) {
+ if (!pathptr)
+ pathptr = ERR_PTR(-EMULTIHOP);
+ return PTR_ERR(pathptr);
+ }
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
--
2.19.0
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #1.2: rename_attack.c --]
[-- Type: text/x-c, Size: 3172 bytes --]
// SPDX-License-Identifier: GPL-2.0+
/*
* Author: Aleksa Sarai <cyphar@cyphar.com>
* Copyright (C) 2018 SUSE LLC.
*/
#define _GNU_SOURCE
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <limits.h>
#include <sys/syscall.h>
#include <linux/fs.h>
/* These come from <uapi/asm-generic/linux/fcntl.h> */
#ifndef O_BENEATH
# define O_BENEATH 00040000000
# define O_XDEV 00100000000
# define O_NOPROCLINKS 00200000000
# define O_NOSYMLINKS 01000000000
# define O_THISROOT 02000000000
#endif
#ifndef AT_BENEATH
# define AT_BENEATH 0x8000
# define AT_XDEV 0x10000
# define AT_NO_PROCLINKS 0x20000
# define AT_NO_SYMLINKS 0x40000
# define AT_THIS_ROOT 0x80000
#endif
#define bail(...) \
do { \
fprintf(stderr, __VA_ARGS__); \
fputs("\n", stderr); \
exit(1); \
} while (0)
int renameat2(int olddirfd, const char *oldpath,
int newdirfd, const char *newpath, unsigned int flags)
{
errno = syscall(__NR_renameat2, olddirfd, oldpath,
newdirfd, newpath, flags);
return errno < 0 ? -1 : 0;
}
char *sprint_path(int fd)
{
char *fdpath = NULL, *fullpath;
if (asprintf(&fdpath, "/proc/self/fd/%d", fd) < 0)
return NULL;
fullpath = malloc(PATH_MAX+1);
if (!fullpath)
goto out1;
memset(fullpath, '\0', PATH_MAX+1);
if (readlink(fdpath, fullpath, PATH_MAX) < 0)
goto out2;
free(fdpath);
return fullpath;
out2:
free(fullpath);
out1:
free(fdpath);
return NULL;
}
int rename_attacker(int dirfd, char *namea, char *nameb)
{
for (;;) {
if (renameat2(dirfd, namea, dirfd, nameb, RENAME_EXCHANGE) < 0)
fprintf(stderr, "rename a<=>b failed: %m\n");
}
return 1;
}
int openat_victim(int dirfd, char *path, unsigned int flags)
{
char *last_path = NULL;
int last_errno = 0;
for (;;) {
int fd = openat(dirfd, path, flags);
if (fd < 0) {
if (errno != last_errno)
printf("errno=%m\n");
last_errno = errno;
} else {
char *path = sprint_path(fd);
if (!strcmp(path, "/")) {
puts("[[ BREAKOUT ]]");
printf("fd=%d\n", fd);
for (;;)
;
}
if (!last_path || strcmp(last_path, path)) {
printf("path=%s\n", path);
free(last_path);
last_path = strdup(path);
}
free(path);
close(fd);
}
}
free(last_path);
return 1;
}
int main(void)
{
char tmppath[] = "__TEST_rename_attack.XXXXXX";
if (!mkdtemp(tmppath))
bail("mkdtemp: %m");
int tmpfd = openat(AT_FDCWD, tmppath, O_PATH|O_DIRECTORY);
if (tmpfd < 0)
bail("open tmppath: %m");
// Make dira and dirb.
if (mkdirat(tmpfd, "a", 0755) < 0)
bail("mkdir dira: %m");
if (mkdirat(tmpfd, "b", 0755) < 0)
bail("mkdir dirb: %m");
if (mkdirat(tmpfd, "a/c", 0755) < 0)
bail("mkdir dira/c: %m");
// Open "a".
int afd = openat(tmpfd, "a", O_PATH|O_DIRECTORY);
if (afd < 0)
bail("open dira: %m");
pid_t child = fork();
if (child < 0)
bail("fork child: %m");
else if (!child)
return rename_attacker(tmpfd, "a/c", "b");
else
return openat_victim(afd, "c/../../../../../../../../../../../../../../../../../../../../../../../../../", O_THISROOT|O_RDONLY);
return 1;
}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [RFC PATCH v4 6/9] x86/cet/ibt: Add arch_prctl functions for IBT
From: Yu-cheng Yu @ 2018-10-04 16:25 UTC (permalink / raw)
To: Andy Lutomirski, Florian Weimer
Cc: Eugene Syromiatnikov, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg
In-Reply-To: <CALCETrXTqxQLWEHhSQ6WsDosnD61rnN2TgAFFomVAf5URP4DzA@mail.gmail.com>
On Thu, 2018-10-04 at 09:12 -0700, Andy Lutomirski wrote:
> On Thu, Oct 4, 2018 at 9:08 AM Florian Weimer <fw@deneb.enyo.de> wrote:
> >
> > * Yu-cheng Yu:
> >
> > > On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote:
> > > > On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote:
> > > > > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect
> > > > > Branch Tracking features.
> > > > >
> > > > > Introduce:
> > > > >
> > > > > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
> > > > > Enable the Indirect Branch Tracking legacy code bitmap.
> > > > >
> > > > > The parameter 'addr' is a pointer to a user buffer.
> > > > > On returning to the caller, the kernel fills the following:
> > > > >
> > > > > *addr = IBT bitmap base address
> > > > > *(addr + 1) = IBT bitmap size
> > > >
> > > > Again, some structure with a size field would be better from
> > > > UAPI/extensibility standpoint.
> > > >
> > > > One additional point: "size" in the structure from kernel should have
> > > > structure size expected by kernel, and at least providing there "0" from
> > > > user space shouldn't lead to failure (in fact, it is possible to provide
> > > > structure size back to userspace even if buffer is too small, along
> > > > with error).
> > >
> > > This has been in GLIBC v2.28. We cannot change it anymore.
> >
> > In theory, you could, if you change the ARCH_CET_LEGACY_BITMAP
> > constant, so that glibc will not use the different arch_prctl
> > operation. We could backport the change into the glibc 2.28 dynamic
> > linker, so that existing binaries will start using CET again. Then
> > only statically linked binaries will be impacted.
> >
> > It's definitely not ideal, but it's doable if the interface is
> > terminally broken or otherwise unacceptable. But to me it looks like
> > this threshold isn't reached here.
>
> I tend to agree.
>
> But I do think there's a real problem that should be fixed and won't
> affect ABI: the *name* of the prctl is pretty bad. I read the test
> several times trying to decide if you meant
> ARCH_GET_CET_LEGACY_BITMAP? But you don't.
>
> Maybe name it ARCH_CET_CREATE_LEGACY_BITMAP? And explicitly document
> what it does if legacy bitmap already exists?
I will fix it.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 6/9] x86/cet/ibt: Add arch_prctl functions for IBT
From: Andy Lutomirski @ 2018-10-04 16:12 UTC (permalink / raw)
To: Florian Weimer
Cc: Yu-cheng Yu, Eugene Syromiatnikov, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, linux-doc, Linux-MM,
linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav
In-Reply-To: <87murtn19o.fsf@mid.deneb.enyo.de>
On Thu, Oct 4, 2018 at 9:08 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Yu-cheng Yu:
>
> > On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote:
> >> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote:
> >> > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect
> >> > Branch Tracking features.
> >> >
> >> > Introduce:
> >> >
> >> > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
> >> > Enable the Indirect Branch Tracking legacy code bitmap.
> >> >
> >> > The parameter 'addr' is a pointer to a user buffer.
> >> > On returning to the caller, the kernel fills the following:
> >> >
> >> > *addr = IBT bitmap base address
> >> > *(addr + 1) = IBT bitmap size
> >>
> >> Again, some structure with a size field would be better from
> >> UAPI/extensibility standpoint.
> >>
> >> One additional point: "size" in the structure from kernel should have
> >> structure size expected by kernel, and at least providing there "0" from
> >> user space shouldn't lead to failure (in fact, it is possible to provide
> >> structure size back to userspace even if buffer is too small, along
> >> with error).
> >
> > This has been in GLIBC v2.28. We cannot change it anymore.
>
> In theory, you could, if you change the ARCH_CET_LEGACY_BITMAP
> constant, so that glibc will not use the different arch_prctl
> operation. We could backport the change into the glibc 2.28 dynamic
> linker, so that existing binaries will start using CET again. Then
> only statically linked binaries will be impacted.
>
> It's definitely not ideal, but it's doable if the interface is
> terminally broken or otherwise unacceptable. But to me it looks like
> this threshold isn't reached here.
I tend to agree.
But I do think there's a real problem that should be fixed and won't
affect ABI: the *name* of the prctl is pretty bad. I read the test
several times trying to decide if you meant
ARCH_GET_CET_LEGACY_BITMAP? But you don't.
Maybe name it ARCH_CET_CREATE_LEGACY_BITMAP? And explicitly document
what it does if legacy bitmap already exists?
--Andy
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Andy Lutomirski @ 2018-10-04 16:11 UTC (permalink / raw)
To: Yu-cheng Yu, Eugene Syromiatnikov
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
linux-doc, Linux-MM, linux-arch, Linux API, Arnd Bergmann,
Balbir Singh, Cyrill Gorcunov, Dave Hansen, Florian Weimer,
H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150553.21016-4-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 8:10 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Indirect branch tracking provides an optional legacy code bitmap
> that indicates locations of non-IBT compatible code. When set,
> each bit in the bitmap represents a page in the linear address is
> legacy code.
>
> We allocate the bitmap only when the application requests it.
> Most applications do not need the bitmap.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index 6adfe795d692..a65d9745af08 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> wrmsrl(MSR_IA32_U_CET, r);
> current->thread.cet.ibt_enabled = 0;
> }
> +
> +int cet_setup_ibt_bitmap(void)
> +{
> + u64 r;
> + unsigned long bitmap;
> + unsigned long size;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> + return -EOPNOTSUPP;
> +
> + if (!current->thread.cet.ibt_bitmap_addr) {
> + /*
> + * Calculate size and put in thread header.
> + * may_expand_vm() needs this information.
> + */
> + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
> + current->thread.cet.ibt_bitmap_size = size;
> + bitmap = do_mmap_locked(0, size, PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE,
> + VM_DONTDUMP);
> +
> + if (bitmap >= TASK_SIZE) {
> + current->thread.cet.ibt_bitmap_size = 0;
> + return -ENOMEM;
> + }
> +
> + current->thread.cet.ibt_bitmap_addr = bitmap;
> + }
> +
> + /*
> + * Lower bits of MSR_IA32_CET_LEG_IW_EN are for IBT
> + * settings. Clear lower bits even bitmap is already
> + * page-aligned.
> + */
> + bitmap = current->thread.cet.ibt_bitmap_addr;
> + bitmap &= PAGE_MASK;
> +
> + /*
> + * Turn on IBT legacy bitmap.
> + */
> + rdmsrl(MSR_IA32_U_CET, r);
> + r |= (MSR_IA32_CET_LEG_IW_EN | bitmap);
> + wrmsrl(MSR_IA32_U_CET, r);
> + return 0;
Why are you writing the MSRs in the case where the bitmap was already allocated?
^ permalink raw reply
* Re: [RFC PATCH v4 6/9] x86/cet/ibt: Add arch_prctl functions for IBT
From: Andy Lutomirski @ 2018-10-04 16:08 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Eugene Syromiatnikov, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg
In-Reply-To: <3350f7b42b32f3f7a1963a9c9c526210c24f7b05.camel@intel.com>
> On Oct 4, 2018, at 8:37 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
>> On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote:
>>> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote:
>>> Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect
>>> Branch Tracking features.
>>>
>>> Introduce:
>>>
>>> arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
>>> Enable the Indirect Branch Tracking legacy code bitmap.
>>>
>>> The parameter 'addr' is a pointer to a user buffer.
>>> On returning to the caller, the kernel fills the following:
>>>
>>> *addr = IBT bitmap base address
>>> *(addr + 1) = IBT bitmap size
>>
>> Again, some structure with a size field would be better from
>> UAPI/extensibility standpoint.
>>
>> One additional point: "size" in the structure from kernel should have
>> structure size expected by kernel, and at least providing there "0" from
>> user space shouldn't lead to failure (in fact, it is possible to provide
>> structure size back to userspace even if buffer is too small, along
>> with error).
>
> This has been in GLIBC v2.28. We cannot change it anymore.
Sure you can. Just change ARCH_CET_LEGACY_BITMAP to a new number. You
might need to change all the constants. And if the ELF note by itself
causes a problem too, you may need to rename it. And maybe ask glibc
to kindly not enable code that depends on non-upstreamed kernel
features.
There is not, and has never been, any ABI compatibility requirement
that says that, if glibc 2.28 "enables" a feature, that the kernel
will ever enable it in a way that makes glibc 2.28 actually support
it. All the kernel needs to do is avoid making glibc 2.28 *crash*.
^ permalink raw reply
* Re: [RFC PATCH v4 6/9] x86/cet/ibt: Add arch_prctl functions for IBT
From: Florian Weimer @ 2018-10-04 16:07 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Eugene Syromiatnikov, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Andy Lutomirski, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pa
In-Reply-To: <3350f7b42b32f3f7a1963a9c9c526210c24f7b05.camel@intel.com>
* Yu-cheng Yu:
> On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote:
>> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote:
>> > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect
>> > Branch Tracking features.
>> >
>> > Introduce:
>> >
>> > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
>> > Enable the Indirect Branch Tracking legacy code bitmap.
>> >
>> > The parameter 'addr' is a pointer to a user buffer.
>> > On returning to the caller, the kernel fills the following:
>> >
>> > *addr = IBT bitmap base address
>> > *(addr + 1) = IBT bitmap size
>>
>> Again, some structure with a size field would be better from
>> UAPI/extensibility standpoint.
>>
>> One additional point: "size" in the structure from kernel should have
>> structure size expected by kernel, and at least providing there "0" from
>> user space shouldn't lead to failure (in fact, it is possible to provide
>> structure size back to userspace even if buffer is too small, along
>> with error).
>
> This has been in GLIBC v2.28. We cannot change it anymore.
In theory, you could, if you change the ARCH_CET_LEGACY_BITMAP
constant, so that glibc will not use the different arch_prctl
operation. We could backport the change into the glibc 2.28 dynamic
linker, so that existing binaries will start using CET again. Then
only statically linked binaries will be impacted.
It's definitely not ideal, but it's doable if the interface is
terminally broken or otherwise unacceptable. But to me it looks like
this threshold isn't reached here.
^ permalink raw reply
* Re: [RFC PATCH v4 03/27] x86/fpu/xstate: Enable XSAVES system states
From: Yu-cheng Yu @ 2018-10-04 15:47 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181002171554.GE29601@zn.tnic>
On Tue, 2018-10-02 at 19:15 +0200, Borislav Petkov wrote:
> On Fri, Sep 21, 2018 at 08:03:27AM -0700, Yu-cheng Yu wrote:
> >
> > diff --git a/arch/x86/include/asm/fpu/xstate.h
> > b/arch/x86/include/asm/fpu/xstate.h
> > index 9b382e5157ed..a32dc5f8c963 100644
> > --- a/arch/x86/include/asm/fpu/xstate.h
> > +++ b/arch/x86/include/asm/fpu/xstate.h
> > @@ -19,10 +19,10 @@
> > #define XSAVE_YMM_SIZE 256
> > #define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
> >
> > -/* System features */
> > -#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)
>
> Previous patch renames it, this patch deletes it. Why do we need all
> that unnecessary churn?
>
> Also, this patch is trying to do a couple of things at once and
> reviewing it is not trivial. Please split the changes logically.
Yes, if we leave XFEATURE_MASK_SUPERVISOR unchanged in the previous patch, this
patch becomes much simpler. Perhaps we don't even need to split this one.
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index 19f8df54c72a..dd2c561c4544 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -51,13 +51,16 @@ static short xsave_cpuid_features[] __initdata = {
> > };
> >
> > /*
> > - * Mask of xstate features supported by the CPU and the kernel:
> > + * Mask of xstate features supported by the CPU and the kernel.
> > + * This is the result from CPUID query, SUPPORTED_XFEATURES_MASK,
> > + * and boot_cpu_has().
> > */
>
> This needs to explain what both masks are - user and system. "CPU" and
> "kernel" is not "user" and "all".
>
> > u64 xfeatures_mask_user __read_mostly;
> > +u64 xfeatures_mask_all __read_mostly;
The first one is all supported "user" states; the latter is "system" and "user"
states combined. I will put in comments.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 6/9] x86/cet/ibt: Add arch_prctl functions for IBT
From: Yu-cheng Yu @ 2018-10-04 15:37 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181004132811.GJ32759@asgard.redhat.com>
On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote:
> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote:
> > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect
> > Branch Tracking features.
> >
> > Introduce:
> >
> > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
> > Enable the Indirect Branch Tracking legacy code bitmap.
> >
> > The parameter 'addr' is a pointer to a user buffer.
> > On returning to the caller, the kernel fills the following:
> >
> > *addr = IBT bitmap base address
> > *(addr + 1) = IBT bitmap size
>
> Again, some structure with a size field would be better from
> UAPI/extensibility standpoint.
>
> One additional point: "size" in the structure from kernel should have
> structure size expected by kernel, and at least providing there "0" from
> user space shouldn't lead to failure (in fact, it is possible to provide
> structure size back to userspace even if buffer is too small, along
> with error).
This has been in GLIBC v2.28. We cannot change it anymore.
>
> >
> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > ---
> > arch/x86/include/uapi/asm/prctl.h | 1 +
> > arch/x86/kernel/cet_prctl.c | 38 ++++++++++++++++++++++++++++++-
> > arch/x86/kernel/process.c | 1 +
> > 3 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/uapi/asm/prctl.h
> > b/arch/x86/include/uapi/asm/prctl.h
> > index 3aec1088e01d..31d2465f9caf 100644
> > --- a/arch/x86/include/uapi/asm/prctl.h
> > +++ b/arch/x86/include/uapi/asm/prctl.h
> > @@ -18,5 +18,6 @@
> > #define ARCH_CET_DISABLE 0x3002
> > #define ARCH_CET_LOCK 0x3003
> > #define ARCH_CET_ALLOC_SHSTK 0x3004
> > +#define ARCH_CET_LEGACY_BITMAP 0x3005
>
> It would probably be nice to have mention of an architecture in these
> definitions ("ARCH_X86_CET_"...), but it's likely too late.
We can still change macro names. I will work on that.
>
> >
> > #endif /* _ASM_X86_PRCTL_H */
> > diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
> > index c4b7c19f5040..df47b5ebc3f4 100644
> > --- a/arch/x86/kernel/cet_prctl.c
> > +++ b/arch/x86/kernel/cet_prctl.c
> > @@ -20,6 +20,8 @@ static int handle_get_status(unsigned long arg2)
> >
> > if (current->thread.cet.shstk_enabled)
> > features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> > + if (current->thread.cet.ibt_enabled)
> > + features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
> >
> > shstk_base = current->thread.cet.shstk_base;
> > shstk_size = current->thread.cet.shstk_size;
> > @@ -49,9 +51,35 @@ static int handle_alloc_shstk(unsigned long arg2)
> > return 0;
> > }
> >
> > +static int handle_bitmap(unsigned long arg2)
> > +{
> > + unsigned long addr, size;
> > +
> > + if (current->thread.cet.ibt_enabled) {
> > + int err;
> > +
> > + err = cet_setup_ibt_bitmap();
> > + if (err)
> > + return err;
> > +
> > + addr = current->thread.cet.ibt_bitmap_addr;
> > + size = current->thread.cet.ibt_bitmap_size;
> > + } else {
> > + addr = 0;
> > + size = 0;
> > + }
> > +
> > + if (put_user(addr, (unsigned long __user *)arg2) ||
> > + put_user(size, (unsigned long __user *)arg2 + 1))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > int prctl_cet(int option, unsigned long arg2)
> > {
> > - if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> > + !cpu_feature_enabled(X86_FEATURE_IBT))
>
> This check is repeated many times, it is probably worth defining
> something like cpu_x86_cet_enabled() or something like that.
> Besides, early introduction of the macro would allow avoiding all these
> changes over the code in IBT patches, only macro definition has
> to be changed that way.
Yes, that makes things easier.
>
> > @@ -73,6 +103,12 @@ int prctl_cet(int option, unsigned long arg2)
> > case ARCH_CET_ALLOC_SHSTK:
> > return handle_alloc_shstk(arg2);
> >
> > + /*
> > + * Allocate legacy bitmap and return address & size to user.
> > + */
> > + case ARCH_CET_LEGACY_BITMAP:
> > + return handle_bitmap(arg2);
> > +
> > default:
> > return -EINVAL;
> > }
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index ac0ea9c7e89f..aea15a9b6a3e 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -797,6 +797,7 @@ long do_arch_prctl_common(struct task_struct *task, int
> > option,
> > case ARCH_CET_DISABLE:
> > case ARCH_CET_LOCK:
> > case ARCH_CET_ALLOC_SHSTK:
> > + case ARCH_CET_LEGACY_BITMAP:
> > return prctl_cet(option, cpuid_enabled);
> > }
>
> I wonder, whether this duplication is really needed for CET-related
> arch_prctl commands, why not just call them from do_arch_prctl_common?
I will fix it.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 6/9] x86/cet/ibt: Add arch_prctl functions for IBT
From: Eugene Syromiatnikov @ 2018-10-04 13:28 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150553.21016-7-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote:
> Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect
> Branch Tracking features.
>
> Introduce:
>
> arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
> Enable the Indirect Branch Tracking legacy code bitmap.
>
> The parameter 'addr' is a pointer to a user buffer.
> On returning to the caller, the kernel fills the following:
>
> *addr = IBT bitmap base address
> *(addr + 1) = IBT bitmap size
Again, some structure with a size field would be better from
UAPI/extensibility standpoint.
One additional point: "size" in the structure from kernel should have
structure size expected by kernel, and at least providing there "0" from
user space shouldn't lead to failure (in fact, it is possible to provide
structure size back to userspace even if buffer is too small, along
with error).
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/uapi/asm/prctl.h | 1 +
> arch/x86/kernel/cet_prctl.c | 38 ++++++++++++++++++++++++++++++-
> arch/x86/kernel/process.c | 1 +
> 3 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
> index 3aec1088e01d..31d2465f9caf 100644
> --- a/arch/x86/include/uapi/asm/prctl.h
> +++ b/arch/x86/include/uapi/asm/prctl.h
> @@ -18,5 +18,6 @@
> #define ARCH_CET_DISABLE 0x3002
> #define ARCH_CET_LOCK 0x3003
> #define ARCH_CET_ALLOC_SHSTK 0x3004
> +#define ARCH_CET_LEGACY_BITMAP 0x3005
It would probably be nice to have mention of an architecture in these
definitions ("ARCH_X86_CET_"...), but it's likely too late.
>
> #endif /* _ASM_X86_PRCTL_H */
> diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
> index c4b7c19f5040..df47b5ebc3f4 100644
> --- a/arch/x86/kernel/cet_prctl.c
> +++ b/arch/x86/kernel/cet_prctl.c
> @@ -20,6 +20,8 @@ static int handle_get_status(unsigned long arg2)
>
> if (current->thread.cet.shstk_enabled)
> features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> + if (current->thread.cet.ibt_enabled)
> + features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
>
> shstk_base = current->thread.cet.shstk_base;
> shstk_size = current->thread.cet.shstk_size;
> @@ -49,9 +51,35 @@ static int handle_alloc_shstk(unsigned long arg2)
> return 0;
> }
>
> +static int handle_bitmap(unsigned long arg2)
> +{
> + unsigned long addr, size;
> +
> + if (current->thread.cet.ibt_enabled) {
> + int err;
> +
> + err = cet_setup_ibt_bitmap();
> + if (err)
> + return err;
> +
> + addr = current->thread.cet.ibt_bitmap_addr;
> + size = current->thread.cet.ibt_bitmap_size;
> + } else {
> + addr = 0;
> + size = 0;
> + }
> +
> + if (put_user(addr, (unsigned long __user *)arg2) ||
> + put_user(size, (unsigned long __user *)arg2 + 1))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> int prctl_cet(int option, unsigned long arg2)
> {
> - if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> + !cpu_feature_enabled(X86_FEATURE_IBT))
This check is repeated many times, it is probably worth defining
something like cpu_x86_cet_enabled() or something like that.
Besides, early introduction of the macro would allow avoiding all these
changes over the code in IBT patches, only macro definition has
to be changed that way.
> @@ -73,6 +103,12 @@ int prctl_cet(int option, unsigned long arg2)
> case ARCH_CET_ALLOC_SHSTK:
> return handle_alloc_shstk(arg2);
>
> + /*
> + * Allocate legacy bitmap and return address & size to user.
> + */
> + case ARCH_CET_LEGACY_BITMAP:
> + return handle_bitmap(arg2);
> +
> default:
> return -EINVAL;
> }
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index ac0ea9c7e89f..aea15a9b6a3e 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -797,6 +797,7 @@ long do_arch_prctl_common(struct task_struct *task, int option,
> case ARCH_CET_DISABLE:
> case ARCH_CET_LOCK:
> case ARCH_CET_ALLOC_SHSTK:
> + case ARCH_CET_LEGACY_BITMAP:
> return prctl_cet(option, cpuid_enabled);
> }
I wonder, whether this duplication is really needed for CET-related
arch_prctl commands, why not just call them from do_arch_prctl_common?
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Johannes Thumshirn @ 2018-10-04 10:04 UTC (permalink / raw)
To: Jan Kara
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, linux-ext4
In-Reply-To: <20181003164407.GK24030-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Wed, Oct 03, 2018 at 06:44:07PM +0200, Jan Kara wrote:
> On Wed 03-10-18 08:13:37, Dan Williams wrote:
> > On Wed, Oct 3, 2018 at 8:07 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > > WRT per-inode DAX property, AFAIU that inode flag is just going to be
> > > advisory thing - i.e., use DAX if possible. If you mount a filesystem with
> > > these inode flags set in a configuration which does not allow DAX to be
> > > used, you will still be able to access such inodes but the access will use
> > > page cache instead. And querying these flags should better show real
> > > on-disk status and not just whether DAX is used as that would result in an
> > > even bigger mess. So this feature seems to be somewhat orthogonal to the
> > > API I'm looking for.
> >
> > True, I imagine once we have that flag we will be able to distinguish
> > the "saved" property and the "effective / live" property of DAX...
> > Also it's really not DAX that applications care about as much as "is
> > there page-cache indirection / overhead for this mapping?". That seems
> > to be a narrower guarantee that we can make than what "DAX" might
> > imply.
>
> Right. So what do people think about my suggestion earlier in the thread to
> use madvise(MADV_DIRECT_ACCESS) for this? Currently it would return success
> when DAX is in use, failure otherwise. Later we could extend it to be also
> used as a hint for caching policy for the inode...
Hmm apart from Dan's objection that it can't really be used for a
query, isn't madvise(2) for mmap(2)?
But AFAIU (from looking at the xfs code, so please correct me if I',
wrong), DAX can be used for the traditional read(2)/write(2) interface
as well.
There is at least:
xfs_file_read_iter()
`-> if (IS_DAX(inode))
`-> xfs_file_dax_read()
`->dax_iomap_rw()
So IMHO something on an inode granularity would make more sens to me.
Byte,
Johannes
--
Johannes Thumshirn Storage
jthumshirn-l3A5Bk7waGM@public.gmane.org +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-04 9:46 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810040209130.113459@chino.kir.corp.google.com>
On Thu 04-10-18 02:15:38, David Rientjes wrote:
> On Thu, 4 Oct 2018, Michal Hocko wrote:
>
> > > > > > So how about this? (not tested yet but it should be pretty
> > > > > > straightforward)
> > > > >
> > > > > Umm, prctl(PR_GET_THP_DISABLE)?
> > > >
> > > > /me confused. I thought you want to query for the flag on a
> > > > _different_ process.
> > >
> > > Why would we want to check three locations (system wide setting, prctl
> > > setting, madvise setting) to determine if a heap can be backed by thp?
> >
> > Because we simply have 3 different ways to control THP? Is this a real
> > problem?
> >
>
> And prior to the offending commit, there were three ways to control thp
> but two ways to determine if a mapping was eligible for thp based on the
> implementation detail of one of those ways.
Yes, it is really unfortunate that we have ever allowed to leak such an
internal stuff like VMA flags to userspace.
> If there are three ways to
> control thp, userspace is still in the dark wrt which takes precedence
> over the other: we have PR_SET_THP_DISABLE but globally sysfs has it set
> to "always", or we have MADV_HUGEPAGE set per smaps but PR_SET_THP_DISABLE
> shown in /proc/pid/status, etc.
>
> Which one is the ultimate authority?
Isn't our documentation good enough? If not then we should document it
properly.
> There's one way to specify it: in a
> single per-mapping location that reveals whether that mapping is eligible
> for thp or not. So I think it would be a very sane extension so that
> smaps reveals if a mapping can be backed by hugepages or not depending on
> the helper function thp uses itself to determine if it can fault
> hugepages. I don't think we should have three locations to check and then
> try to resolve which one takes precedence over the other for each
> userspace implementation (and perhaps how the kernel implementation
> evolves).
But we really have three different ways to disable thp. Which one has
caused the end result might be interesting/important because different
entities might be under control. You either have to contact your admin
for the global one, or whomever has launched you for the prctl thing. So
the distinction might be important.
Checking 3 different places and the precedence rules is not really
trivial but I do not see any reason why this couldn't be implemented in
a library so the user doesn't really have to scratch head.
If you really insist to have per-vma thing then all right but do not
conflate vma flags and the higher level logic and make it its own line
in the smaps output and make sure it reports only THP able VMAs.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-04 9:15 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20181004055842.GA22173@dhcp22.suse.cz>
On Thu, 4 Oct 2018, Michal Hocko wrote:
> > > > > So how about this? (not tested yet but it should be pretty
> > > > > straightforward)
> > > >
> > > > Umm, prctl(PR_GET_THP_DISABLE)?
> > >
> > > /me confused. I thought you want to query for the flag on a
> > > _different_ process.
> >
> > Why would we want to check three locations (system wide setting, prctl
> > setting, madvise setting) to determine if a heap can be backed by thp?
>
> Because we simply have 3 different ways to control THP? Is this a real
> problem?
>
And prior to the offending commit, there were three ways to control thp
but two ways to determine if a mapping was eligible for thp based on the
implementation detail of one of those ways. If there are three ways to
control thp, userspace is still in the dark wrt which takes precedence
over the other: we have PR_SET_THP_DISABLE but globally sysfs has it set
to "always", or we have MADV_HUGEPAGE set per smaps but PR_SET_THP_DISABLE
shown in /proc/pid/status, etc.
Which one is the ultimate authority? There's one way to specify it: in a
single per-mapping location that reveals whether that mapping is eligible
for thp or not. So I think it would be a very sane extension so that
smaps reveals if a mapping can be backed by hugepages or not depending on
the helper function thp uses itself to determine if it can fault
hugepages. I don't think we should have three locations to check and then
try to resolve which one takes precedence over the other for each
userspace implementation (and perhaps how the kernel implementation
evolves).
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-04 5:58 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810031547150.202532@chino.kir.corp.google.com>
On Wed 03-10-18 15:51:05, David Rientjes wrote:
> On Wed, 3 Oct 2018, Michal Hocko wrote:
>
> > > > So how about this? (not tested yet but it should be pretty
> > > > straightforward)
> > >
> > > Umm, prctl(PR_GET_THP_DISABLE)?
> >
> > /me confused. I thought you want to query for the flag on a
> > _different_ process.
>
> Why would we want to check three locations (system wide setting, prctl
> setting, madvise setting) to determine if a heap can be backed by thp?
Because we simply have 3 different ways to control THP? Is this a real
problem?
> If the nh flag being exported to VmFlag is to be extended beyond what my
> patch did, I suggest (1) it does it for the system wide setting as well
> and/or (2) calling a helper function to determine if the vma could be
> backed by thp in the first place regardless of any setting to determine if
> nh/hg is important.
>
> The last thing I suggest is done is adding a third place to check.
But conflating the three ways into a single exported symbol (be it nh
or something else) just makes the api more confusing longterm. I am
pretty sure we have made that mistake in the past already.
What if somebody really wants to check for PR_SET_THP_DISABLE? There is
currently no way to do that on a remote process right now AFAICS. So it
makes sense to export the state in general. Any exported API should be
about consistency. If you want to combine all three checks then
just do that in the userspace or in a library function.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH v4 21/27] x86/cet/shstk: ELF header parsing of Shadow Stack
From: Eugene Syromiatnikov @ 2018-10-03 23:27 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-22-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:45AM -0700, Yu-cheng Yu wrote:
> Look in .note.gnu.property of an ELF file and check if Shadow Stack needs
> to be enabled for the task.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/Kconfig | 4 +
> arch/x86/include/asm/elf.h | 5 +
> arch/x86/include/uapi/asm/elf_property.h | 15 +
> arch/x86/kernel/Makefile | 2 +
> arch/x86/kernel/elf.c | 340 +++++++++++++++++++++++
> fs/binfmt_elf.c | 15 +
> include/uapi/linux/elf.h | 1 +
> 7 files changed, 382 insertions(+)
> create mode 100644 arch/x86/include/uapi/asm/elf_property.h
> create mode 100644 arch/x86/kernel/elf.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 808aa3aecf3c..6377125543cc 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1919,12 +1919,16 @@ config X86_INTEL_CET
> config ARCH_HAS_SHSTK
> def_bool n
>
> +config ARCH_HAS_PROGRAM_PROPERTIES
> + def_bool n
> +
> config X86_INTEL_SHADOW_STACK_USER
> prompt "Intel Shadow Stack for user-mode"
> def_bool n
> depends on CPU_SUP_INTEL && X86_64
> select X86_INTEL_CET
> select ARCH_HAS_SHSTK
> + select ARCH_HAS_PROGRAM_PROPERTIES
> ---help---
> Shadow stack provides hardware protection against program stack
> corruption. Only when all the following are true will an application
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 0d157d2a1e2a..5b5f169c5c07 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -382,4 +382,9 @@ struct va_alignment {
>
> extern struct va_alignment va_align;
> extern unsigned long align_vdso_addr(unsigned long);
> +
> +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> +extern int arch_setup_features(void *ehdr, void *phdr, struct file *file,
> + bool interp);
> +#endif
> #endif /* _ASM_X86_ELF_H */
> diff --git a/arch/x86/include/uapi/asm/elf_property.h b/arch/x86/include/uapi/asm/elf_property.h
> new file mode 100644
> index 000000000000..af361207718c
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/elf_property.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _UAPI_ASM_X86_ELF_PROPERTY_H
> +#define _UAPI_ASM_X86_ELF_PROPERTY_H
> +
> +/*
> + * pr_type
> + */
> +#define GNU_PROPERTY_X86_FEATURE_1_AND (0xc0000002)
> +
> +/*
> + * Bits for GNU_PROPERTY_X86_FEATURE_1_AND
> + */
> +#define GNU_PROPERTY_X86_FEATURE_1_SHSTK (0x00000002)
Hm, these defeinitions aren't much different comparing to NT_*
definitions in include/uapi/linux/elf.h, is it expected that those
properties have to be parsed individually for each architecture?
> +
> +#endif /* _UAPI_ASM_X86_ELF_PROPERTY_H */
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index fbb2d91fb756..36b14ef410c8 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -141,6 +141,8 @@ obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>
> obj-$(CONFIG_X86_INTEL_CET) += cet.o
>
> +obj-$(CONFIG_ARCH_HAS_PROGRAM_PROPERTIES) += elf.o
Same thing here, enablement of program properties per se seems rather generic.
> diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> new file mode 100644
> index 000000000000..2fddd0bc545b
> --- /dev/null
> +++ b/arch/x86/kernel/elf.c
> @@ -0,0 +1,340 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Look at an ELF file's .note.gnu.property and determine if the file
> + * supports shadow stack and/or indirect branch tracking.
> + * The path from the ELF header to the note section is the following:
> + * elfhdr->elf_phdr->elf_note->property[].
> + */
> +
> +#include <asm/cet.h>
> +#include <asm/elf_property.h>
> +#include <asm/prctl.h>
> +#include <asm/processor.h>
> +#include <uapi/linux/elf-em.h>
> +#include <uapi/linux/prctl.h>
> +#include <linux/binfmts.h>
> +#include <linux/elf.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include <linux/string.h>
> +#include <linux/compat.h>
> +
> +/*
> + * The .note.gnu.property layout:
> + *
> + * struct elf_note {
> + * u32 n_namesz; --> sizeof(n_name[]); always (4)
> + * u32 n_ndescsz;--> sizeof(property[])
> + * u32 n_type; --> always NT_GNU_PROPERTY_TYPE_0
> + * };
> + * char n_name[4]; --> always 'GNU\0'
> + *
> + * struct {
> + * struct property_x86 {
> + * u32 pr_type;
> + * u32 pr_datasz;
> + * };
> + * u8 pr_data[pr_datasz];
> + * }[];
> + */
> +
> +#define BUF_SIZE (PAGE_SIZE / 4)
> +
> +struct property_x86 {
> + u32 pr_type;
> + u32 pr_datasz;
> +};
> +
> +typedef bool (test_fn)(void *buf, u32 *arg);
> +typedef void *(next_fn)(void *buf, u32 *arg);
> +
> +static inline bool test_note_type_0(void *buf, u32 *arg)
> +{
> + struct elf_note *n = buf;
> +
> + return ((n->n_namesz == 4) && (memcmp(n + 1, "GNU", 4) == 0) &&
> + (n->n_type == NT_GNU_PROPERTY_TYPE_0));
> +}
> +
> +static inline void *next_note(void *buf, u32 *arg)
> +{
> + struct elf_note *n = buf;
> + u32 align = *arg;
> + int size;
> +
> + size = round_up(sizeof(*n) + n->n_namesz, align);
> + size = round_up(size + n->n_descsz, align);
> +
> + if (buf + size < buf)
> + return NULL;
> + else
> + return (buf + size);
> +}
> +
> +static inline bool test_property_x86(void *buf, u32 *arg)
> +{
> + struct property_x86 *pr = buf;
> + u32 max_type = *arg;
> +
> + if (pr->pr_type > max_type)
> + *arg = pr->pr_type;
> +
> + return (pr->pr_type == GNU_PROPERTY_X86_FEATURE_1_AND);
> +}
> +
> +static inline void *next_property(void *buf, u32 *arg)
> +{
> + struct property_x86 *pr = buf;
> + u32 max_type = *arg;
> +
> + if ((buf + sizeof(*pr) + pr->pr_datasz < buf) ||
> + (pr->pr_type > GNU_PROPERTY_X86_FEATURE_1_AND) ||
> + (pr->pr_type > max_type))
> + return NULL;
> + else
> + return (buf + sizeof(*pr) + pr->pr_datasz);
> +}
> +
> +/*
> + * Scan 'buf' for a pattern; return true if found.
> + * *pos is the distance from the beginning of buf to where
> + * the searched item or the next item is located.
> + */
> +static int scan(u8 *buf, u32 buf_size, int item_size,
> + test_fn test, next_fn next, u32 *arg, u32 *pos)
> +{
> + int found = 0;
> + u8 *p, *max;
> +
> + max = buf + buf_size;
> + if (max < buf)
> + return 0;
> +
> + p = buf;
> +
> + while ((p + item_size < max) && (p + item_size > buf)) {
> + if (test(p, arg)) {
> + found = 1;
> + break;
> + }
> +
> + p = next(p, arg);
> + }
> +
> + *pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
> + return found;
> +}
> +
> +/*
> + * Search a NT_GNU_PROPERTY_TYPE_0 for GNU_PROPERTY_X86_FEATURE_1_AND.
> + */
> +static int find_feature_x86(struct file *file, unsigned long desc_size,
> + loff_t file_offset, u8 *buf, u32 *feature)
> +{
> + u32 buf_pos;
> + unsigned long read_size;
> + unsigned long done;
> + int found = 0;
> + int ret = 0;
> + u32 last_pr = 0;
> +
> + *feature = 0;
> + buf_pos = 0;
> +
> + for (done = 0; done < desc_size; done += buf_pos) {
> + read_size = desc_size - done;
> + if (read_size > BUF_SIZE)
> + read_size = BUF_SIZE;
> +
> + ret = kernel_read(file, buf, read_size, &file_offset);
> +
> + if (ret != read_size)
> + return (ret < 0) ? ret : -EIO;
> +
> + ret = 0;
> + found = scan(buf, read_size, sizeof(struct property_x86),
> + test_property_x86, next_property,
> + &last_pr, &buf_pos);
> +
> + if ((!buf_pos) || found)
> + break;
> +
> + file_offset += buf_pos - read_size;
> + }
> +
> + if (found) {
> + struct property_x86 *pr =
> + (struct property_x86 *)(buf + buf_pos);
> +
> + if (pr->pr_datasz == 4) {
> + u32 *max = (u32 *)(buf + read_size);
> + u32 *data = (u32 *)((u8 *)pr + sizeof(*pr));
> +
> + if (data + 1 <= max) {
> + *feature = *data;
> + } else {
> + file_offset += buf_pos - read_size;
> + file_offset += sizeof(*pr);
> + ret = kernel_read(file, feature, 4,
> + &file_offset);
> + }
> + }
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Search a PT_NOTE segment for the first NT_GNU_PROPERTY_TYPE_0.
> + */
> +static int find_note_type_0(struct file *file, unsigned long note_size,
> + loff_t file_offset, u32 align, u32 *feature)
> +{
> + u8 *buf;
> + u32 buf_pos;
> + unsigned long read_size;
> + unsigned long done;
> + int found = 0;
> + int ret = 0;
> +
> + buf = kmalloc(BUF_SIZE, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + *feature = 0;
> + buf_pos = 0;
> +
> + for (done = 0; done < note_size; done += buf_pos) {
> + read_size = note_size - done;
> + if (read_size > BUF_SIZE)
> + read_size = BUF_SIZE;
> +
> + ret = kernel_read(file, buf, read_size, &file_offset);
> +
> + if (ret != read_size) {
> + ret = (ret < 0) ? ret : -EIO;
> + kfree(buf);
> + return ret;
> + }
> +
> + /*
> + * item_size = sizeof(struct elf_note) + elf_note.n_namesz.
> + * n_namesz is 4 for the note type we look for.
> + */
> + ret = 0;
> + found += scan(buf, read_size, sizeof(struct elf_note) + 4,
> + test_note_type_0, next_note,
> + &align, &buf_pos);
> +
> + file_offset += buf_pos - read_size;
> +
> + if (found == 1) {
> + struct elf_note *n =
> + (struct elf_note *)(buf + buf_pos);
> + u32 start = round_up(sizeof(*n) + n->n_namesz, align);
> + u32 total = round_up(start + n->n_descsz, align);
> +
> + ret = find_feature_x86(file, n->n_descsz,
> + file_offset + start,
> + buf, feature);
> + file_offset += total;
> + buf_pos += total;
> + } else if (!buf_pos) {
> + *feature = 0;
> + break;
> + }
> + }
> +
> + kfree(buf);
> + return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static int check_notes_32(struct file *file, struct elf32_phdr *phdr,
> + int phnum, u32 *feature)
> +{
> + int i;
> + int err = 0;
> +
> + for (i = 0; i < phnum; i++, phdr++) {
> + if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 4))
> + continue;
> +
> + err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
> + phdr->p_align, feature);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_X86_64
> +static int check_notes_64(struct file *file, struct elf64_phdr *phdr,
> + int phnum, u32 *feature)
> +{
> + int i;
> + int err = 0;
> +
> + for (i = 0; i < phnum; i++, phdr++) {
> + if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 8))
> + continue;
> +
> + err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
> + phdr->p_align, feature);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +int arch_setup_features(void *ehdr_p, void *phdr_p,
> + struct file *file, bool interp)
> +{
> + int err = 0;
> + u32 feature = 0;
> +
> + struct elf64_hdr *ehdr64 = ehdr_p;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return 0;
> +
> + if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
> + struct elf64_phdr *phdr64 = phdr_p;
> +
> + err = check_notes_64(file, phdr64, ehdr64->e_phnum,
> + &feature);
> + if (err < 0)
> + goto out;
> + } else {
> +#ifdef CONFIG_COMPAT
> + struct elf32_hdr *ehdr32 = ehdr_p;
> +
> + if (ehdr32->e_ident[EI_CLASS] == ELFCLASS32) {
> + struct elf32_phdr *phdr32 = phdr_p;
> +
> + err = check_notes_32(file, phdr32, ehdr32->e_phnum,
> + &feature);
> + if (err < 0)
> + goto out;
> + }
> +#endif
> + }
> +
> + memset(¤t->thread.cet, 0, sizeof(struct cet_status));
> +
> + if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> + if (feature & GNU_PROPERTY_X86_FEATURE_1_SHSTK) {
> + err = cet_setup_shstk();
> + if (err < 0)
> + goto out;
> + }
> + }
> +
> +out:
> + return err;
> +}
There's a lot of similar code with bpf stackmap .build-id code (commit
v4.17-rc1~148^2~156^2~3^2~1), it might be worthy generalising some ELF
traversal routines, since there's general need of parsing ELF property
segments.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox