RE: [PATCH 3/5] cifsd: add file operations
> -Original Message-
> From: Namjae Jeon
> Sent: Sunday, March 21, 2021 10:14 PM
> To: linux-fsde...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> c...@vger.kernel.org
> Cc: linux-cifsd-de...@lists.sourceforge.net; smfre...@gmail.com;
> senozhat...@chromium.org; hyc@gmail.com; v...@zeniv.linux.org.uk;
> h...@lst.de; h...@infradead.org; ronniesahlb...@gmail.com;
> aurelien.ap...@gmail.com; aap...@suse.com; sand...@sandeen.net;
> dan.carpen...@oracle.com; colin.k...@canonical.com;
> rdun...@infradead.org; Namjae Jeon ;
> Sergey Senozhatsky ; Steve French
>
> Subject: [PATCH 3/5] cifsd: add file operations
>
> This adds file operations and buffer pool for cifsd.
>
> Signed-off-by: Namjae Jeon
> Signed-off-by: Sergey Senozhatsky
> Signed-off-by: Hyunchul Lee
> Acked-by: Ronnie Sahlberg
> Signed-off-by: Steve French
> ---
> fs/cifsd/buffer_pool.c | 292 ++
> fs/cifsd/buffer_pool.h | 28 +
> fs/cifsd/vfs.c | 1989
> fs/cifsd/vfs.h | 314 +++
> fs/cifsd/vfs_cache.c | 851 +
> fs/cifsd/vfs_cache.h | 213 +
> 6 files changed, 3687 insertions(+)
> create mode 100644 fs/cifsd/buffer_pool.c
> create mode 100644 fs/cifsd/buffer_pool.h
> create mode 100644 fs/cifsd/vfs.c
> create mode 100644 fs/cifsd/vfs.h
> create mode 100644 fs/cifsd/vfs_cache.c
> create mode 100644 fs/cifsd/vfs_cache.h
>
> diff --git a/fs/cifsd/buffer_pool.c b/fs/cifsd/buffer_pool.c
> new file mode 100644
> index ..864fea547c68
> --- /dev/null
> +++ b/fs/cifsd/buffer_pool.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "glob.h"
> +#include "buffer_pool.h"
> +#include "connection.h"
> +#include "mgmt/ksmbd_ida.h"
> +
> +static struct kmem_cache *filp_cache;
> +
> +struct wm {
> + struct list_headlist;
> + unsigned intsz;
> + charbuffer[0];
> +};
> +
> +struct wm_list {
> + struct list_headlist;
> + unsigned intsz;
> +
> + spinlock_t wm_lock;
> + int avail_wm;
> + struct list_headidle_wm;
> + wait_queue_head_t wm_wait;
> +};
> +
> +static LIST_HEAD(wm_lists);
> +static DEFINE_RWLOCK(wm_lists_lock);
> +
> +void *ksmbd_alloc(size_t size)
> +{
> + return kvmalloc(size, GFP_KERNEL | __GFP_ZERO);
> +}
> +
> +void ksmbd_free(void *ptr)
> +{
> + kvfree(ptr);
> +}
> +
> +static struct wm *wm_alloc(size_t sz, gfp_t flags)
> +{
> + struct wm *wm;
> + size_t alloc_sz = sz + sizeof(struct wm);
> +
> + wm = kvmalloc(alloc_sz, flags);
> + if (!wm)
> + return NULL;
> + wm->sz = sz;
> + return wm;
> +}
> +
> +static int register_wm_size_class(size_t sz)
> +{
> + struct wm_list *l, *nl;
> +
> + nl = kvmalloc(sizeof(struct wm_list), GFP_KERNEL);
> + if (!nl)
> + return -ENOMEM;
> +
> + nl->sz = sz;
> + spin_lock_init(&nl->wm_lock);
> + INIT_LIST_HEAD(&nl->idle_wm);
> + INIT_LIST_HEAD(&nl->list);
> + init_waitqueue_head(&nl->wm_wait);
> + nl->avail_wm = 0;
> +
> + write_lock(&wm_lists_lock);
> + list_for_each_entry(l, &wm_lists, list) {
> + if (l->sz == sz) {
> + write_unlock(&wm_lists_lock);
> + kvfree(nl);
> + return 0;
> + }
> + }
> +
> + list_add(&nl->list, &wm_lists);
> + write_unlock(&wm_lists_lock);
> + return 0;
> +}
> +
> +static struct wm_list *match_wm_list(size_t size)
> +{
> + struct wm_list *l, *rl = NULL;
> +
> + read_lock(&wm_lists_lock);
> + list_for_each_entry(l, &wm_lists, list) {
> + if (l->sz == size) {
> + rl = l;
> + break;
> + }
> + }
> + read_unlock(&wm_lists_lock);
> + return rl;
> +}
> +
> +static struct wm *find_wm(size_t size)
> +{
> + struct wm_list *wm_list;
> + struct wm *wm;
> +
> + wm_list = match_wm_list(size);
> + if (!wm_list) {
> + if (register_wm_size_class(size))
> + return NULL;
> + wm_list = match_wm_list(size);
> + }
> +
> + if (!wm_list)
> + return NULL;
> +
> + while (1) {
> + spin_lock(&wm_list->wm_lock);
> + if (!list_empty(&wm_list->idle_wm)) {
> + wm = list_entry(wm_list->idle_wm.next,
> + struct wm,
> + list);
> + list_del(&wm->list);
> + spin_unlock(&wm_list->wm_lock);
> + return wm;
> + }
> +
> + if (wm_list->avail_wm > num_online_cpus()) {
> + spin_unlock(
RE: [PATCH] security: fix the default value of secid_to_secctx hook
> -Original Message- > From: linux-kernel-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Arnd Bergmann > Sent: Saturday, May 16, 2020 1:05 AM > To: Alexei Starovoitov > Cc: James Morris ; Anders Roxell > ; Alexei Starovoitov ; Daniel > Borkmann ; LKML ; > Network Development ; bpf > > Subject: Re: [PATCH] security: fix the default value of secid_to_secctx hook I would *really* appreciate it if discussions about the LSM infrastructure where done on the linux-security-module mail list. (added to CC). > > On Sat, May 16, 2020 at 1:29 AM Alexei Starovoitov > wrote: > > > > On Thu, May 14, 2020 at 12:47 PM Alexei Starovoitov > > wrote: > > > > > > On Thu, May 14, 2020 at 12:43 PM James Morris > > > wrote: > > > > > > > > On Wed, 13 May 2020, Alexei Starovoitov wrote: > > > > > > > > > James, > > > > > > > > > > since you took the previous similar patch are you going to pick this > > > > > one up as well? > > > > > Or we can route it via bpf tree to Linus asap. > > > > > > > > Routing via your tree is fine. > > > > > > Perfect. > > > Applied to bpf tree. Thanks everyone. > > > > Looks like it was a wrong fix. > > It breaks audit like this: > > sudo auditctl -e 0 > > [ 88.400296] audit: error in audit_log_task_context > > [ 88.400976] audit: error in audit_log_task_context > > [ 88.401597] audit: type=1305 audit(1589584951.198:89): op=set > > audit_enabled=0 old=1 auid=0 ses=1 res=0 > > [ 88.402691] audit: type=1300 audit(1589584951.198:89): > > arch=c03e syscall=44 success=yes exit=52 a0=3 a1=7ffe42a37400 > > a2=34 a3=0 items=0 ppid=2250 pid=2251 auid=0 uid=0 gid=0 euid=0 suid=0 > > fsuid=0 egid=0 sgid=0 fsgid=0 tty=ttyS0 se) > > [ 88.405587] audit: type=1327 audit(1589584951.198:89): > > proctitle=617564697463746C002D650030 > > Error sending enable request (Operation not supported) > > > > when CONFIG_LSM= has "bpf" in it. > > Do you have more than one LSM enabled? It looks like > the problem with security_secid_to_secctx() is now that it > returns an error if any of the LSMs fail and the caller expects > it to succeed if at least one of them sets the secdata pointer. > > The problem earlier was that the call succeeded even though > no LSM had set the pointer. > > What is the behavior we actually expect from this function if > multiple LSM are loaded? > >Arnd
RE: [Patch v4 16/18] x86/speculation: Enable STIBP to protect security sensitive tasks
> -Original Message-
> From: Tim Chen [mailto:tim.c.c...@linux.intel.com]
> Sent: Tuesday, October 30, 2018 2:35 PM
> To: Schaufler, Casey ; Jiri Kosina
> ; Thomas Gleixner
> Cc: Tom Lendacky ; Ingo Molnar
> ; Peter Zijlstra ; Josh Poimboeuf
> ; Andrea Arcangeli ; David
> Woodhouse ; Andi Kleen ;
> Hansen, Dave ; Mallick, Asit K
> ; Arjan van de Ven ; Jon
> Masters ; Waiman Long ;
> linux-kernel@vger.kernel.org; x...@kernel.org
> Subject: Re: [Patch v4 16/18] x86/speculation: Enable STIBP to protect
> security
> sensitive tasks
>
> On 10/30/2018 02:07 PM, Schaufler, Casey wrote:
> >> -Original Message-
> >> From: Tim Chen [mailto:tim.c.c...@linux.intel.com]
> >> Sent: Tuesday, October 30, 2018 11:49 AM
> >> To: Jiri Kosina ; Thomas Gleixner
> >> Cc: Tim Chen ; Tom Lendacky
> >> ; Ingo Molnar ; Peter
> >> Zijlstra ; Josh Poimboeuf ;
> >> Andrea Arcangeli ; David Woodhouse
> >> ; Andi Kleen ; Hansen, Dave
> >> ; Schaufler, Casey ;
> >> Mallick, Asit K ; Arjan van de Ven
> >> ; Jon Masters ; Waiman Long
> >> ; linux-kernel@vger.kernel.org; x...@kernel.org
> >> Subject: [Patch v4 16/18] x86/speculation: Enable STIBP to protect security
> >> sensitive tasks
> >>
> >> Enable STIBP defense on high security tasks.
> >>
> >> For normal tasks, STIBP is unused so they are not impacted by overhead
> >> from STIBP in lite protection mode.
> >>
> >> Signed-off-by: Tim Chen
> >> ---
> >> arch/x86/kernel/cpu/bugs.c | 33 +
> >> 1 file changed, 33 insertions(+)
> >>
> >> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> >> index 54f4675..b402b96 100644
> >> --- a/arch/x86/kernel/cpu/bugs.c
> >> +++ b/arch/x86/kernel/cpu/bugs.c
> >> @@ -14,6 +14,8 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >> +#include
> >>
> >> #include
> >> #include
> >> @@ -770,6 +772,37 @@ static int ssb_prctl_set(struct task_struct *task,
> >> unsigned long ctrl)
> >>return 0;
> >> }
> >>
> >> +static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
> >> +{
> >> + bool update = false;
> >> +
> >> + if (!static_branch_unlikely(&spectre_v2_app_lite))
> >> + return;
> >> +
> >> + if (stibp_on)
> >> + update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
> >> + else
> >> + update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
> >> +
> >> + if (!update)
> >> + return;
> >> +
> >> + if (tsk == current)
> >> + speculation_ctrl_update_current();
> >> +}
> >> +
> >> +void arch_set_security(struct task_struct *tsk, unsigned int value)
> >
> > In this context "security" isn't descriptive. arch_set_stibp_defenses()
> > would be better.
>
> A more generic name decoupled from STIBP will be preferable. There
> can other kind of security defenses to be erected in
> the future.
>
> Perhaps arch_set_mitigation?
Better. On the other hand, adding function call layers just in case leads
to cascades of functions that do nothing but call other functions, and that
makes code hard to understand. I would leave generalization for the 2nd
person who wants to add mitigations.
>
> Thanks.
>
> Tim
>
> >
> > Since "value" should only ever have one of two values, and those
> > map directly to "true" or "false" this should be a bool, making the
> > code trivial:
> >
> > void arch_set_stibp_defenses(struct task_struct *task, bool stibp)
> > {
> > set_task_stibp(task, stibp);
> > }
> >
> > Or perhaps arch_set_security() should go away, and the calling
> > code would call set_task_stibp() directly. Unless there is some compelling
> > reason for the abstractions.
> >
> >> +{
> >> + if (value > SECURITY_HIGH)
> >> + return;
> >> +
> >> + /* Update STIBP defenses */
> >> + if (value == SECURITY_HIGH)
> >> + set_task_stibp(tsk, true);
> >> + else
> >> + set_task_stibp(tsk, false);
> >> +}
> >> +
> >> int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long
> >> which,
> >> unsigned long ctrl)
> >> {
> >> --
> >> 2.9.4
> >
RE: [Patch v4 13/18] security: Update security level of a process when modifying its dumpability
> -Original Message- > From: Tim Chen [mailto:tim.c.c...@linux.intel.com] > Sent: Tuesday, October 30, 2018 2:31 PM > To: Schaufler, Casey ; Jiri Kosina > ; Thomas Gleixner > Cc: Tom Lendacky ; Ingo Molnar > ; Peter Zijlstra ; Josh Poimboeuf > ; Andrea Arcangeli ; David > Woodhouse ; Andi Kleen ; > Hansen, Dave ; Mallick, Asit K > ; Arjan van de Ven ; Jon > Masters ; Waiman Long ; > linux-kernel@vger.kernel.org; x...@kernel.org; linux-security-module security-mod...@vger.kernel.org> > Subject: Re: [Patch v4 13/18] security: Update security level of a process > when > modifying its dumpability > > On 10/30/2018 01:57 PM, Schaufler, Casey wrote: > > > > > This isn't an LSM hook and hence does not belong in this file. > > arch_set_security() isn't descriptive, and is in fact a bad choice > > as task_struct has a field "security". This function has nothing > > to do with the task->security field, which is what I would expect > > based on the name. > > > > What file will be a logical place for this function? kernel/cpu.c ? You're working with CPU localized mitigations, right? You don't want it under security/ as that's all supposed to be bits of the LSM infrastructure. > >> + > >> +int update_process_security(struct task_struct *task) > > > > Again, this isn't an LSM hook and does not belong in this file. > > Also again, "security" isn't descriptive in the name. > > > > Thanks. > > Tim
RE: [Patch v4 16/18] x86/speculation: Enable STIBP to protect security sensitive tasks
> -Original Message-
> From: Tim Chen [mailto:tim.c.c...@linux.intel.com]
> Sent: Tuesday, October 30, 2018 11:49 AM
> To: Jiri Kosina ; Thomas Gleixner
> Cc: Tim Chen ; Tom Lendacky
> ; Ingo Molnar ; Peter
> Zijlstra ; Josh Poimboeuf ;
> Andrea Arcangeli ; David Woodhouse
> ; Andi Kleen ; Hansen, Dave
> ; Schaufler, Casey ;
> Mallick, Asit K ; Arjan van de Ven
> ; Jon Masters ; Waiman Long
> ; linux-kernel@vger.kernel.org; x...@kernel.org
> Subject: [Patch v4 16/18] x86/speculation: Enable STIBP to protect security
> sensitive tasks
>
> Enable STIBP defense on high security tasks.
>
> For normal tasks, STIBP is unused so they are not impacted by overhead
> from STIBP in lite protection mode.
>
> Signed-off-by: Tim Chen
> ---
> arch/x86/kernel/cpu/bugs.c | 33 +
> 1 file changed, 33 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 54f4675..b402b96 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -14,6 +14,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
> #include
> @@ -770,6 +772,37 @@ static int ssb_prctl_set(struct task_struct *task,
> unsigned long ctrl)
> return 0;
> }
>
> +static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
> +{
> + bool update = false;
> +
> + if (!static_branch_unlikely(&spectre_v2_app_lite))
> + return;
> +
> + if (stibp_on)
> + update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
> + else
> + update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
> +
> + if (!update)
> + return;
> +
> + if (tsk == current)
> + speculation_ctrl_update_current();
> +}
> +
> +void arch_set_security(struct task_struct *tsk, unsigned int value)
In this context "security" isn't descriptive. arch_set_stibp_defenses()
would be better.
Since "value" should only ever have one of two values, and those
map directly to "true" or "false" this should be a bool, making the
code trivial:
void arch_set_stibp_defenses(struct task_struct *task, bool stibp)
{
set_task_stibp(task, stibp);
}
Or perhaps arch_set_security() should go away, and the calling
code would call set_task_stibp() directly. Unless there is some compelling
reason for the abstractions.
> +{
> + if (value > SECURITY_HIGH)
> + return;
> +
> + /* Update STIBP defenses */
> + if (value == SECURITY_HIGH)
> + set_task_stibp(tsk, true);
> + else
> + set_task_stibp(tsk, false);
> +}
> +
> int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
>unsigned long ctrl)
> {
> --
> 2.9.4
RE: [Patch v4 13/18] security: Update security level of a process when modifying its dumpability
> -Original Message-
> From: Tim Chen [mailto:tim.c.c...@linux.intel.com]
> Sent: Tuesday, October 30, 2018 11:49 AM
> To: Jiri Kosina ; Thomas Gleixner
> Cc: Tim Chen ; Tom Lendacky
> ; Ingo Molnar ; Peter
> Zijlstra ; Josh Poimboeuf ;
> Andrea Arcangeli ; David Woodhouse
> ; Andi Kleen ; Hansen, Dave
> ; Schaufler, Casey ;
> Mallick, Asit K ; Arjan van de Ven
> ; Jon Masters ; Waiman Long
> ; linux-kernel@vger.kernel.org; x...@kernel.org
Added LSM mail list to the CC:
> Subject: [Patch v4 13/18] security: Update security level of a process when
> modifying its dumpability
"Level" isn't a good description of security characteristics
as it has been overloaded in too many ways already. More
on that later.
> When a process is made non-dumpable, the action implies a higher level
> of security implicitly as its memory is imposed with access restriction.
How is this "higher level" manifest?
> A call to update_process_security() is added to update security defenses
> according to a process's dumpability and its implied security level.
Can you describe the set of security attributes that are implied by the
process' dumpability? I would think that the dumpability would be an
artifact of these attributes, not the other way around.
> Architecture specific defenses is erected for threads in the process
nit: s/is/are/
> by calling arch_set_security(task, SECURITY_LEVEL_HIGH) or the defenses
> relaxed via arch_set_security(task, SECURITY_LEVEL_NORMAL). Such defenses
> may incur extra overhead and is reserved for tasks needing high security.
nit: s/is/are/
> Signed-off-by: Tim Chen
> ---
> fs/exec.c| 2 ++
> include/linux/security.h | 6 ++
> kernel/cred.c| 5 -
> kernel/sys.c | 1 +
> security/security.c | 31 +++
> 5 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 1ebf6e5..e70c8a7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1366,6 +1366,8 @@ void setup_new_exec(struct linux_binprm * bprm)
> else
> set_dumpable(current->mm, SUID_DUMP_USER);
>
> + update_process_security(current);
> +
> arch_setup_new_exec();
> perf_event_exec();
> __set_task_comm(current, kbasename(bprm->filename), true);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 75f4156..469d05f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -61,6 +61,12 @@ struct mm_struct;
> /* LSM Agnostic defines for sb_set_mnt_opts */
> #define SECURITY_LSM_NATIVE_LABELS 1
>
> +/* Security level */
> +#define SECURITY_NORMAL 0
> +#define SECURITY_HIGH1
NAK: "NORMAL" and "HIGH" are meaningless in this context.
If you intend to differentiate between "dumpable" and "undumpable"
you could use those, although I would recommend something that
describes the reason the task is dumpable.
> +
> +extern int update_process_security(struct task_struct *task);
> +
> struct ctl_table;
> struct audit_krule;
> struct user_namespace;
> diff --git a/kernel/cred.c b/kernel/cred.c
> index ecf0365..0806a74 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
>
> #if 0
> #define kdebug(FMT, ...) \
> @@ -445,8 +446,10 @@ int commit_creds(struct cred *new)
> !uid_eq(old->fsuid, new->fsuid) ||
> !gid_eq(old->fsgid, new->fsgid) ||
> !cred_cap_issubset(old, new)) {
> - if (task->mm)
> + if (task->mm) {
> set_dumpable(task->mm, suid_dumpable);
> + update_process_security(task);
> + }
> task->pdeath_signal = 0;
> smp_wmb();
> }
> diff --git a/kernel/sys.c b/kernel/sys.c
> index cf5c675..c6f179a 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2293,6 +2293,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
> arg2, unsigned long, arg3,
> break;
> }
> set_dumpable(me->mm, arg2);
> + update_process_security(me);
> break;
>
> case PR_SET_UNALIGN:
> diff --git a/security/security.c b/security/security.c
> index 736e78d..12460f2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
>
> #include
> @@ -1353,6 +1355,35 @@ int security_ino
RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
> -Original Message- > From: James Morris [mailto:jmor...@namei.org] > Sent: Thursday, September 27, 2018 3:47 PM > To: Casey Schaufler > Cc: Schaufler, Casey ; kris...@linux.intel.com; > kernel-harden...@lists.openwall.com; Dock, Deneen T > ; linux-kernel@vger.kernel.org; Hansen, Dave > ; linux-security-mod...@vger.kernel.org; > seli...@tycho.nsa.gov; ar...@linux.intel.com > Subject: Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel > > On Thu, 27 Sep 2018, Casey Schaufler wrote: > > > On 9/27/2018 2:45 PM, James Morris wrote: > > > On Wed, 26 Sep 2018, Casey Schaufler wrote: > > > > > >> +/* > > >> + * Namespace checks. Considered safe if: > > >> + * cgroup namespace is the same > > >> + * User namespace is the same > > >> + * PID namespace is the same > > >> + */ > > >> +if (current->nsproxy) > > >> +ccgn = current->nsproxy->cgroup_ns; > > >> +if (p->nsproxy) > > >> +pcgn = p->nsproxy->cgroup_ns; > > >> +if (ccgn != pcgn) > > >> +return -EACCES; > > >> +if (current->cred->user_ns != p->cred->user_ns) > > >> +return -EACCES; > > >> +if (task_active_pid_ns(current) != task_active_pid_ns(p)) > > >> +return -EACCES; > > >> +return 0; > > > I really don't like the idea of hard-coding namespace security semantics > > > in an LSM. Also, I'm not sure if these semantics make any sense. > > > > Checks on namespaces where explicitly requested. > > By whom and what is the rationale? The rationale is to protect containers. Since those closest thing there is to a definition of containers is "uses namespaces" that becomes the focus. Separating them out does not make too much sense as I would expect someone concerned with one to be concerned with all.
RE: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
> -Original Message- > From: Stephen Smalley [mailto:s...@tycho.nsa.gov] > Sent: Thursday, September 27, 2018 8:50 AM > To: Schaufler, Casey ; kernel- > harden...@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security- > mod...@vger.kernel.org; seli...@tycho.nsa.gov; Hansen, Dave > ; Dock, Deneen T ; > kris...@linux.intel.com; ar...@linux.intel.com; Paul Moore moore.com> > Subject: Re: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED > > On 09/26/2018 04:34 PM, Casey Schaufler wrote: > > From: Casey Schaufler > > > > A ptrace access check with mode PTRACE_MODE_SCHED gets called > > from process switching code. This precludes the use of audit or avc, > > as the locking is incompatible. The only available check that > > can be made without using avc is a comparison of the secids. > > This is not very satisfactory as it will indicate possible > > vulnerabilies much too aggressively. > Can you document (in the patch description and/or in the inline > documentation in lsm_hooks.h) what locks can be safely used when this > hook is called with PTRACE_MODE_SCHED? rcu_read_lock() seemingly must > be safe since it is being called by task_sid() below. Are any other > locking primitives safe? Peter Zijlstra had this comment on locking in the SELinux ptrace path. avc_has_perm_noaudit() security_compute_av() read_lock(&state->ss->policy_rwlock); avc_insert() spin_lock_irqsave(); avc_denied() avc_update_node() spin_lock_irqsave(); under the scheduler's raw_spinlock_t, which are invalid lock nestings. I don't know that it would be impossible to address these issues, but as many people have noted over the years I am not now nor have ever been an expert on locking. > > Does the PTRACE_MODE_SCHED check have to occur while holding the > scheduler lock, or could it be performed before taking the lock? My understanding is that the lock is required. > Can you cite the commit or patch posting (e.g. from lore or patchwork) > that defines PTRACE_MODE_SCHED and its usage as part of the patch > description for context? Is this based on the v7 patchset, e.g. > https://lore.kernel.org/lkml/nycvar.yfh.7.76.1809251437340.15...@cbobk.fhf > r.pm/ Yes, that's the one. Sorry, I should have identified that. > > > > > Signed-off-by: Casey Schaufler > > --- > > security/selinux/hooks.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index ad9a9b8e9979..160239791007 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2267,6 +2267,8 @@ static int selinux_ptrace_access_check(struct > task_struct *child, > > u32 sid = current_sid(); > > u32 csid = task_sid(child); > > > > + if (mode & PTRACE_MODE_SCHED) > > + return sid == csid ? 0 : -EACCES; > IIUC, this logic is essentially the same as the uid-based check, > including the fact that even a "privileged" process is not given any > special handling since they always return false from ptrace_has_cap() > for PTRACE_MODE_SCHED. If they are ok with applying IBPB whenever uids > differ, then doing so whenever sids/contexts differ does not seem like > an onerous thing. > > > > if (mode & PTRACE_MODE_READ) > > return avc_has_perm(&selinux_state, > > sid, csid, SECCLASS_FILE, FILE__READ, > NULL); > >
RE: [PATCH v6 0/3] Harden spectrev2 userspace-userspace protection
> -Original Message-
> From: Jiri Kosina [mailto:ji...@kernel.org]
> Sent: Wednesday, September 12, 2018 2:05 AM
> To: Thomas Gleixner ; Ingo Molnar ;
> Peter Zijlstra ; Josh Poimboeuf
> ; Andrea Arcangeli ;
> Woodhouse, David ; Andi Kleen ;
> Tim Chen ; Schaufler, Casey
>
> Cc: linux-kernel@vger.kernel.org; x...@kernel.org
> Subject: [PATCH v6 0/3] Harden spectrev2 userspace-userspace protection
>
> Currently, linux kernel is basically not preventing userspace-userspace
> spectrev2 attack, because:
>
> - IBPB is basically unused (issued only for tasks that marked themselves
> explicitly non-dumpable, which is absolutely negligible minority of all
> software out there), therefore cross-process branch buffer posioning
> using spectrev2 is possible
>
> - STIBP is completely unused, therefore cross-process branch buffer
> poisoning using spectrev2 between processess running on two HT siblings
> thread s is possible
>
> This patchset changes IBPB semantics, so that it's now applied whenever
> context-switching between processess that can't use ptrace() to achieve
> the same. This admittedly comes with extra overhad on a context switch;
> systems that don't care about could disable the mitigation using
> nospectre_v2 boot option.
> The IBPB implementaion is heavily based on original patches by Tim Chen.
>
> In addition to that, we unconditionally turn STIBP on so that HT siblings
> always have separate branch buffers.
>
> We've been carrying IBPB implementation with the same semantics in our
> (SUSE) trees since january disclosure; STIBP was more or less ignored up
> to today.
>
> v1->v2:
> include IBPB changes
> v2->v3:
> fix IBPB 'who can trace who' semantics
> wire up STIBP flipping to SMT hotplug
> v3->v4:
> dropped ___ptrace_may_access(), as it's not needed
> fixed deadlock with LSM/audit/selinux (Andrea Arcangeli)
> statically patch out the ptrace check if !IBPB
>
> v4->v5:
> fix MSR writing logic (Thomas Gleixner, Josh Poimboeuf)
>
> v5->v6:
> propagate X86_FEATURE_RSB_CTXSW setting to sysfs
> propagate STIBP setting to sysfs (Thomas Gleixner)
> simplify arch_smt_update() (Thomas Gleixner)
>
> Jiri Kosina (3):
> x86/speculation: apply IBPB more strictly to avoid cross-process data
> leak
> x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation
> x86/speculation: Propagate information about RSB filling mitigation to
> sysfs
>
> arch/x86/kernel/cpu/bugs.c | 60
> ++--
> arch/x86/mm/tlb.c | 31 ---
> include/linux/ptrace.h | 4
> kernel/cpu.c | 11 ++-
> kernel/ptrace.c| 12
> 5 files changed, 96 insertions(+), 22 deletions(-)
>
> --
> Jiri Kosina
> SUSE Labs
The locking issue with SELinux has a simple fix as below.
The other LSMs don't manifest this issue. With the change to
SELinux the call to security_ptrace_access_check() can and
should be made unconditionally.
Patch is attached, whitespace damaged (known problem) patch:
SELinux: Handle audit locking for PTRACE_MODE_IBPB
The SELinux audit code locking cannot be used from the
task switching code, which is where PTRACE_MODE_IBPB comes
from. As this is a system check, not a user action, audit
is not needed, and would generate noise. Use the unaudited
check for this case.
Signed-off-by: Casey Schaufler
---
kernel/ptrace.c | 4 +---
security/selinux/hooks.c | 5 +
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 5c5e7cb597cd..202a4d9c2af7 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -330,9 +330,7 @@ int __ptrace_may_access(struct task_struct *task, unsigned
int mode)
!ptrace_has_cap(mm->user_ns, mode
return -EPERM;
- if (!(mode & PTRACE_MODE_NOACCESS_CHK))
- return security_ptrace_access_check(task, mode);
- return 0;
+ return security_ptrace_access_check(task, mode);
}
bool ptrace_may_access(struct task_struct *task, unsigned int mode)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 161a4f29f860..30d21142e9fe 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2215,7 +2215,12 @@ static int selinux_ptrace_access_check(struct
task_struct *child,
{
u32 sid = current_sid();
u32 csid = task_sid(child);
+ struct av_decision avd;
+ if (mode == PTRACE_MODE_IBPB)
+ return avc_has_perm_noaudit(&selinux_state, sid, csid,
+ SECCL
RE: [PATCH v6 1/3] x86/speculation: apply IBPB more strictly to avoid cross-process data leak
> -Original Message- > From: Jiri Kosina [mailto:ji...@kernel.org] > > > @@ -325,10 +326,13 @@ static int __ptrace_may_access(struct task_struct > *task, unsigned int mode) > mm = task->mm; > if (mm && > ((get_dumpable(mm) != SUID_DUMP_USER) && > - !ptrace_has_cap(mm->user_ns, mode))) > + ((mode & PTRACE_MODE_NOACCESS_CHK) || > +!ptrace_has_cap(mm->user_ns, mode > return -EPERM; > > - return security_ptrace_access_check(task, mode); > + if (!(mode & PTRACE_MODE_NOACCESS_CHK)) > + return security_ptrace_access_check(task, mode); > + return 0; Because PTRACE_MODE_IBPB includes PTRACE_MODE_NOAUDIT you shouldn't need this change. Do you have a good way to exercise this code path? I'm having trouble getting to the check, and have yet to get a case where PTRACE_MODE_NOACCESS_CHK is set. > } > > bool ptrace_may_access(struct task_struct *task, unsigned int mode) > > -- > Jiri Kosina > SUSE Labs
RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak
> -Original Message- > From: Thomas Gleixner [mailto:t...@linutronix.de] ... > > Casey, can you please spare us the completely redundant copy of the mail > header? Sorry to be a bother. > > Short of a patch to show the changes (which I wish I could do today, but > > really can't) what I want to see is: > > > > - Put ptrace back to using the security module interfaces. > > - Identify where this causes locking issues and work with the module > > owners (a reasonable lot, all) to provide lock safe paths for the IBPB > case. > > > > Otherwise, I have to add a new LSM hook right after your ptrace call and > > duplicate a whole lot of what you've just turned off, plus creating lock > > safe code that duplicates what ptrace already does. While I would rather > > have the side-channel checks be separate from the ptrace checks I can't > > justify doing both. > > I'm not yet convinced that making these decisions purely based on LSM is a > good idea. The current patch-under-discussion uses the ptrace access policy as the criteria for deciding that side-channel attacks are worrisome. This patch excludes the LSM checks that are usually called by the ptrace code. If it retained the LSM checks the need to do anything special in an LSM would be limited to things that the ptrace checks don't already do. > The LSM based decision can optionally replace the built in > default decision at runtime, LSM modules can add restrictions, but do not replace existing decisions. > but it cannot replace it because its simpler > for most people to install a new kernel than fiddling with LSM. Um, installing a new kernel is the usual way to fiddle with LSM. It's done by configuration options, so I don't see the distinction you're making. > We want a halfways sane default solution for this ASAP and Jiri's patch is > straight forward providing one without preventing you from adding your > magic LSM stuff once you have time to work on it including all (locking) > problems it creates. There's actually one locking issue in the SELinux ptrace hook, and that's due to auditing. It's easy to fix. I did it in the side channel LSM that I proposed. There's nothing magic about the "LSM stuff", especially compared to what goes on in ptrace. > Nice try to offload that to Jiri. Oh, come off it. If ptrace is the right way to do this check, that's great. What I'm saying is that rather than turning off the LSM checks in ptrace because it will take some work to get it right for the environment it gets called from the right thing to do is to fix the LSM code. How about this? Take Jiri's patch as written. You get everything except checks on the security blobs and any "magic" that my safesidechannel module did. I will propose a follow on patch that fixes the SELinux code to eliminate the locking issue and enables the LSM hooks in the IBPB case. I can then do a revised "magic" safesidechannel security module that uses the ptrace hook instead of adding a new hook explicitly for IBPB. There is some danger that in the future ptrace and IBPB criteria will diverge sufficiently that a common hook becomes nonsensical. As no one else seems concerned about this possibility, I won't lose any sleep over it either. > Thanks, > > tglx
RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak
> -Original Message- > From: Jiri Kosina [mailto:ji...@kernel.org] > Sent: Monday, September 10, 2018 1:42 PM > To: Schaufler, Casey > Cc: Thomas Gleixner ; Ingo Molnar ; > Peter Zijlstra ; Josh Poimboeuf > ; Andrea Arcangeli ; > Woodhouse, David ; Andi Kleen ; > Tim Chen ; linux-kernel@vger.kernel.org; > x...@kernel.org > Subject: RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid > cross-process data leak > > On Mon, 10 Sep 2018, Schaufler, Casey wrote: > > > It you're going to call __ptrace_access_check(), > > I guess you mean __ptrace_may_access() here. Sorry, yes. Too much code, too little brain. > > which already includes an LSM hook, it makes a whole lot of sense to > > make that the path for doing any module specific checks. It seems wrong > > to disable the LSM hook there, then turn around and introduce a new one > > that does the check you just disabled. The patches I had proposed > > created a new LSM hook because there was not path to an existing hook. > > With your addition of __ptrace_access_check() that is no longer an issue > > once any locking problems are resolved. Rather than use a new hook, the > > existing ptrace hooks ought to work just fine, and any new checks can be > > added in a new module that has its own ptrace_access_check hook. > > Sorry for being dense, but what exactly are you proposing here then? Sorry for not being clear. You added a call to __ptrace_may_access(). The security modules that get called from __ptrace_may_access() via security_ptrace_access_check() have known and/or suspected locking issues. So far so good. ... > This patch (v4 and v5) explicitly avoids calling out to ptrace_has_cap() > (which calls out to LSM through has_ns_capability_*() -> > security_capable()) in PTRACE_MODE_IBPB case, exactly to avoid locking > issues with security modules; there are known callchains that lead to > deadlock. So rather than avoiding calling these, why not avoid doing the things inside the module specific code that cause the locking issues? Move the checks you put in the ptrace code into the security module hooks. I can't say 100%, but from what I've seen so far, it's locking in the audit sub-system that causes the issues, and that is pretty easy to work around. > With the same reasoning, security_ptrace_access_check() call is avoided, > only there is no know particular callchain that'd lead to a lock being > taken, but noone has done such audit (yet), as it's all hidden behind LSM > callbacks. I've had a pretty good look, and there's nothing magic about the LSM callbacks. > So please tell me what exactly you'd like to see changed in the IBPB patch > and why exactly, I am not seeing it yet. Short of a patch to show the changes (which I wish I could do today, but really can't) what I want to see is: - Put ptrace back to using the security module interfaces. - Identify where this causes locking issues and work with the module owners (a reasonable lot, all) to provide lock safe paths for the IBPB case. Otherwise, I have to add a new LSM hook right after your ptrace call and duplicate a whole lot of what you've just turned off, plus creating lock safe code that duplicates what ptrace already does. While I would rather have the side-channel checks be separate from the ptrace checks I can't justify doing both. I'm sorry if that isn't clearer. I am trying. > > Thanks, > > -- > Jiri Kosina > SUSE Labs
RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak
> -Original Message- > From: Jiri Kosina [mailto:ji...@kernel.org] > Sent: Monday, September 10, 2018 12:36 PM > To: Schaufler, Casey > Cc: Thomas Gleixner ; Ingo Molnar ; > Peter Zijlstra ; Josh Poimboeuf > ; Andrea Arcangeli ; > Woodhouse, David ; Andi Kleen ; > Tim Chen ; linux-kernel@vger.kernel.org; > x...@kernel.org > Subject: RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid > cross-process data leak > > On Mon, 10 Sep 2018, Schaufler, Casey wrote: > > > Yes, It would require that this patch be tested against all the existing > > security modules that provide a ptrace_access_check hook. It's not like > > the security module writers don't have a bunch of locking issues to deal > > with. > > Yeah, that was indeed my concern. > > So can we agree on doing this in the 2nd envisioned step, when this is > going to be replaced by LSM as discussed [1] previously? It you're going to call __ptrace_access_check(), which already includes an LSM hook, it makes a whole lot of sense to make that the path for doing any module specific checks. It seems wrong to disable the LSM hook there, then turn around and introduce a new one that does the check you just disabled. The patches I had proposed created a new LSM hook because there was not path to an existing hook. With your addition of __ptrace_access_check() that is no longer an issue once any locking problems are resolved. Rather than use a new hook, the existing ptrace hooks ought to work just fine, and any new checks can be added in a new module that has its own ptrace_access_check hook. > [1] > http://lkml.kernel.org/r/99FC4B6EFCEFD44486C35F4C281DC67321447094@OR > SMSX107.amr.corp.intel.com > > Thanks, > > -- > Jiri Kosina > SUSE Labs
RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak
> -Original Message- > From: Jiri Kosina [mailto:ji...@kernel.org] > Sent: Monday, September 10, 2018 12:14 PM > To: Schaufler, Casey > Cc: Thomas Gleixner ; Ingo Molnar ; > Peter Zijlstra ; Josh Poimboeuf > ; Andrea Arcangeli ; > Woodhouse, David ; Andi Kleen ; > Tim Chen ; linux-kernel@vger.kernel.org; > x...@kernel.org > Subject: RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid > cross-process data leak > > On Mon, 10 Sep 2018, Schaufler, Casey wrote: > > > Why are you dropping the LSM check here, when in v4 you fixed the > > SELinux audit locking issue? We can avoid introducing an LSM hook > > and all the baggage around it if you can do the > security_ptrace_access_check() > > here. > > So what guarantees that none of the hooks that > security_ptrace_access_check() is invoking will not be taking locks (from > scheduler context in this case)? The locking issue in the security modules is the same regardless of whether the call of security_ptrace_access_check() comes from the __ptrace_access_check() you're adding here or from a new security hook (I have proposed security_task_safe_sidechannel) that gets added in the same place later on. Adding a new hook results in duplication, because there now has to be code that does exactly the same thing as __ptrace_access_check() but without the new NOACCESS_CHECK mode. Yes, It would require that this patch be tested against all the existing security modules that provide a ptrace_access_check hook. It's not like the security module writers don't have a bunch of locking issues to deal with. > Thanks, > > -- > Jiri Kosina > SUSE Labs
RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak
> -Original Message-
> From: Jiri Kosina [mailto:ji...@kernel.org]
> Sent: Monday, September 10, 2018 2:24 AM
> To: Thomas Gleixner ; Ingo Molnar ;
> Peter Zijlstra ; Josh Poimboeuf
> ; Andrea Arcangeli ;
> Woodhouse, David ; Andi Kleen ;
> Tim Chen ; Schaufler, Casey
>
> Cc: linux-kernel@vger.kernel.org; x...@kernel.org
> Subject: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid
> cross-process data leak
>
> From: Jiri Kosina
>
> Currently, we are issuing IBPB only in cases when switching into a non-
> dumpable
> process, the rationale being to protect such 'important and security
> sensitive'
> processess (such as GPG) from data leak into a different userspace process via
> spectre v2.
>
> This is however completely insufficient to provide proper userspace-to-
> userpace
> spectrev2 protection, as any process can poison branch buffers before being
> scheduled out, and the newly scheduled process immediately becomes
> spectrev2
> victim.
>
> In order to minimize the performance impact (for usecases that do require
> spectrev2 protection), issue the barrier only in cases when switching between
> processess where the victim can't be ptraced by the potential attacker (as in
> such cases, the attacker doesn't have to bother with branch buffers at all).
>
> Fixes: 18bf3c3ea8 ("x86/speculation: Use Indirect Branch Prediction Barrier in
> context switch")
> Originally-by: Tim Chen
> Signed-off-by: Jiri Kosina
> ---
> arch/x86/mm/tlb.c | 31 ---
> include/linux/ptrace.h | 4
> kernel/ptrace.c| 12
> 3 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index e96b99eb800c..ed402441 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -180,6 +181,19 @@ static void sync_current_stack_to_mm(struct
> mm_struct *mm)
> }
> }
>
> +static bool ibpb_needed(struct task_struct *tsk, u64 last_ctx_id)
> +{
> + /*
> + * Check if the current (previous) task has access to the memory
> + * of the @tsk (next) task. If access is denied, make sure to
> + * issue a IBPB to stop user->user Spectre-v2 attacks.
> + *
> + * Note: __ptrace_may_access() returns 0 or -ERRNO.
> + */
> + return (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id &&
> + __ptrace_may_access(tsk, PTRACE_MODE_IBPB));
> +}
> +
> void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> struct task_struct *tsk)
> {
> @@ -262,18 +276,13 @@ void switch_mm_irqs_off(struct mm_struct *prev,
> struct mm_struct *next,
>* one process from doing Spectre-v2 attacks on another.
>*
>* As an optimization, flush indirect branches only when
> - * switching into processes that disable dumping. This
> - * protects high value processes like gpg, without having
> - * too high performance overhead. IBPB is *expensive*!
> - *
> - * This will not flush branches when switching into kernel
> - * threads. It will also not flush if we switch to idle
> - * thread and back to the same process. It will flush if we
> - * switch to a different non-dumpable process.
> + * switching into a processes that can't be ptrace by the
> + * current one (as in such case, attacker has much more
> + * convenient way how to tamper with the next process than
> + * branch buffer poisoning).
>*/
> - if (tsk && tsk->mm &&
> - tsk->mm->context.ctx_id != last_ctx_id &&
> - get_dumpable(tsk->mm) != SUID_DUMP_USER)
> + if (static_cpu_has(X86_FEATURE_USE_IBPB) &&
> + ibpb_needed(tsk, last_ctx_id))
> indirect_branch_prediction_barrier();
>
> if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 4f36431c380b..983d3f5545a8 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -64,12 +64,15 @@ extern void exit_ptrace(struct task_struct *tracer,
> struct list_head *dead);
> #define PTRACE_MODE_NOAUDIT 0x04
> #define PTRACE_MODE_FSCREDS 0x08
> #define PTRACE_MODE_REALCREDS 0x10
> +#define PTRACE_MO
RE: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can be applied on arbitrary tasks
> -Original Message- > From: Peter Zijlstra [mailto:pet...@infradead.org] > Sent: Wednesday, September 05, 2018 12:03 PM > To: Andrea Arcangeli > Cc: Jiri Kosina ; Andi Kleen ; Tim > Chen > ; Schaufler, Casey ; > Thomas Gleixner ; Ingo Molnar ; > Josh Poimboeuf ; Woodhouse, David > ; Oleg Nesterov ; linux- > ker...@vger.kernel.org; x...@kernel.org > Subject: Re: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can > be applied on arbitrary tasks > > On Wed, Sep 05, 2018 at 02:40:18PM -0400, Andrea Arcangeli wrote: > > [ 1838.769917] <> [] > avc_compute_av+0x126/0x1b5 > > That does read_lock(), which is not allowed from scheduler context. > > > [ 1838.777125] [] ? walk_tg_tree_from+0xbe/0x110 > > [ 1838.783828] [] avc_has_perm_noaudit+0xc4/0x110 > > In current code this can end up in avc_update_node() which uses > spin_lock(), which is a bug from scheduler context.o > > > [ 1838.790628] [] cred_has_capability+0x6b/0x120 > > [ 1838.797331] [] ? ktime_get+0x4c/0xd0 > > [ 1838.803160] [] ? > clockevents_program_event+0x6b/0xf0 > > [ 1838.810532] [] selinux_capable+0x2e/0x40 > > [ 1838.816748] [] security_capable_noaudit+0x15/0x20 > > [ 1838.823829] [] has_ns_capability_noaudit+0x15/0x20 > > [ 1838.831014] [] ptrace_has_cap+0x35/0x40 > > [ 1838.837126] [] ___ptrace_may_access+0xa7/0x1e0 > > [ 1838.843925] [] __schedule+0x26e/0xa00 > > [ 1838.849855] [] schedule_preempt_disabled+0x29/0x70 > > [ 1838.857041] [] cpu_startup_entry+0x184/0x290 > > [ 1838.863637] [] start_secondary+0x1da/0x250 > > So yes, looks like all that security LSM nonsense isn't going to work > here. What won't work is using the ptrace code. That is one of the reasons why you can't just blindly use it. Look at the patch set I submitted and you'll see that the SELinux selinux_task_safe_sidechannel() hook does not do the things that cause the lockup.
RE: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can be applied on arbitrary tasks
> -Original Message- > From: Peter Zijlstra [mailto:pet...@infradead.org] > Sent: Wednesday, September 05, 2018 1:00 AM > To: Jiri Kosina > Cc: Tim Chen ; Thomas Gleixner > ; Ingo Molnar ; Josh Poimboeuf > ; Andrea Arcangeli ; > Woodhouse, David ; Oleg Nesterov > ; Schaufler, Casey ; linux- > ker...@vger.kernel.org; x...@kernel.org > Subject: Re: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can > be applied on arbitrary tasks > > On Tue, Sep 04, 2018 at 07:35:29PM +0200, Jiri Kosina wrote: > > On Tue, 4 Sep 2018, Tim Chen wrote: > > > > > > Current ptrace_may_access() implementation assumes that the 'source' > task is > > > > always the caller (current). > > > > > > > > Expose ___ptrace_may_access() that can be used to apply the check on > arbitrary > > > > tasks. > > > > > > Casey recently has proposed putting the decision making of whether to > > > do IBPB in the security module. > > > > > > https://lwn.net/ml/kernel-hardening/20180815235355.14908-4- > casey.schauf...@intel.com/ > > > > > > That will have the advantage of giving the administrator a more > > > flexibility > > > of when to turn on IBPB. The policy is very similar to what you have > proposed here > > > but I think the security module is a more appropriate place for the > > > security > policy. > > > > Yeah, well, honestly, I have a bit hard time buying the "generic > > sidechannel prevention security module" idea, given how completely > > different in nature all the mitigations have been so far. I don't see that > > trying to abstract this somehow provides more clarity. > > > > So if this should be done in LSM, it'd probably have to be written by > > someone else than me :) who actually understands how the "sidechannel > LSM" > > idea works. > > Yeah, I'm not convinced on LSM either. Lets just do these here patches > first and then Casey can try and convince us later. Works for me. There are advantages to doing it either way. The LSM approach allows you to consider implications of LSM data, which you can't do otherwise. Once the hook is available it becomes the obvious place to do other checks.
RE: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can be applied on arbitrary tasks
> -Original Message- > From: Andrea Arcangeli [mailto:aarca...@redhat.com] > Sent: Tuesday, September 04, 2018 4:37 PM > To: Schaufler, Casey > Cc: Jiri Kosina ; Tim Chen ; > Thomas Gleixner ; Ingo Molnar ; > Peter Zijlstra ; Josh Poimboeuf > ; Woodhouse, David ; Oleg > Nesterov ; linux-kernel@vger.kernel.org; x...@kernel.org > Subject: Re: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can > be applied on arbitrary tasks > > Hello, > > On Tue, Sep 04, 2018 at 06:10:47PM +, Schaufler, Casey wrote: > > The real reason to use an LSM based approach is that overloading ptrace > > checks is a Really Bad Idea. Ptrace is a user interface. Side-channel is a > > processor interface. Even if ptrace_may_access() does exactly what you > > "Side channel is a processor interface" doesn't make me optimistic, > but I assume you're not speaking for Intel. Sorry, I've been working in security too long for my optimistic streak to be very wide. > > want it to for side-channel mitigation today it would be incredibly > > inappropriate to tie the two together for eternity. You don't want to > > restrict > > the ptrace checks to only those things that are also good for side-channel, > > and vice versa. > > It seems like you want to make this more configurable, Not especially, no. I have gotten considerable feedback that it should be configurable, and while I may not see the value myself, I do respect the people who've requested the configurability. > we have all > debugfs x86 specific tweaks to disable IBPB at runtime and we don't > allow a runtime opt-out of IBPB alone. > > If you shutdown either IBRS or retpolines at runtime with debugfs, > then IBPB goes away too. > > Giving a finegrined way to disable only IBPB we found was overkill > because IBPB has never been measurable if done only when the prev task > cannot ptrace the next task (which is a superset of the too weak > upstream not dumpable check, but still not a performance issue). > > Allowing IBPB runtime opt-out doesn't make much sense if you don't > allow to disable retpolines too still at runtime. And disabling > retpolines from LSM doesn't sounds the right place, it's an x86 > temporary quirk only relevant for current buggy CPUs. > > There should be a function that decides when IBPB and flush_RSB should > be run (flush_RSB has to be run if SMEP because with SMEP there's no > need to run flush_RSB at every kernel entry anymore), and that > function happens to check ptrace today, but it would check other stuff > too if we had other interfaces besides ptrace that would allow the > prev task to read the memory of the next task. So it's not so much > about ptrace nor about IBPB, it's about "IBPB&flush_RSB" vs "prev task > can read memory of next task". Then each arch can implement > "IBPB&flush_RSB" method its own way but the check is for the common > code and it should be in the scheduler and there's just 1 version of > this check needed. Right, I get that. There's more to ptrace access than "I can read the other task". There's more to what the processor is up to than IBPB. > > I don't think there should be a ton of different versions of this > function each providing a different answer, which is what LSM would > provide. If they provide different answers there should be different functions. It's a question of viewpoint. If you don't care about the SELinux viewpoint you shouldn't have to include it in your checks, any more than you care about x86 issues when you're running on a MIPS processor. > At most you can add a x86 debugfs tweak to shut off the logic but > again it would make more sense if retpolines could be shut off at > runtime too, doing it just for IBPB sounds backwards because it has > such an unmeasurable overhead. > > > Yes. That would be me. > > Even the attempt to make an innocuous call like > ptrace_has_cap(tcred->user_ns, mode) will eventually > deadlock there. Which is yet another reason there needs to be separate logic. > I used a PTRACE_MODE_ check that the ptrace check code uses to filter > out specific calls that may eventually enter LSM and hard lockup in > non reproducible workloads (some false positive IBPB is ok, especially > if it avoids a deadlock). Yes, even security people have to worry about locking. > Everything can be fixed in any way, but calling LSM from scheduler > code doesn't sound the most robust thing to do in general because what > works outside the scheduler doesn't work from within those semi atomic > regions (it tends to work initially until it eventually > deadlocks). The original code of such check, had all sort of deadlocks > because of that. What *is* the most robust way? Yes, there are locking issues. The code in the LSM infrastructure and in the security modules needs to be aware of that and deal with it. The SELinux code I proposed is more complex than it could be because the audit code requires locking that doesn't work in the switching context. > Thanks, > Andrea
RE: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can be applied on arbitrary tasks
> -Original Message- > From: Jiri Kosina [mailto:ji...@kernel.org] > Sent: Tuesday, September 04, 2018 10:35 AM > To: Tim Chen > Cc: Thomas Gleixner ; Ingo Molnar ; > Peter Zijlstra ; Josh Poimboeuf > ; Andrea Arcangeli ; > Woodhouse, David ; Oleg Nesterov > ; Schaufler, Casey ; linux- > ker...@vger.kernel.org; x...@kernel.org > Subject: Re: [PATCH v3 1/3] ptrace: Provide ___ptrace_may_access() that can > be applied on arbitrary tasks > > On Tue, 4 Sep 2018, Tim Chen wrote: > > > > Current ptrace_may_access() implementation assumes that the 'source' > task is > > > always the caller (current). > > > > > > Expose ___ptrace_may_access() that can be used to apply the check on > arbitrary > > > tasks. > > > > Casey recently has proposed putting the decision making of whether to > > do IBPB in the security module. > > > > https://lwn.net/ml/kernel-hardening/20180815235355.14908-4- > casey.schauf...@intel.com/ > > > > That will have the advantage of giving the administrator a more flexibility > > of when to turn on IBPB. The policy is very similar to what you have > > proposed > here > > but I think the security module is a more appropriate place for the security > policy. > > Yeah, well, honestly, I have a bit hard time buying the "generic > sidechannel prevention security module" idea, given how completely > different in nature all the mitigations have been so far. I don't see that > trying to abstract this somehow provides more clarity. The real reason to use an LSM based approach is that overloading ptrace checks is a Really Bad Idea. Ptrace is a user interface. Side-channel is a processor interface. Even if ptrace_may_access() does exactly what you want it to for side-channel mitigation today it would be incredibly inappropriate to tie the two together for eternity. You don't want to restrict the ptrace checks to only those things that are also good for side-channel, and vice versa. > So if this should be done in LSM, it'd probably have to be written by > someone else than me :) who actually understands how the "sidechannel LSM" > idea works. Yes. That would be me. > > Thanks, > > -- > Jiri Kosina > SUSE Labs
RE: [kernel-hardening] Re: [PATCH 1/2] security, perf: allow further restriction of perf_event_open
> -Original Message- > From: Peter Zijlstra [mailto:pet...@infradead.org] > Sent: Wednesday, August 03, 2016 7:41 AM > To: Kees Cook > Cc: Jeff Vander Stoep ; Ingo Molnar ; > Arnaldo Carvalho de Melo ; Alexander Shishkin > ; linux-...@vger.kernel.org; kernel- > harden...@lists.openwall.com; LKML ; > Jonathan Corbet ; Eric W. Biederman > > Subject: Re: [kernel-hardening] Re: [PATCH 1/2] security, perf: allow further > restriction of perf_event_open > > On Tue, Aug 02, 2016 at 01:51:47PM -0700, Kees Cook wrote: > > Let me take this another way instead. What would be a better way to > > provide a mechanism for system owners to disable perf without an LSM? > > (Since far fewer folks run with an enforcing "big" LSM: I'm seeking as > > wide a coverage as possible.) > > Could something like a new capability bit work? Yes, it could, but we don't have a real good experience with the advanced use of capabilities. The big distros and the "OS"s have yet to adopt them seriously. It's the same sort of issue, really. The granularity gets to you. > I'm thinking that applications that have network connections already > drop all possible capabilities (I know, unlikely to be true, but should > be true for most stuff I hope). This would disable perf for remote code > execution exploits, including web-browsers and the lot. > > It would keep perf working for local stuff by default, although > obviously with pam_cap you can limit this when and where needed. > > For Android this could mean the JVM explicitly dropping the cap for its > 'children' while retaining the use itself. And this would also keep perf > working on the ADB shell stuff. > > > And, I think this would allow a JIT executable to gain the cap using > file caps, even when the user using it doesn't have it, which would keep > things usable even in restricted environments. > > > Or am I misunderstanding capabilities -- which is entirely possible.
