* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-12 16:16 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <7c367905-e8c9-7665-d923-c850e05c757a@tycho.nsa.gov>
On 12.02.2020 18:21, Stephen Smalley wrote:
> On 2/12/20 8:53 AM, Alexey Budankov wrote:
>> On 12.02.2020 16:32, Stephen Smalley wrote:
>>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>>> Hi Stephen,
>>>>
>>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>>
>>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>>
>>>> <SNIP>
>>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>>>
>>>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>>>
>>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>>
>>>>>> static inline bool perfmon_capable(void)
>>>>>> {
>>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>>> }
>>>>>>
>>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>>> privileged process.
>>>>>>
>>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>>> based approach to use perf_event_open system call.
>>>>>
>>>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>>>
>>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>>>> When running perf stat -a I am observing this AVC audit messages:
>>>>
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>
>>>> However there is no capability related messages around. I suppose my refpolicy should
>>>> be modified somehow to observe capability related AVCs.
>>>>
>>>> Could you please comment or clarify on how to enable caps related AVCs in order
>>>> to test the concerned logging.
>>>
>>> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
>>
>> Yes, I already have it like this:
>> common cap2
>> {
>> <------>mac_override<--># unused by SELinux
>> <------>mac_admin
>> <------>syslog
>> <------>wake_alarm
>> <------>block_suspend
>> <------>audit_read
>> <------>perfmon
>> }
>>
>> dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
>> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
>
> Some denials may be silenced by dontaudit rules; semodule -DB will strip those and semodule -B will restore them. Other possibility is that the process doesn't have CAP_PERFMON in its effective set and therefore never reaches SELinux at all; denied first by the capability module.
Yes, that all makes sense.
selinux_capable() calls avc_audit() logging but cap_capable() doesn't, so proper order matters.
I am doing debug tracing of the kernel code to reveal the exact reasons.
~Alexey
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Stephen Smalley @ 2020-02-12 16:09 UTC (permalink / raw)
To: Daniel Colascione, Casey Schaufler
Cc: Tim Murray, Nosh Minwalla, Nick Kralevich, Lokesh Gidra,
linux-kernel, Linux API, selinux, LSM List
In-Reply-To: <CAKOZueuh2MR4UKi60-GVgPkXjncHx8J=mTTjRquB82CfS7DxBA@mail.gmail.com>
On 2/11/20 6:27 PM, Daniel Colascione wrote:
> On Tue, Feb 11, 2020 at 3:13 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> On 2/11/2020 2:55 PM, Daniel Colascione wrote:
>>> Userfaultfd in unprivileged contexts could be potentially very
>>> useful. We'd like to harden userfaultfd to make such unprivileged use
>>> less risky. This patch series allows SELinux to manage userfaultfd
>>> file descriptors and allows administrators to limit userfaultfd to
>>> servicing user-mode faults, increasing the difficulty of using
>>> userfaultfd in exploit chains invoking delaying kernel faults.
>>>
>>> A new anon_inodes interface allows callers to opt into SELinux
>>> management of anonymous file objects. In this mode, anon_inodes
>>> creates new ephemeral inodes for anonymous file objects instead of
>>> reusing a singleton dummy inode. A new LSM hook gives security modules
>>> an opportunity to configure and veto these ephemeral inodes.
>>>
>>> Existing anon_inodes users must opt into the new functionality.
>>>
>>> Daniel Colascione (6):
>>> Add a new flags-accepting interface for anonymous inodes
>>> Add a concept of a "secure" anonymous file
>>> Teach SELinux about a new userfaultfd class
>>> Wire UFFD up to SELinux
>>> Let userfaultfd opt out of handling kernel-mode faults
>>> Add a new sysctl for limiting userfaultfd to user mode faults
>>
>> This must be posted to the linux Security Module list
>> <linux-security-module@vger.kernel.org>
>
> Added. I thought selinux@ was sufficient.
scripts/get_maintainer.pl can be helpful in identifying relevant lists
and maintainers for each patch. I don't use its output blindly as it
tends to over-approximate but since your patches span the VFS, LSM
framework, and selinux, you do need to include relevant
maintainers/lists for each.
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: KP Singh @ 2020-02-12 16:04 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Jann Horn, KP Singh, kernel list, bpf,
linux-security-module, Brendan Jackman, Florent Revest,
Thomas Garnier, Alexei Starovoitov, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <ff6dec98-5e33-4603-1b90-e4bff23695cc@iogearbox.net>
On 12-Feb 14:27, Daniel Borkmann wrote:
> On 2/12/20 3:45 AM, Alexei Starovoitov wrote:
> > On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
> > >
> > > Another approach could be to have a special nop inside call_int_hook()
> > > macro which would then get patched to avoid these situations. Somewhat
> > > similar like static keys where it could be defined anywhere in text but
> > > with updating of call_int_hook()'s RC for the verdict.
> >
> > Sounds nice in theory. I couldn't quite picture how that would look
> > in the code, so I hacked:
> > diff --git a/security/security.c b/security/security.c
> > index 565bc9b67276..ce4bc1e5e26c 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -28,6 +28,7 @@
> > #include <linux/string.h>
> > #include <linux/msg.h>
> > #include <net/flow.h>
> > +#include <linux/jump_label.h>
> >
> > #define MAX_LSM_EVM_XATTR 2
> >
> > @@ -678,12 +679,26 @@ static void __init lsm_early_task(struct task_struct *task)
> > * This is a hook that returns a value.
> > */
> >
> > +#define LSM_HOOK_NAME(FUNC) \
> > + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##FUNC);
> > +#include <linux/lsm_hook_names.h>
> > +#undef LSM_HOOK_NAME
> > +__diag_push();
> > +__diag_ignore(GCC, 8, "-Wstrict-prototypes", "");
> > +#define LSM_HOOK_NAME(FUNC) \
> > + int bpf_lsm_call_##FUNC() {return 0;}
> > +#include <linux/lsm_hook_names.h>
> > +#undef LSM_HOOK_NAME
> > +__diag_pop();
> > +
> > #define call_void_hook(FUNC, ...) \
> > do { \
> > struct security_hook_list *P; \
> > \
> > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> > P->hook.FUNC(__VA_ARGS__); \
> > + if (static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> > + (void)bpf_lsm_call_##FUNC(__VA_ARGS__); \
> > } while (0)
> >
> > #define call_int_hook(FUNC, IRC, ...) ({ \
> > @@ -696,6 +711,8 @@ static void __init lsm_early_task(struct task_struct *task)
> > if (RC != 0) \
> > break; \
> > } \
> > + if (RC == IRC && static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> > + RC = bpf_lsm_call_##FUNC(__VA_ARGS__); \
>
> Nit: the `RC == IRC` test could be moved behind the static_branch_unlikely() so
> that it would be bypassed when not enabled.
>
> > } while (0); \
> > RC; \
> > })
> >
> > The assembly looks good from correctness and performance points.
> > union security_list_options can be split into lsm_hook_names.h too
> > to avoid __diag_ignore. Is that what you have in mind?
> > I don't see how one can improve call_int_hook() macro without
> > full refactoring of linux/lsm_hooks.h
> > imo static_key doesn't have to be there in the first set. We can add this
> > optimization later.
>
> Yes, like the above diff looks good, and then we'd dynamically attach the program
> at bpf_lsm_call_##FUNC()'s fexit hook for a direct jump, so all the security_blah()
> internals could stay as-is which then might also address Jann's concerns wrt
> concrete annotation as well as potential locking changes inside security_blah().
> Agree that patching out via static key could be optional but since you were talking
> about avoiding indirect jumps..
I like this approach as well. Will give it a go and update the
patches. Thanks a lot for your inputs!
- KP
>
> Thanks,
> Daniel
^ permalink raw reply
* Re: [PATCH v8 10/11] docs: proc: add documentation for "hidepid=4" and "subset=pidfs" options and new mount behavior
From: Alexey Gladkov @ 2020-02-12 16:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <CALCETrWOXXYy5fo+D0wVBEviyk38ACqvO5Fep_oTEY6+UrS=4g@mail.gmail.com>
On Mon, Feb 10, 2020 at 10:29:23AM -0800, Andy Lutomirski wrote:
> On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> >
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> > ---
> > Documentation/filesystems/proc.txt | 53 ++++++++++++++++++++++++++++++
> > 1 file changed, 53 insertions(+)
> >
> > diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> > index 99ca040e3f90..4741fd092f36 100644
> > --- a/Documentation/filesystems/proc.txt
> > +++ b/Documentation/filesystems/proc.txt
> > @@ -50,6 +50,8 @@ Table of Contents
> > 4 Configuring procfs
> > 4.1 Mount options
> >
> > + 5 Filesystem behavior
> > +
> > ------------------------------------------------------------------------------
> > Preface
> > ------------------------------------------------------------------------------
> > @@ -2021,6 +2023,7 @@ The following mount options are supported:
> >
> > hidepid= Set /proc/<pid>/ access mode.
> > gid= Set the group authorized to learn processes information.
> > + subset= Show only the specified subset of procfs.
> >
> > hidepid=0 means classic mode - everybody may access all /proc/<pid>/ directories
> > (default).
> > @@ -2042,6 +2045,56 @@ information about running processes, whether some daemon runs with elevated
> > privileges, whether other user runs some sensitive program, whether other users
> > run any program at all, etc.
> >
> > +hidepid=4 means that procfs should only contain /proc/<pid>/ directories
> > +that the caller can ptrace.
>
> I have a couple of minor nits here.
>
> First, perhaps we could stop using magic numbers and use words.
> hidepid=ptraceable is actually comprehensible, whereas hidepid=4
> requires looking up what '4' means.
Do you mean to add string aliases for the values?
hidepid=0 == hidepid=default
hidepid=1 == hidepid=restrict
hidepid=2 == hidepid=ownonly
hidepid=4 == hidepid=ptraceable
Something like that ?
> Second, there is PTRACE_MODE_ATTACH and PTRACE_MODE_READ. Which is it?
This is PTRACE_MODE_READ.
> > +
> > gid= defines a group authorized to learn processes information otherwise
> > prohibited by hidepid=. If you use some daemon like identd which needs to learn
> > information about processes information, just add identd to this group.
>
> How is this better than just creating an entirely separate mount a
> different hidepid and a different gid owning it?
I'm not sure I understand the question. Now you cannot have two proc with
different hidepid in the same pid_namespace.
> In any event,
> usually gid= means that this gid is the group owner of inodes. Let's
> call it something different. gid_override_hidepid might be credible.
> But it's also really weird -- do different groups really see different
> contents when they read a directory?
If you use hidepid=2,gid=wheel options then the user is not in the wheel
group will see only their processes and the user in the wheel group will
see whole tree. The gid= is a kind of whitelist for hidepid=1|2.
--
Rgrds, legion
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Casey Schaufler @ 2020-02-12 15:52 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Jann Horn, KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Matthew Garrett,
Christian Brauner, Mickaël Salaün, Florent Revest,
Brendan Jackman, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Kernel Team, Casey Schaufler
In-Reply-To: <20200212024542.gdsafhvqykucdp4h@ast-mbp>
On 2/11/2020 6:45 PM, Alexei Starovoitov wrote:
> On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
>> Another approach could be to have a special nop inside call_int_hook()
>> macro which would then get patched to avoid these situations. Somewhat
>> similar like static keys where it could be defined anywhere in text but
>> with updating of call_int_hook()'s RC for the verdict.
Tell me again why you can't register your BPF hooks like all the
other security modules do? You keep reintroducing BPF as a special
case, and I don't see why.
> Sounds nice in theory. I couldn't quite picture how that would look
> in the code, so I hacked:
> diff --git a/security/security.c b/security/security.c
> index 565bc9b67276..ce4bc1e5e26c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,7 @@
> #include <linux/string.h>
> #include <linux/msg.h>
> #include <net/flow.h>
> +#include <linux/jump_label.h>
>
> #define MAX_LSM_EVM_XATTR 2
>
> @@ -678,12 +679,26 @@ static void __init lsm_early_task(struct task_struct *task)
> * This is a hook that returns a value.
> */
>
> +#define LSM_HOOK_NAME(FUNC) \
> + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##FUNC);
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK_NAME
> +__diag_push();
> +__diag_ignore(GCC, 8, "-Wstrict-prototypes", "");
> +#define LSM_HOOK_NAME(FUNC) \
> + int bpf_lsm_call_##FUNC() {return 0;}
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK_NAME
> +__diag_pop();
> +
> #define call_void_hook(FUNC, ...) \
> do { \
> struct security_hook_list *P; \
> \
> hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> P->hook.FUNC(__VA_ARGS__); \
> + if (static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> + (void)bpf_lsm_call_##FUNC(__VA_ARGS__); \
> } while (0)
>
> #define call_int_hook(FUNC, IRC, ...) ({ \
> @@ -696,6 +711,8 @@ static void __init lsm_early_task(struct task_struct *task)
> if (RC != 0) \
> break; \
> } \
> + if (RC == IRC && static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> + RC = bpf_lsm_call_##FUNC(__VA_ARGS__); \
> } while (0); \
> RC; \
> })
>
> The assembly looks good from correctness and performance points.
> union security_list_options can be split into lsm_hook_names.h too
> to avoid __diag_ignore. Is that what you have in mind?
> I don't see how one can improve call_int_hook() macro without
> full refactoring of linux/lsm_hooks.h
> imo static_key doesn't have to be there in the first set. We can add this
> optimization later.
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-02-12 15:45 UTC (permalink / raw)
To: Alexey Budankov
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <7c367905-e8c9-7665-d923-c850e05c757a@tycho.nsa.gov>
On 2/12/20 10:21 AM, Stephen Smalley wrote:
> On 2/12/20 8:53 AM, Alexey Budankov wrote:
>> On 12.02.2020 16:32, Stephen Smalley wrote:
>>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>>> Hi Stephen,
>>>>
>>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>>
>>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>>
>>>> <SNIP>
>>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system
>>>>>>>>>>> performance
>>>>>>>>>>
>>>>>>>>>> Why _noaudit()? Normally only used when a permission failure
>>>>>>>>>> is non-fatal to the operation. Otherwise, we want the audit
>>>>>>>>>> message.
>>>>>>
>>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>>
>>>>>> static inline bool perfmon_capable(void)
>>>>>> {
>>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>>> }
>>>>>>
>>>>>> It keeps the implementation simple and readable. The
>>>>>> implementation is more
>>>>>> performant in the sense of calling the API - one capable() call
>>>>>> for CAP_PERFMON
>>>>>> privileged process.
>>>>>>
>>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and
>>>>>> unprivileged processes,
>>>>>> but this bloating also advertises and leverages using more secure
>>>>>> CAP_PERFMON
>>>>>> based approach to use perf_event_open system call.
>>>>>
>>>>> I can live with that. We just need to document that when you see
>>>>> both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process,
>>>>> try only allowing CAP_PERFMON first and see if that resolves the
>>>>> issue. We have a similar issue with CAP_DAC_READ_SEARCH versus
>>>>> CAP_DAC_OVERRIDE.
>>>>
>>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>>> I am using the refpolicy version with enabled perf_event tclass [1],
>>>> in permissive mode.
>>>> When running perf stat -a I am observing this AVC audit messages:
>>>>
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for
>>>> pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t
>>>> tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel }
>>>> for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t
>>>> tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for
>>>> pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t
>>>> tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write }
>>>> for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t
>>>> tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>
>>>> However there is no capability related messages around. I suppose my
>>>> refpolicy should
>>>> be modified somehow to observe capability related AVCs.
>>>>
>>>> Could you please comment or clarify on how to enable caps related
>>>> AVCs in order
>>>> to test the concerned logging.
>>>
>>> The new perfmon permission has to be defined in your policy; you'll
>>> have a message in dmesg about "Permission perfmon in class
>>> capability2 not defined in policy.". You can either add it to the
>>> common cap2 definition in refpolicy/policy/flask/access_vectors and
>>> rebuild your policy or extract your base module as CIL, add it there,
>>> and insert the updated module.
>>
>> Yes, I already have it like this:
>> common cap2
>> {
>> <------>mac_override<--># unused by SELinux
>> <------>mac_admin
>> <------>syslog
>> <------>wake_alarm
>> <------>block_suspend
>> <------>audit_read
>> <------>perfmon
>> }
>>
>> dmesg stopped reporting perfmon as not defined but audit.log still
>> doesn't report CAP_PERFMON denials.
>> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however
>> perfmon_capable() does check for it.
>
> Some denials may be silenced by dontaudit rules; semodule -DB will strip
> those and semodule -B will restore them. Other possibility is that the
> process doesn't have CAP_PERFMON in its effective set and therefore
> never reaches SELinux at all; denied first by the capability module.
Also, the fact that your denials are showing up in user_systemd_t
suggests that something is off in your policy or userspace/distro; I
assume that is a domain type for the systemd --user instance, but your
shell and commands shouldn't be running in that domain (user_t would be
more appropriate for that).
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-02-12 15:21 UTC (permalink / raw)
To: Alexey Budankov
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <8141da2e-49cf-c02d-69e9-8a7cbdc91431@linux.intel.com>
On 2/12/20 8:53 AM, Alexey Budankov wrote:
> On 12.02.2020 16:32, Stephen Smalley wrote:
>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>> Hi Stephen,
>>>
>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>
>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>
>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>
>>> <SNIP>
>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>>
>>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>>
>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>
>>>>> static inline bool perfmon_capable(void)
>>>>> {
>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>> }
>>>>>
>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>> privileged process.
>>>>>
>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>> based approach to use perf_event_open system call.
>>>>
>>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>>
>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>>> When running perf stat -a I am observing this AVC audit messages:
>>>
>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>
>>> However there is no capability related messages around. I suppose my refpolicy should
>>> be modified somehow to observe capability related AVCs.
>>>
>>> Could you please comment or clarify on how to enable caps related AVCs in order
>>> to test the concerned logging.
>>
>> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
>
> Yes, I already have it like this:
> common cap2
> {
> <------>mac_override<--># unused by SELinux
> <------>mac_admin
> <------>syslog
> <------>wake_alarm
> <------>block_suspend
> <------>audit_read
> <------>perfmon
> }
>
> dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
Some denials may be silenced by dontaudit rules; semodule -DB will strip
those and semodule -B will restore them. Other possibility is that the
process doesn't have CAP_PERFMON in its effective set and therefore
never reaches SELinux at all; denied first by the capability module.
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-12 14:59 UTC (permalink / raw)
To: LKML
Cc: Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <20200212144921.sykucj4mekcziicz@comp-core-i7-2640m-0182e6>
Alexey Gladkov <gladkov.alexey@gmail.com> writes:
> On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote:
>> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
>>
>> > This allows to flush dcache entries of a task on multiple procfs mounts
>> > per pid namespace.
>> >
>> > The RCU lock is used because the number of reads at the task exit time
>> > is much larger than the number of procfs mounts.
>>
>> A couple of quick comments.
>>
>> > Cc: Kees Cook <keescook@chromium.org>
>> > Cc: Andy Lutomirski <luto@kernel.org>
>> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
>> > ---
>> > fs/proc/base.c | 20 +++++++++++++++-----
>> > fs/proc/root.c | 27 ++++++++++++++++++++++++++-
>> > include/linux/pid_namespace.h | 2 ++
>> > include/linux/proc_fs.h | 2 ++
>> > 4 files changed, 45 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/fs/proc/base.c b/fs/proc/base.c
>> > index 4ccb280a3e79..24b7c620ded3 100644
>> > --- a/fs/proc/base.c
>> > +++ b/fs/proc/base.c
>> > @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
>> > .permission = proc_pid_permission,
>> > };
>> >
>> > -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>> > +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
>> Perhaps just rename things like:
>> > +static void proc_flush_task_root(struct dentry *root, pid_t pid, pid_t tgid)
>> > {
>>
>> I don't think the mnt_ prefix conveys any information, and it certainly
>> makes everything longer and more cumbersome.
>>
>> > struct dentry *dentry, *leader, *dir;
>> > char buf[10 + 1];
>> > @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>> > name.name = buf;
>> > name.len = snprintf(buf, sizeof(buf), "%u", pid);
>> > /* no ->d_hash() rejects on procfs */
>> > - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
>> > + dentry = d_hash_and_lookup(mnt_root, &name);
>> > if (dentry) {
>> > d_invalidate(dentry);
>> > dput(dentry);
>> > @@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>> >
>> > name.name = buf;
>> > name.len = snprintf(buf, sizeof(buf), "%u", tgid);
>> > - leader = d_hash_and_lookup(mnt->mnt_root, &name);
>> > + leader = d_hash_and_lookup(mnt_root, &name);
>> > if (!leader)
>> > goto out;
>> >
>> > @@ -3208,14 +3208,24 @@ void proc_flush_task(struct task_struct *task)
>> > int i;
>> > struct pid *pid, *tgid;
>> > struct upid *upid;
>> > + struct dentry *mnt_root;
>> > + struct proc_fs_info *fs_info;
>> >
>> > pid = task_pid(task);
>> > tgid = task_tgid(task);
>> >
>> > for (i = 0; i <= pid->level; i++) {
>> > upid = &pid->numbers[i];
>> > - proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
>> > - tgid->numbers[i].nr);
>> > +
>> > + rcu_read_lock();
>> > + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
>> > + mnt_root = fs_info->m_super->s_root;
>> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
>> > + }
>> > + rcu_read_unlock();
>> > +
>> > + mnt_root = upid->ns->proc_mnt->mnt_root;
>> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
>>
>> I don't think this following of proc_mnt is needed. It certainly
>> shouldn't be. The loop through all of the super blocks should be
>> enough.
>
> Yes, thanks!
>
>> Once this change goes through. UML can be given it's own dedicated
>> proc_mnt for the initial pid namespace, and proc_mnt can be removed
>> entirely.
>
> After you deleted the old sysctl syscall we could probably do it.
>
>> Unless something has changed recently UML is the only other user of
>> pid_ns->proc_mnt. That proc_mnt really only exists to make the loop in
>> proc_flush_task easy to write.
>
> Now I think, is there any way to get rid of proc_mounts or even
> proc_flush_task somehow.
>
>> It also probably makes sense to take the rcu_read_lock() over
>> that entire for loop.
>
> Al Viro pointed out to me that I cannot use rcu locks here :(
Fundamentally proc_flush_task is an optimization. Just getting rid of
dentries earlier. At least at one point it was an important
optimization because the old process dentries would just sit around
doing nothing for anyone.
I wonder if instead of invalidating specific dentries we could instead
fire wake up a shrinker and point it at one or more instances of proc.
The practical challenge I see is something might need to access the
dentries to see that they are invalid.
We definitely could try without this optimization and see what happens.
Eric
^ permalink raw reply
* Re: [PATCH v8 03/11] proc: move /proc/{self|thread-self} dentries to proc_fs_info
From: Alexey Gladkov @ 2020-02-12 15:00 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <CALCETrWGpRr86tVKJU-sEMcg+x0Yzp+TbiBhrAc71RaO8=DYGQ@mail.gmail.com>
On Mon, Feb 10, 2020 at 10:23:23AM -0800, Andy Lutomirski wrote:
> On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> >
> > This is a preparation patch that moves /proc/{self|thread-self} dentries
> > to be stored inside procfs fs_info struct instead of making them per pid
> > namespace. Since we want to support multiple procfs instances we need to
> > make sure that these dentries are also per-superblock instead of
> > per-pidns,
>
> The changelog makes perfect sense so far...
>
> > unmounting a private procfs won't clash with other procfs
> > mounts.
>
> This doesn't parse as part of the previous sentence. I'm also not
> convinced that this really involves unmounting per se. Maybe just
> delete these words.
Sure. I will remove this part.
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v8 05/11] proc: add helpers to set and get proc hidepid and gid mount options
From: Alexey Gladkov @ 2020-02-12 14:57 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <CALCETrVjv04OOdzGNf7sRmRR-KUgY7xdMXA236nHZ1arn0KwVQ@mail.gmail.com>
On Mon, Feb 10, 2020 at 10:30:45AM -0800, Andy Lutomirski wrote:
> On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> >
> > This is a cleaning patch to add helpers to set and get proc mount
> > options instead of directly using them. This make it easy to track
> > what's happening and easy to update in future.
>
> On a cursory inspection, this looks like it obfuscates the code, and I
> don't see where it does something useful later in the series. What is
> this abstraction for?
To be honest, this part is from Djalal Harouni. For me, these wrappers
exist for a slight increase in readability.
I will not cry if you tell me to remove them :)
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH 00/24] user_namespace: introduce fsid mappings
From: Christian Brauner @ 2020-02-12 14:51 UTC (permalink / raw)
To: Jann Horn
Cc: linux-security-module, Kees Cook, Jonathan Corbet, kernel list,
Linux Containers, smbarber, Alexander Viro, Linux API,
linux-fsdevel, Alexey Dobriyan, Eric W. Biederman
In-Reply-To: <CAG48ez1GKOfXDZFD7-hGGjT8L9YEojn94DU5_=W8HL3pzdrCgg@mail.gmail.com>
On Tue, Feb 11, 2020 at 09:55:46PM +0100, Jann Horn via Containers wrote:
> On Tue, Feb 11, 2020 at 5:59 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> > This is the implementation of shiftfs which was cooked up during lunch at
> > Linux Plumbers 2019 the day after the container's microconference. The
> > idea is a design-stew from Stéphane, Aleksa, Eric, and myself. Back then
> > we all were quite busy with other work and couldn't really sit down and
> > implement it. But I took a few days last week to do this work, including
> > demos and performance testing.
> > This implementation does not require us to touch the vfs substantially
> > at all. Instead, we implement shiftfs via fsid mappings.
> > With this patch, it took me 20 mins to port both LXD and LXC to support
> > shiftfs via fsid mappings.
> >
> > For anyone wanting to play with this the branch can be pulled from:
> > https://github.com/brauner/linux/tree/fsid_mappings
> > https://gitlab.com/brauner/linux/-/tree/fsid_mappings
> > https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=fsid_mappings
> >
> > The main use case for shiftfs for us is in allowing shared writable
> > storage to multiple containers using non-overlapping id mappings.
> > In such a scenario you want the fsids to be valid and identical in both
> > containers for the shared mount. A demo for this exists in [3].
> > If you don't want to read on, go straight to the other demos below in
> > [1] and [2].
>
> I guess essentially this means that you want to have UID separation
> between containers to prevent the containers - or their owners - from
> interfering between each other, but for filesystem access, you don't
> want to isolate them from each other using DAC controls on the files
> and folders inside the containers' directory hierarchies, instead
> relying on mode-0700 parent directories to restrict access to the
> container owner? Or would you still have separate UIDs for e.g. the
> container's UID range 0-65535, and then map the shared UID range at
> 100000, or something like that?
Yes.
So if you look at the permissions right now for the directory under
which the rootfs for the container and other stuff resides we have
root@wittgenstein|/var/lib/lxd/storage-pools/zfs/containers
> perms *
d--x------ 100 alp1
d--x------ 100 f1
d--x------ 100 f2
We don't really share the rootfs between containers right now since we
treat them as standalone systems but with fsid mappings that's possible
too. Layer-sharing-centric runtimes very much will want something like
that.
>
> > People not as familiar with user namespaces might not be aware that fsid
> > mappings already exist. Right now, fsid mappings are always identical to
> > id mappings. Specifically, the kernel will lookup fsuids in the uid
> > mappings and fsgids in the gid mappings of the relevant user namespace.
>
> That's a bit like saying that a kernel without CONFIG_USER_NS still
> has user ID mappings, they just happen to be identity mappings. :P
If you have CONFIG_USER_NS=n then you have (as you're well aware)
[<0, 0>, <1,1>, ..., <n,n>] so yeah that's true and analyzing it like
that makes sense. :P
>
> > With this patch series we simply introduce the ability to create fsid
> > mappings that are different from the id mappings of a user namespace.
> >
> > In the usual case of running an unprivileged container we will have
> > setup an id mapping, e.g. 0 100000 100000. The on-disk mapping will
> > correspond to this id mapping, i.e. all files which we want to appear as
> > 0:0 inside the user namespace will be chowned to 100000:100000 on the
> > host. This works, because whenever the kernel needs to do a filesystem
> > access it will lookup the corresponding uid and gid in the idmapping
> > tables of the container.
> > Now think about the case where we want to have an id mapping of 0 100000
> > 100000 but an on-disk mapping of 0 300000 100000 which is needed to e.g.
> > share a single on-disk mapping with multiple containers that all have
> > different id mappings.
> > This will be problematic. Whenever a filesystem access is requested, the
> > kernel will now try to lookup a mapping for 300000 in the id mapping
> > tables of the user namespace but since there is none the files will
> > appear to be owned by the overflow id, i.e. usually 65534:65534 or
> > nobody:nogroup.
> >
> > With fsid mappings we can solve this by writing an id mapping of 0
> > 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> > access the kernel will now lookup the mapping for 300000 in the fsid
> > mapping tables of the user namespace. And since such a mapping exists,
> > the corresponding files will have correct ownership.
>
> Sorry to bring up something as disgusting as setuid execution, but:
No that's exactly what this needs. :)
> What happens when there's a setuid root file with ->i_uid==300000? I
> guess the only way to make that work inside the containers would be
> something like make_kuid(current_user_ns(),
> from_kfsuid(current_user_ns(), inode->i_uid)) in the setuid execve
> path?
What's the specific callpath you're thinking about?
So if you look at patch
https://lore.kernel.org/lkml/20200211165753.356508-16-christian.brauner@ubuntu.com/
it does
- new->suid = new->fsuid = new->euid;
- new->sgid = new->fsgid = new->egid;
+ fsuid = from_kuid_munged(new->user_ns, new->euid);
+ kfsuid = make_kfsuid(new->user_ns, fsuid);
+ new->suid = new->euid;
+ new->fsuid = kfsuid;
+
+ fsgid = from_kgid_munged(new->user_ns, new->egid);
+ kfsgid = make_kfsgid(new->user_ns, fsgid);
+ new->sgid = new->egid;
+ new->fsgid = kfsgid;
One thing I definitely missed though in the setuid path is to adapt
fs/exec.c:bprm_fill_uid():
diff --git a/fs/exec.c b/fs/exec.c
index 74d88dab98dd..ad839934fdff 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1547,8 +1547,8 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
inode_unlock(inode);
/* We ignore suid/sgid if there are no mappings for them in the ns */
- if (!kuid_has_mapping(bprm->cred->user_ns, uid) ||
- !kgid_has_mapping(bprm->cred->user_ns, gid))
+ if (!kfsuid_has_mapping(bprm->cred->user_ns, uid) ||
+ !kfsgid_has_mapping(bprm->cred->user_ns, gid))
return;
if (mode & S_ISUID) {
>
> > A note on proc (and sys), the proc filesystem is special in sofar as it
> > only has a single superblock that is (currently but might be about to
> > change) visible in all user namespaces (same goes for sys). This means
> > it has special semantics in many ways, including how file ownership and
> > access works. The fsid mapping implementation does not alter how proc
> > (and sys) ownership works. proc and sys will both continue to lookup
> > filesystem access in id mapping tables.
>
> In your example, a process with namespaced UID set (0, 0, 0, 0) will
> have kernel UIDs (100000, 100000, 100000, 300000), right? And then if
Yes.
> I want to open /proc/$pid/personality of another process with the same
> UIDs, may_open() will call inode_permission() -> do_inode_permission()
> -> generic_permission() -> acl_permission_check(), which will compare
> current_fsuid() (which is 300000) against inode->i_uid. But
> inode->i_uid was filled by proc_pid_make_inode()->task_dump_owner(),
> which set inode->i_uid to 100000, right?
Yes. That should be fixable by something like below, I think. (And we can
probably shortcut this by adding a helper that does tell us whether there's
been any fsid mapping setup or not for this user namespace.)
static int acl_permission_check(struct inode *inode, int mask)
{
+ kuid_t kuid;
unsigned int mode = inode->i_mode;
- if (likely(uid_eq(current_fsuid(), inode->i_uid)))
+ if (!is_userns_visible(inode->i_sb->s_iflags)) {
+ kuid = inode->i_uid;
+ } else {
+ kuid = make_kuid(current_user_ns(),
+ from_kfsuid(current_user_ns(), inode->i_uid));
+ }
+
+ if (likely(uid_eq(current_fsuid(), kuid)))
mode >>= 6;
else {&& (mode & S_IRWXG)) {
>
> Also, e.g. __ptrace_may_access() uses cred->fsuid for a comparison
> with another task's real/effective/saved UID.
Right, you even introduced this check in 2015 iirc.
Both of your points make me think that it'd be easiest to introduce
cred->{kfsuid,kfsgid} and whenever an access decision on a
is_userns_visible() filesystem has to be made those will be used. This avoids
having to do on-the fly translations and ptrace_may_access() can just grow a
flag indicating what fscreds it's supposed to look at?
>
> [...]
> > # Demos
> > [1]: Create a container with different id and fsid mappings.
> > https://asciinema.org/a/300233
> > [2]: Create a container with id mappings but without fsid mappings.
> > https://asciinema.org/a/300234
> > [3]: Share storage between multiple containers with non-overlapping id
> > mappings.
> > https://asciinema.org/a/300235
>
> (I really dislike this asciinema thing; if you want to quickly glance
> through the output instead of reading at the same speed as it was
> typed, a simple pastebin works much better unless you absolutely have
> to show things that use stuff like ncurses UI.)
Hmkay, I went through the trouble of converting the asciinema output to
basic shell for all tree demos. :) I made them available as github gists.
So:
demo1: https://gist.github.com/brauner/8e1117720b3f9fab22e44c17f12184bf
demo2: https://gist.github.com/brauner/41a36026a9a1496af0095dce1545548e
demo3: https://gist.github.com/brauner/4586d6bc680a018bc8e1dd114a45592a
^ permalink raw reply related
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Alexey Gladkov @ 2020-02-12 14:49 UTC (permalink / raw)
To: Eric W. Biederman
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <87v9odlxbr.fsf@x220.int.ebiederm.org>
On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote:
> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
>
> > This allows to flush dcache entries of a task on multiple procfs mounts
> > per pid namespace.
> >
> > The RCU lock is used because the number of reads at the task exit time
> > is much larger than the number of procfs mounts.
>
> A couple of quick comments.
>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> > ---
> > fs/proc/base.c | 20 +++++++++++++++-----
> > fs/proc/root.c | 27 ++++++++++++++++++++++++++-
> > include/linux/pid_namespace.h | 2 ++
> > include/linux/proc_fs.h | 2 ++
> > 4 files changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 4ccb280a3e79..24b7c620ded3 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
> > .permission = proc_pid_permission,
> > };
> >
> > -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> > +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
> Perhaps just rename things like:
> > +static void proc_flush_task_root(struct dentry *root, pid_t pid, pid_t tgid)
> > {
>
> I don't think the mnt_ prefix conveys any information, and it certainly
> makes everything longer and more cumbersome.
>
> > struct dentry *dentry, *leader, *dir;
> > char buf[10 + 1];
> > @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> > name.name = buf;
> > name.len = snprintf(buf, sizeof(buf), "%u", pid);
> > /* no ->d_hash() rejects on procfs */
> > - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
> > + dentry = d_hash_and_lookup(mnt_root, &name);
> > if (dentry) {
> > d_invalidate(dentry);
> > dput(dentry);
> > @@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> >
> > name.name = buf;
> > name.len = snprintf(buf, sizeof(buf), "%u", tgid);
> > - leader = d_hash_and_lookup(mnt->mnt_root, &name);
> > + leader = d_hash_and_lookup(mnt_root, &name);
> > if (!leader)
> > goto out;
> >
> > @@ -3208,14 +3208,24 @@ void proc_flush_task(struct task_struct *task)
> > int i;
> > struct pid *pid, *tgid;
> > struct upid *upid;
> > + struct dentry *mnt_root;
> > + struct proc_fs_info *fs_info;
> >
> > pid = task_pid(task);
> > tgid = task_tgid(task);
> >
> > for (i = 0; i <= pid->level; i++) {
> > upid = &pid->numbers[i];
> > - proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
> > - tgid->numbers[i].nr);
> > +
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
> > + mnt_root = fs_info->m_super->s_root;
> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
> > + }
> > + rcu_read_unlock();
> > +
> > + mnt_root = upid->ns->proc_mnt->mnt_root;
> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
>
> I don't think this following of proc_mnt is needed. It certainly
> shouldn't be. The loop through all of the super blocks should be
> enough.
Yes, thanks!
> Once this change goes through. UML can be given it's own dedicated
> proc_mnt for the initial pid namespace, and proc_mnt can be removed
> entirely.
After you deleted the old sysctl syscall we could probably do it.
> Unless something has changed recently UML is the only other user of
> pid_ns->proc_mnt. That proc_mnt really only exists to make the loop in
> proc_flush_task easy to write.
Now I think, is there any way to get rid of proc_mounts or even
proc_flush_task somehow.
> It also probably makes sense to take the rcu_read_lock() over
> that entire for loop.
Al Viro pointed out to me that I cannot use rcu locks here :(
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v8 08/11] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option
From: Alexey Gladkov @ 2020-02-12 14:34 UTC (permalink / raw)
To: Jordan Glover
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Eric W . Biederman,
Greg Kroah-Hartman, Ingo Molnar, J . Bruce Fields, Jeff Layton,
Jonathan Corbet, Kees Cook, Linus Torvalds, Oleg Nesterov,
Solar Designer
In-Reply-To: <aBJUaM4BeffJa3vj1p1rUZRN60LVv39CTN9ETLC-swk2b6CvAW8BbP6QbxK5zBGwSYOEiRgjE-auqdRo-pYXxhwuJ_h5rbZ9uyeFqLcLSJQ=@protonmail.ch>
On Mon, Feb 10, 2020 at 04:29:31PM +0000, Jordan Glover wrote:
> On Monday, February 10, 2020 3:05 PM, Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> > If "hidepid=4" mount option is set then do not instantiate pids that
> > we can not ptrace. "hidepid=4" means that procfs should only contain
> > pids that the caller can ptrace.
> >
> > Cc: Kees Cook keescook@chromium.org
> > Cc: Andy Lutomirski luto@kernel.org
> > Signed-off-by: Djalal Harouni tixxdz@gmail.com
> > Signed-off-by: Alexey Gladkov gladkov.alexey@gmail.com
> >
> > fs/proc/base.c | 15 +++++++++++++++
> > fs/proc/root.c | 14 +++++++++++---
> > include/linux/proc_fs.h | 1 +
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 24b7c620ded3..49937d54e745 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -699,6 +699,14 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
> > struct task_struct *task,
> > int hide_pid_min)
> > {
> >
> > - /*
> > - - If 'hidpid' mount option is set force a ptrace check,
> > - - we indicate that we are using a filesystem syscall
> > - - by passing PTRACE_MODE_READ_FSCREDS
> > - */
> > - if (proc_fs_hide_pid(fs_info) == HIDEPID_NOT_PTRACABLE)
> > - return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> >
> >
> > - if (proc_fs_hide_pid(fs_info) < hide_pid_min)
> > return true;
> > if (in_group_p(proc_fs_pid_gid(fs_info)))
> > @@ -3271,7 +3279,14 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
> > if (!task)
> > goto out;
> >
> > - /* Limit procfs to only ptracable tasks */
> > - if (proc_fs_hide_pid(fs_info) == HIDEPID_NOT_PTRACABLE) {
> > - if (!has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS))
> >
> >
> > - goto out_put_task;
> >
> >
> > - }
> > - result = proc_pid_instantiate(dentry, task, NULL);
> > +out_put_task:
> > put_task_struct(task);
> > out:
> > return result;
> > diff --git a/fs/proc/root.c b/fs/proc/root.c
> > index e2bb015da1a8..5e27bb31f125 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -52,6 +52,15 @@ static const struct fs_parameter_description proc_fs_parameters = {
> > .specs = proc_param_specs,
> > };
> >
> > +static inline int
> > +valid_hidepid(unsigned int value)
> > +{
> >
> > - return (value == HIDEPID_OFF ||
> > - value == HIDEPID_NO_ACCESS ||
> >
> >
> > - value == HIDEPID_INVISIBLE ||
> >
> >
> > - value == HIDEPID_NOT_PTRACABLE);
> >
> >
> >
> > +}
> > +
> > static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > {
> > struct proc_fs_context *ctx = fc->fs_private;
> > @@ -68,10 +77,9 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > break;
> >
> > case Opt_hidepid:
> >
> > - if (!valid_hidepid(result.uint_32))
> >
> >
> > - return invalf(fc, "proc: unknown value of hidepid.\\n");
> > ctx->hidepid = result.uint_32;
> >
> >
> >
> > - if (ctx->hidepid < HIDEPID_OFF ||
> >
> >
> > - ctx->hidepid > HIDEPID_INVISIBLE)
> >
> >
> > - return invalf(fc, "proc: hidepid value must be between 0 and 2.\\n");
> > break;
> >
> >
> >
> > default:
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index f307940f8311..6822548405a7 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -17,6 +17,7 @@ enum {
> > HIDEPID_OFF = 0,
> > HIDEPID_NO_ACCESS = 1,
> > HIDEPID_INVISIBLE = 2,
> >
> > - HIDEPID_NOT_PTRACABLE = 4, /* Limit pids to only ptracable pids */
>
> Is there a reason new option is "4" instead of "3"? The order 1..2..4 may be
> confusing for people.
This is just mask. For now hidepid values are mutually exclusive, but
since it moved to uapi, I thought it would be good if there was an
opportunity to combine values.
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Alexey Gladkov @ 2020-02-12 14:26 UTC (permalink / raw)
To: Al Viro
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <20200211224553.GK23230@ZenIV.linux.org.uk>
On Tue, Feb 11, 2020 at 10:45:53PM +0000, Al Viro wrote:
> On Mon, Feb 10, 2020 at 04:05:15PM +0100, Alexey Gladkov wrote:
> > This allows to flush dcache entries of a task on multiple procfs mounts
> > per pid namespace.
> >
> > The RCU lock is used because the number of reads at the task exit time
> > is much larger than the number of procfs mounts.
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> > ---
> > fs/proc/base.c | 20 +++++++++++++++-----
> > fs/proc/root.c | 27 ++++++++++++++++++++++++++-
> > include/linux/pid_namespace.h | 2 ++
> > include/linux/proc_fs.h | 2 ++
> > 4 files changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 4ccb280a3e79..24b7c620ded3 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
> > .permission = proc_pid_permission,
> > };
> >
> > -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> > +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
> > {
> > struct dentry *dentry, *leader, *dir;
> > char buf[10 + 1];
> > @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> > name.name = buf;
> > name.len = snprintf(buf, sizeof(buf), "%u", pid);
> > /* no ->d_hash() rejects on procfs */
> > - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
> > + dentry = d_hash_and_lookup(mnt_root, &name);
> > if (dentry) {
> > d_invalidate(dentry);
> ... which can block
> > dput(dentry);
> ... and so can this
>
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
> > + mnt_root = fs_info->m_super->s_root;
> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
>
> ... making that more than slightly unsafe.
I see. So, I can't use rcu locks here as well as spinlocks.
Thanks!
--
Rgrds, legion
^ permalink raw reply
* Re: [RFC PATCH 0/2] ima: uncompressed module appraisal support
From: Nayna @ 2020-02-12 14:04 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, dmitry.kasatkin, jmorris, serge, dhowells, geert,
gregkh, nayna, tglx, bauerman, mpe, linux-integrity,
linux-security-module, linux-kernel, Roberto Sassu
In-Reply-To: <0842A02F-3166-4E29-9CC5-9E4C5057E270@oracle.com>
On 2/11/20 12:33 PM, Eric Snowberg wrote:
>> On Feb 10, 2020, at 1:33 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> On Mon, 2020-02-10 at 12:24 -0700, Eric Snowberg wrote:
>>>> On Feb 10, 2020, at 10:09 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> Ok, understood, “modsig” refers to strictly kernel module appended signatures
>>>>> without regard to the keyring that verifies it. Since there are inconsistencies
>>>>> here, would you consider something like my first patch? It will verify an
>>>>> uncompressed kernel module containing an appended signature when the public key
>>>>> is contained within the kernel keyring instead of the ima keyring. Why force a
>>>>> person to add the same keys into the ima keyring for validation? Especially when
>>>>> the kernel keyring is now used to verify appended signatures in the compressed
>>>>> modules.
>>>> Different use case scenarios have different requirements. Suppose for
>>>> example that the group creating the kernel image is not the same as
>>>> using it. The group using the kernel image could sign all files,
>>>> including kernel modules (imasig), with their own private key. Only
>>>> files that they signed would be permitted. Your proposal would break
>>>> the current expectations, allowing kernel modules signed by someone
>>>> else to be loaded.
>>>>
>>> All the end user needs to do is compress any module created by the group that built
>>> the original kernel image to work around the scenario above. Then the appended
>>> signature in the compressed module will be verified by the kernel keyring. Does
>>> this mean there is a security problem that should be fixed, if this is a concern?
>> Again, the issue isn't compressed/uncompressed kernel modules, but the
>> syscall used to load the kernel module. IMA can prevent using the the
>> init_module syscall. Refer to the ima_load_data() LOADING_MODULE
>> case.
> Within the ima_load_data() LOADING_MODULE case, to prevent IMA from using
> the init_module syscall, is_module_sig_enforced() must return false. Currently
> when is_module_sig_enforced() returns true, the kernel keyring is always used
> for verification.
>
> What if I change this part of my patch from
>
> + if (rc && func == MODULE_CHECK)
>
> to
>
> + sig_enforce = is_module_sig_enforced();
> + if (sig_enforce && rc && func == MODULE_CHECK)
>
> Now when the init_module syscall is available, finit_module syscall will use
> both the ima keyring and kernel keyring for verification. When the
> init_module syscall is blocked from use, the finit_module syscall will only use
> the ima keyring for validation. I believe this would satisfy both your use
> case and mine.
>
There are two syscalls - init_module, finit_module - and two signature
verification methods. The problem you are trying to address is the
finit_module syscall, using both signature verification methods. Why
enable both signature verification methods ?
If the modules are signed with build time generated keys, use module
signature verification. If the keys are generated by you and can be
added to .ima keyring, use IMA policy. The IMA_ARCH_POLICY defines
secure boot policies at runtime, based on the secure boot state of the
system. The arch-specific policies defines IMA policy only if the module
verification is not enabled.
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-12 13:53 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <dd6a1382-7b2f-a6e6-a1ac-009566d7f556@tycho.nsa.gov>
On 12.02.2020 16:32, Stephen Smalley wrote:
> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>> Hi Stephen,
>>
>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>
>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>
>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>
>> <SNIP>
>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>
>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>
>>>> So far so good, I suggest using the simplest version for v6:
>>>>
>>>> static inline bool perfmon_capable(void)
>>>> {
>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>> }
>>>>
>>>> It keeps the implementation simple and readable. The implementation is more
>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>> privileged process.
>>>>
>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>> based approach to use perf_event_open system call.
>>>
>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>
>> I am trying to reproduce this double logging with CAP_PERFMON.
>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>> When running perf stat -a I am observing this AVC audit messages:
>>
>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>
>> However there is no capability related messages around. I suppose my refpolicy should
>> be modified somehow to observe capability related AVCs.
>>
>> Could you please comment or clarify on how to enable caps related AVCs in order
>> to test the concerned logging.
>
> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
Yes, I already have it like this:
common cap2
{
<------>mac_override<--># unused by SELinux
<------>mac_admin
<------>syslog
<------>wake_alarm
<------>block_suspend
<------>audit_read
<------>perfmon
}
dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
~Alexey
>
>
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-02-12 13:32 UTC (permalink / raw)
To: Alexey Budankov
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <2e38c33d-f085-1320-8cc2-45f74b6ad86d@linux.intel.com>
On 2/12/20 3:53 AM, Alexey Budankov wrote:
> Hi Stephen,
>
> On 22.01.2020 17:07, Stephen Smalley wrote:
>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>
>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>
>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>
> <SNIP>
>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>
>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>
>>> So far so good, I suggest using the simplest version for v6:
>>>
>>> static inline bool perfmon_capable(void)
>>> {
>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>> }
>>>
>>> It keeps the implementation simple and readable. The implementation is more
>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>> privileged process.
>>>
>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>> based approach to use perf_event_open system call.
>>
>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>
> I am trying to reproduce this double logging with CAP_PERFMON.
> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
> When running perf stat -a I am observing this AVC audit messages:
>
> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>
> However there is no capability related messages around. I suppose my refpolicy should
> be modified somehow to observe capability related AVCs.
>
> Could you please comment or clarify on how to enable caps related AVCs in order
> to test the concerned logging.
The new perfmon permission has to be defined in your policy; you'll have
a message in dmesg about "Permission perfmon in class capability2 not
defined in policy.". You can either add it to the common cap2
definition in refpolicy/policy/flask/access_vectors and rebuild your
policy or extract your base module as CIL, add it there, and insert the
updated module.
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Daniel Borkmann @ 2020-02-12 13:27 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jann Horn, KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Matthew Garrett,
Christian Brauner, Mickaël Salaün, Florent Revest,
Brendan Jackman, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Kernel Team
In-Reply-To: <20200212024542.gdsafhvqykucdp4h@ast-mbp>
On 2/12/20 3:45 AM, Alexei Starovoitov wrote:
> On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
>>
>> Another approach could be to have a special nop inside call_int_hook()
>> macro which would then get patched to avoid these situations. Somewhat
>> similar like static keys where it could be defined anywhere in text but
>> with updating of call_int_hook()'s RC for the verdict.
>
> Sounds nice in theory. I couldn't quite picture how that would look
> in the code, so I hacked:
> diff --git a/security/security.c b/security/security.c
> index 565bc9b67276..ce4bc1e5e26c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,7 @@
> #include <linux/string.h>
> #include <linux/msg.h>
> #include <net/flow.h>
> +#include <linux/jump_label.h>
>
> #define MAX_LSM_EVM_XATTR 2
>
> @@ -678,12 +679,26 @@ static void __init lsm_early_task(struct task_struct *task)
> * This is a hook that returns a value.
> */
>
> +#define LSM_HOOK_NAME(FUNC) \
> + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##FUNC);
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK_NAME
> +__diag_push();
> +__diag_ignore(GCC, 8, "-Wstrict-prototypes", "");
> +#define LSM_HOOK_NAME(FUNC) \
> + int bpf_lsm_call_##FUNC() {return 0;}
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK_NAME
> +__diag_pop();
> +
> #define call_void_hook(FUNC, ...) \
> do { \
> struct security_hook_list *P; \
> \
> hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> P->hook.FUNC(__VA_ARGS__); \
> + if (static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> + (void)bpf_lsm_call_##FUNC(__VA_ARGS__); \
> } while (0)
>
> #define call_int_hook(FUNC, IRC, ...) ({ \
> @@ -696,6 +711,8 @@ static void __init lsm_early_task(struct task_struct *task)
> if (RC != 0) \
> break; \
> } \
> + if (RC == IRC && static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> + RC = bpf_lsm_call_##FUNC(__VA_ARGS__); \
Nit: the `RC == IRC` test could be moved behind the static_branch_unlikely() so
that it would be bypassed when not enabled.
> } while (0); \
> RC; \
> })
>
> The assembly looks good from correctness and performance points.
> union security_list_options can be split into lsm_hook_names.h too
> to avoid __diag_ignore. Is that what you have in mind?
> I don't see how one can improve call_int_hook() macro without
> full refactoring of linux/lsm_hooks.h
> imo static_key doesn't have to be there in the first set. We can add this
> optimization later.
Yes, like the above diff looks good, and then we'd dynamically attach the program
at bpf_lsm_call_##FUNC()'s fexit hook for a direct jump, so all the security_blah()
internals could stay as-is which then might also address Jann's concerns wrt
concrete annotation as well as potential locking changes inside security_blah().
Agree that patching out via static key could be optional but since you were talking
about avoiding indirect jumps..
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-12 8:53 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <d7213569-9578-7201-6106-f5ebc95bd6be@tycho.nsa.gov>
Hi Stephen,
On 22.01.2020 17:07, Stephen Smalley wrote:
> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>
>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>
>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>
>>>>>
>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>
<SNIP>
>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>
>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>
>> So far so good, I suggest using the simplest version for v6:
>>
>> static inline bool perfmon_capable(void)
>> {
>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>> }
>>
>> It keeps the implementation simple and readable. The implementation is more
>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>> privileged process.
>>
>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>> based approach to use perf_event_open system call.
>
> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
I am trying to reproduce this double logging with CAP_PERFMON.
I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
When running perf stat -a I am observing this AVC audit messages:
type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
However there is no capability related messages around. I suppose my refpolicy should
be modified somehow to observe capability related AVCs.
Could you please comment or clarify on how to enable caps related AVCs in order
to test the concerned logging.
Thanks,
Alexey
---
[1] https://github.com/SELinuxProject/refpolicy.git
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Kees Cook @ 2020-02-12 7:50 UTC (permalink / raw)
To: Daniel Colascione
Cc: timmurray, nosh, nnk, lokeshgidra, linux-kernel, linux-api,
selinux, Andrea Arcangeli, Mike Rapoport, Peter Xu, Jann Horn,
linux-security-module
In-Reply-To: <20200211225547.235083-1-dancol@google.com>
Hi!
Firstly, thanks for working on this! It's been on my TODO list for a
while. :)
Casey already recommended including the LSM list to CC (since this is a
new LSM -- there are many LSMs). Additionally, the series should
probably be sent _to_ the userfaultfd maintainers:
Andrea Arcangeli <aarcange@redhat.com>
Mike Rapoport <rppt@linux.ibm.com>
and I'd also CC a couple other people that have done recent work:
Peter Xu <peterx@redhat.com>
Jann Horn <jannh@google.com>
More notes below...
On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> Userfaultfd in unprivileged contexts could be potentially very
> useful. We'd like to harden userfaultfd to make such unprivileged use
> less risky. This patch series allows SELinux to manage userfaultfd
> file descriptors and allows administrators to limit userfaultfd to
> servicing user-mode faults, increasing the difficulty of using
> userfaultfd in exploit chains invoking delaying kernel faults.
I actually think these are two very different goals and likely the
series could be split into two for them. One is LSM hooking of
userfaultfd and the SELinux attachment, and the other is the user-mode
fault restrictions. And they would likely go via separate trees (LSM
through James's LSM tree, and probably akpm's -mm tree for the sysctl).
> A new anon_inodes interface allows callers to opt into SELinux
> management of anonymous file objects. In this mode, anon_inodes
> creates new ephemeral inodes for anonymous file objects instead of
> reusing a singleton dummy inode. A new LSM hook gives security modules
> an opportunity to configure and veto these ephemeral inodes.
>
> Existing anon_inodes users must opt into the new functionality.
>
> Daniel Colascione (6):
> Add a new flags-accepting interface for anonymous inodes
> Add a concept of a "secure" anonymous file
> Teach SELinux about a new userfaultfd class
> Wire UFFD up to SELinux
The above is the first "series"... I don't have much opinion about it,
though I do like the idea of making userfaultfd visible to the LSM.
> Let userfaultfd opt out of handling kernel-mode faults
> Add a new sysctl for limiting userfaultfd to user mode faults
Now this I'm very interested in. Can you go into more detail about two
things:
- What is the threat being solved? (I understand the threat, but detailing
it in the commit log is important for people who don't know it. Existing
commit cefdca0a86be517bc390fc4541e3674b8e7803b0 gets into some of the
details already, but I'd like to see reference to external sources like
https://duasynt.com/blog/linux-kernel-heap-spray)
- Why is this needed in addition to the existing vm.unprivileged_userfaultfd
sysctl? (And should this maybe just be another setting for that
sysctl, like "2"?)
As to the mechanics of the change, I'm not sure I like the idea of adding
a UAPI flag for this. Why not just retain the permission check done at
open() and if kernelmode faults aren't allowed, ignore them? This would
require no changes to existing programs and gains the desired defense.
(And, I think, the sysctl value could be bumped to "2" as that's a
better default state -- does qemu actually need kernelmode traps?)
Thanks again for the patches!
-Kees
>
> Documentation/admin-guide/sysctl/vm.rst | 13 ++++
> fs/anon_inodes.c | 89 +++++++++++++++++--------
> fs/userfaultfd.c | 29 ++++++--
> include/linux/anon_inodes.h | 27 ++++++--
> include/linux/lsm_hooks.h | 8 +++
> include/linux/security.h | 2 +
> include/linux/userfaultfd_k.h | 3 +
> include/uapi/linux/userfaultfd.h | 9 +++
> kernel/sysctl.c | 9 +++
> security/security.c | 8 +++
> security/selinux/hooks.c | 68 +++++++++++++++++++
> security/selinux/include/classmap.h | 2 +
> 12 files changed, 229 insertions(+), 38 deletions(-)
>
> --
> 2.25.0.225.g125e21ebc7-goog
>
--
Kees Cook
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Alexei Starovoitov @ 2020-02-12 2:45 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jann Horn, KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Matthew Garrett,
Christian Brauner, Mickaël Salaün, Florent Revest,
Brendan Jackman, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Kernel Team
In-Reply-To: <1cd10710-a81b-8f9b-696d-aa40b0a67225@iogearbox.net>
On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
>
> Another approach could be to have a special nop inside call_int_hook()
> macro which would then get patched to avoid these situations. Somewhat
> similar like static keys where it could be defined anywhere in text but
> with updating of call_int_hook()'s RC for the verdict.
Sounds nice in theory. I couldn't quite picture how that would look
in the code, so I hacked:
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..ce4bc1e5e26c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -28,6 +28,7 @@
#include <linux/string.h>
#include <linux/msg.h>
#include <net/flow.h>
+#include <linux/jump_label.h>
#define MAX_LSM_EVM_XATTR 2
@@ -678,12 +679,26 @@ static void __init lsm_early_task(struct task_struct *task)
* This is a hook that returns a value.
*/
+#define LSM_HOOK_NAME(FUNC) \
+ DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##FUNC);
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK_NAME
+__diag_push();
+__diag_ignore(GCC, 8, "-Wstrict-prototypes", "");
+#define LSM_HOOK_NAME(FUNC) \
+ int bpf_lsm_call_##FUNC() {return 0;}
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK_NAME
+__diag_pop();
+
#define call_void_hook(FUNC, ...) \
do { \
struct security_hook_list *P; \
\
hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
P->hook.FUNC(__VA_ARGS__); \
+ if (static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
+ (void)bpf_lsm_call_##FUNC(__VA_ARGS__); \
} while (0)
#define call_int_hook(FUNC, IRC, ...) ({ \
@@ -696,6 +711,8 @@ static void __init lsm_early_task(struct task_struct *task)
if (RC != 0) \
break; \
} \
+ if (RC == IRC && static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
+ RC = bpf_lsm_call_##FUNC(__VA_ARGS__); \
} while (0); \
RC; \
})
The assembly looks good from correctness and performance points.
union security_list_options can be split into lsm_hook_names.h too
to avoid __diag_ignore. Is that what you have in mind?
I don't see how one can improve call_int_hook() macro without
full refactoring of linux/lsm_hooks.h
imo static_key doesn't have to be there in the first set. We can add this
optimization later.
^ permalink raw reply related
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Daniel Borkmann @ 2020-02-12 0:09 UTC (permalink / raw)
To: Alexei Starovoitov, Jann Horn
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Matthew Garrett,
Christian Brauner, Mickaël Salaün, Florent Revest,
Brendan Jackman, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Kernel Team
In-Reply-To: <CAADnVQLsiWgSBXbuxmpkC9TS8d1aQRw2zDHG8J6E=kfcRoXtKQ@mail.gmail.com>
On 2/12/20 12:26 AM, Alexei Starovoitov wrote:
> On Tue, Feb 11, 2020 at 1:38 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Tue, Feb 11, 2020 at 09:33:49PM +0100, Jann Horn wrote:
>>>>
>>>> Got it. Then let's whitelist them ?
>>>> All error injection points are marked with ALLOW_ERROR_INJECTION().
>>>> We can do something similar here, but let's do it via BTF and avoid
>>>> abusing yet another elf section for this mark.
>>>> I think BTF_TYPE_EMIT() should work. Just need to pick explicit enough
>>>> name and extensive comment about what is going on.
>>>
>>> Sounds reasonable to me. :)
>>
>> awesome :)
>
> Looks like the kernel already provides this whitelisting.
> $ bpftool btf dump file /sys/kernel/btf/vmlinux |grep FUNC|grep '\<security_'
> gives the list of all LSM hooks that lsm-bpf will be able to attach to.
> There are two exceptions there security_add_hooks() and security_init().
> Both are '__init'. Too late for lsm-bpf to touch.
> So filtering BTF funcs by 'security_' prefix will be enough.
> It should be documented though.
> The number of attachable funcs depends on kconfig which is
> a nice property and further strengthen the point that
> lsm-bpf is very much kernel specific.
> We probably should blacklist security_bpf*() hooks though.
One thing that is not quite clear to me wrt the fexit approach; assuming
we'd whitelist something like security_inode_link():
int security_inode_link(struct dentry *old_dentry, struct inode *dir,
struct dentry *new_dentry)
{
if (unlikely(IS_PRIVATE(d_backing_inode(old_dentry))))
return 0;
return call_int_hook(inode_link, 0, old_dentry, dir, new_dentry);
}
Would this then mean the BPF prog needs to reimplement above check by
probing old_dentry->d_inode to later ensure its verdict stays 0 there
too, or that such extra code is to be moved to call-sites instead? If
former, what about more complex logic?
Another approach could be to have a special nop inside call_int_hook()
macro which would then get patched to avoid these situations. Somewhat
similar like static keys where it could be defined anywhere in text but
with updating of call_int_hook()'s RC for the verdict.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Daniel Colascione @ 2020-02-11 23:27 UTC (permalink / raw)
To: Casey Schaufler
Cc: Tim Murray, Nosh Minwalla, Nick Kralevich, Lokesh Gidra,
linux-kernel, Linux API, selinux, LSM List
In-Reply-To: <9ae20f6e-c5c0-4fd7-5b61-77218d19480b@schaufler-ca.com>
On Tue, Feb 11, 2020 at 3:13 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/11/2020 2:55 PM, Daniel Colascione wrote:
> > Userfaultfd in unprivileged contexts could be potentially very
> > useful. We'd like to harden userfaultfd to make such unprivileged use
> > less risky. This patch series allows SELinux to manage userfaultfd
> > file descriptors and allows administrators to limit userfaultfd to
> > servicing user-mode faults, increasing the difficulty of using
> > userfaultfd in exploit chains invoking delaying kernel faults.
> >
> > A new anon_inodes interface allows callers to opt into SELinux
> > management of anonymous file objects. In this mode, anon_inodes
> > creates new ephemeral inodes for anonymous file objects instead of
> > reusing a singleton dummy inode. A new LSM hook gives security modules
> > an opportunity to configure and veto these ephemeral inodes.
> >
> > Existing anon_inodes users must opt into the new functionality.
> >
> > Daniel Colascione (6):
> > Add a new flags-accepting interface for anonymous inodes
> > Add a concept of a "secure" anonymous file
> > Teach SELinux about a new userfaultfd class
> > Wire UFFD up to SELinux
> > Let userfaultfd opt out of handling kernel-mode faults
> > Add a new sysctl for limiting userfaultfd to user mode faults
>
> This must be posted to the linux Security Module list
> <linux-security-module@vger.kernel.org>
Added. I thought selinux@ was sufficient.
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Alexei Starovoitov @ 2020-02-11 23:26 UTC (permalink / raw)
To: Jann Horn
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <20200211213819.j4ltrjjkuywihpnv@ast-mbp>
On Tue, Feb 11, 2020 at 1:38 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 11, 2020 at 09:33:49PM +0100, Jann Horn wrote:
> > >
> > > Got it. Then let's whitelist them ?
> > > All error injection points are marked with ALLOW_ERROR_INJECTION().
> > > We can do something similar here, but let's do it via BTF and avoid
> > > abusing yet another elf section for this mark.
> > > I think BTF_TYPE_EMIT() should work. Just need to pick explicit enough
> > > name and extensive comment about what is going on.
> >
> > Sounds reasonable to me. :)
>
> awesome :)
Looks like the kernel already provides this whitelisting.
$ bpftool btf dump file /sys/kernel/btf/vmlinux |grep FUNC|grep '\<security_'
gives the list of all LSM hooks that lsm-bpf will be able to attach to.
There are two exceptions there security_add_hooks() and security_init().
Both are '__init'. Too late for lsm-bpf to touch.
So filtering BTF funcs by 'security_' prefix will be enough.
It should be documented though.
The number of attachable funcs depends on kconfig which is
a nice property and further strengthen the point that
lsm-bpf is very much kernel specific.
We probably should blacklist security_bpf*() hooks though.
Otherwise inception fans will have a field day.
Disallowing bpf with bpf :)
^ permalink raw reply
* Re: [PATCH 1/2] crypto: rename sm3-256 to sm3 in hash_algo_name
From: Mimi Zohar @ 2020-02-11 23:23 UTC (permalink / raw)
To: Tianjia Zhang, herbert, davem, dmitry.kasatkin, jmorris, serge,
ebiggers
Cc: linux-crypto, linux-integrity, linux-security-module,
linux-kernel
In-Reply-To: <1581457759.5125.18.camel@linux.ibm.com>
On Tue, 2020-02-11 at 16:49 -0500, Mimi Zohar wrote:
> On Mon, 2020-02-10 at 20:44 +0800, Tianjia Zhang wrote:
> > The name sm3-256 is defined in hash_algo_name in hash_info, but the
> > algorithm name implemented in sm3_generic.c is sm3, which will cause
> > the sm3-256 algorithm to be not found in some application scenarios of
> > the hash algorithm, and an ENOENT error will occur. For example,
> > IMA, keys, and other subsystems that reference hash_algo_name all use
> > the hash algorithm of sm3.
> >
> > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
>
> The "hash_map" needs to be updated to reflect this change.
>
> static struct tpm2_hash tpm2_hash_map[] = {
> {HASH_ALGO_SHA1, TPM_ALG_SHA1},
> {HASH_ALGO_SHA256, TPM_ALG_SHA256},
> {HASH_ALGO_SHA384, TPM_ALG_SHA384},
> {HASH_ALGO_SHA512, TPM_ALG_SHA512},
> {HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
> };
Never mind, the enum name "HASH_ALGO_SM3_256" didn't change. Just the
string changed.
Mimi
^ 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