* Re: [PATCH] ima: Add a space after printing a LSM rule for readability
From: Clay Chang @ 2020-01-05 1:12 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-kernel, linux-security-module, linux-integrity,
dmitry.kasatkin, jmorris, serge
In-Reply-To: <1578071487.5152.13.camel@linux.ibm.com>
On Fri, Jan 03, 2020 at 12:11:27PM -0500, Mimi Zohar wrote:
> On Fri, 2020-01-03 at 15:51 +0800, clayc@hpe.com wrote:
> > From: Clay Chang <clayc@hpe.com>
>
> Normally this "From" line is only seen when the sender isn't the patch
> author. Any ideas what happened?
>
Hi Mimi,
Apparently I should not use "--from" in git-send-email command.
> >
> > When reading ima_policy from securityfs, there is a missing
> > space between output string of LSM rules.
> >
> > Signed-off-by: Clay Chang <clayc@hpe.com>
>
> Good catch! IMA policy rules based on LSM labels are used to
> constrain which files are in policy. Normally a single LSM label is
> enough (e.g. dont_measure obj_type=auditd_log_t). Could you include
> in this patch description a use case where multiple LSM labels are
> needed?
>
Apology for not expressed my intention clearly. The intention of this
patch is to add a space after printing LSM rules (if any) and the
remaining rules.
Currently, if I have a policy, for example:
appraise func=BPRM_CHECK obj_type=shell_exec_t appraise_type=imasig
The read back result is:
appraise func=BPRM_CHECK obj_type=shell_exec_tappraise_type=imasig
which is not correct.
I do not have a case for multiple LSM labels, but if there is one
such case, this patch will also apply.
I will post a v2 patch with tuned description.
Thanks,
Clay
^ permalink raw reply
* Re: INFO: rcu detected stall in sys_sendfile64
From: Tetsuo Handa @ 2020-01-04 11:09 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: syzbot, syzkaller-bugs, Ingo Molnar, Peter Zijlstra, James Morris,
LKML, linux-security-module, Serge E. Hallyn
In-Reply-To: <CACT4Y+YqWgZZFXdX2A2jVYEdHfY9ywGMgRRP5W4Uqdu__rA63g@mail.gmail.com>
On 2018/12/20 3:42, Dmitry Vyukov wrote:
> On Wed, Dec 19, 2018 at 11:13 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> On 2018/12/19 18:27, syzbot wrote:
>>> HEAD commit: ddfbab46539f Merge tag 'scsi-fixes' of git://git.kernel.or..
>>> git tree: upstream
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=15b87fa3400000
>>> kernel config: https://syzkaller.appspot.com/x/.config?x=861a3573f4e78ba1
>>> dashboard link: https://syzkaller.appspot.com/bug?extid=bcad772bbc241b4c6147
>>> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13912ccd400000
>>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=145781db400000
>>
>> This is not a LSM problem, for the reproducer is calling
>> sched_setattr(SCHED_DEADLINE) with very large values.
>>
>> sched_setattr(0, {size=0, sched_policy=0x6 /* SCHED_??? */, sched_flags=0, sched_nice=0, sched_priority=0, sched_runtime=2251799813724439, sched_deadline=4611686018427453437, sched_period=0}, 0) = 0
>>
>> I think that this problem is nothing but an insane sched_setattr() parameter.
>>
>> #syz invalid
>
> Note there was another one with sched_setattr, which turned out to be
> some serious problem in kernel (sched_setattr should not cause CPU
> stall for 3 minutes):
> INFO: rcu detected stall in do_idle
> https://syzkaller.appspot.com/bug?extid=385468161961cee80c31
> https://groups.google.com/forum/#!msg/syzkaller-bugs/crrfvusGtwI/IoD_zus4BgAJ
>
> Maybe it another incarnation of the same bug, that one is still not fixed.
>
Can we let syzbot blacklist sched_setattr() for now? There are many stall reports
doing sched_setattr(SCHED_RR) which makes it difficult to find stall reports not
using sched_setattr().
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Andy Lutomirski @ 2020-01-04 0:49 UTC (permalink / raw)
To: KP Singh, Rick Edgecombe
Cc: linux-kernel, bpf, x86, linux-security-module, Kees Cook,
David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, Thomas Garnier, Florent Revest,
Brendan Jackman, Jann Horn, Matthew Garrett, Michael Halcrow
In-Reply-To: <20200103234725.22846-1-kpsingh@chromium.org>
> On Jan 4, 2020, at 8:47 AM, KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> The image for the BPF trampolines is allocated with
> bpf_jit_alloc_exe_page which marks this allocated page executable. This
> means that the allocated memory is W and X at the same time making it
> susceptible to WX based attacks.
>
> Since the allocated memory is shared between two trampolines (the
> current and the next), 2 pages must be allocated to adhere to W^X and
> the following sequence is obeyed where trampolines are modified:
Can we please do better rather than piling garbage on top of garbage?
>
> - Mark memory as non executable (set_memory_nx). While module_alloc for
> x86 allocates the memory as PAGE_KERNEL and not PAGE_KERNEL_EXEC, not
> all implementations of module_alloc do so
How about fixing this instead?
> - Mark the memory as read/write (set_memory_rw)
Probably harmless, but see above about fixing it.
> - Modify the trampoline
Seems reasonable. It’s worth noting that this whole approach is suboptimal: the “module” allocator should really be returning a list of pages to be written (not at the final address!) with the actual executable mapping to be materialized later, but that’s a bigger project that you’re welcome to ignore for now. (Concretely, it should produce a vmap address with backing pages but with the vmap alias either entirely unmapped or read-only. A subsequent healer would, all at once, make the direct map pages RO or not-present and make the vmap alias RX.)
> - Mark the memory as read-only (set_memory_ro)
> - Mark the memory as executable (set_memory_x)
No, thanks. There’s very little excuse for doing two IPI flushes when one would suffice.
As far as I know, all architectures can do this with a single flush without races x86 certainly can. The module freeing code gets this sequence right. Please reuse its mechanism or, if needed, export the relevant interfaces.
^ permalink raw reply
* Re: [PATCH bpf-next v1 12/13] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-01-04 0:09 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <CAEf4BzY4K-vgSFPjV=pn3quc5DT1+eGkJnZfSw4+b0fERzPVfw@mail.gmail.com>
On 23-Dez 22:49, Andrii Nakryiko wrote:
> On Fri, Dec 20, 2019 at 7:42 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > * Load a BPF program that audits mprotect calls
> > * Attach the program to the "file_mprotect" LSM hook
> > * Verify if the program is actually loading by reading
> > securityfs
> > * Initialize the perf events buffer and poll for audit events
> > * Do an mprotect on some memory allocated on the heap
> > * Verify if the audit event was received
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > MAINTAINERS | 2 +
> > .../bpf/prog_tests/lsm_mprotect_audit.c | 129 ++++++++++++++++++
> > .../selftests/bpf/progs/lsm_mprotect_audit.c | 58 ++++++++
> > 3 files changed, 189 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
> > create mode 100644 tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c
> >
>
> [...]
>
> > +/*
> > + * Define some of the structs used in the BPF program.
> > + * Only the field names and their sizes need to be the
> > + * same as the kernel type, the order is irrelevant.
> > + */
> > +struct mm_struct {
> > + unsigned long start_brk, brk, start_stack;
> > +};
> > +
> > +struct vm_area_struct {
> > + unsigned long start_brk, brk, start_stack;
> > + unsigned long vm_start, vm_end;
> > + struct mm_struct *vm_mm;
> > + unsigned long vm_flags;
> > +};
> > +
> > +BPF_TRACE_3("lsm/file_mprotect", mprotect_audit,
> > + struct vm_area_struct *, vma,
> > + unsigned long, reqprot, unsigned long, prot)
> > +{
> > + struct mprotect_audit_log audit_log = {};
> > + int is_heap = 0;
> > +
> > + __builtin_preserve_access_index(({
>
> you don't need __builtin_preserve_access_index, if you mark
> vm_area_struct and mm_struct with
> __attribute__((preserve_access_index)
Cool, updated!
>
> > + is_heap = (vma->vm_start >= vma->vm_mm->start_brk &&
> > + vma->vm_end <= vma->vm_mm->brk);
> > + }));
> > +
> > + audit_log.magic = MPROTECT_AUDIT_MAGIC;
> > + audit_log.is_heap = is_heap;
> > + bpf_lsm_event_output(&perf_buf_map, BPF_F_CURRENT_CPU, &audit_log,
> > + sizeof(audit_log));
>
> You test would be much simpler if you use global variables to pass
> data back to userspace, instead of using perf buffer.
>
> Also please see fentry_fexit.c test for example of using BPF skeleton
> to shorten and simpify userspace part of test.
Thanks for the skeleton work!
This makes using global variables easier and the tests are indeed much
simpler, I have updated it for the next revision.
One follow up question regarding global variables, let's say I have
the following global variable defined in the BPF program:
struct result_info {
__u32 count;
};
struct result_info result = {
.count = 0,
};
The defintion of result_info needs to be included before the .skel.h
as it's not automatically generated or maybe I am missing a
trick here?
For now, I have defined this in a header which gets included both in
the program and the test.
- KP
>
> > + return 0;
> > +}
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH bpf-next v1 05/13] tools/libbpf: Add support in libbpf for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-01-03 23:59 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <CAEf4BzYz6wswhr+byP_xabLoWyA2ah8P2a-STOgqXzuiNkHShw@mail.gmail.com>
On 23-Dez 16:07, Andrii Nakryiko wrote:
> On Fri, Dec 20, 2019 at 7:43 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > Update the libbpf library with functionality to load and
> > attach a program type BPF_PROG_TYPE_LSM, currently with
> > only one expected attach type BPF_LSM_MAC.
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > tools/lib/bpf/bpf.c | 2 +-
> > tools/lib/bpf/bpf.h | 6 +++++
> > tools/lib/bpf/libbpf.c | 44 +++++++++++++++++++++--------------
> > tools/lib/bpf/libbpf.h | 2 ++
> > tools/lib/bpf/libbpf.map | 6 +++++
> > tools/lib/bpf/libbpf_probes.c | 1 +
> > 6 files changed, 43 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 98596e15390f..9c6fb083f7de 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -228,7 +228,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > memset(&attr, 0, sizeof(attr));
> > attr.prog_type = load_attr->prog_type;
> > attr.expected_attach_type = load_attr->expected_attach_type;
> > - if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
> > + if (needs_btf_attach(attr.prog_type)) {
> > attr.attach_btf_id = load_attr->attach_btf_id;
> > attr.attach_prog_fd = load_attr->attach_prog_fd;
> > } else {
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 3c791fa8e68e..df2a00ff349f 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -177,6 +177,12 @@ LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
> > __u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
> > __u64 *probe_offset, __u64 *probe_addr);
> >
> > +static inline bool needs_btf_attach(enum bpf_prog_type prog_type)
> > +{
> > + return (prog_type == BPF_PROG_TYPE_TRACING ||
> > + prog_type == BPF_PROG_TYPE_LSM);
> > +}
> > +
>
> This doesn't have to be a public API, right? It also doesn't follow
> naming conventions of libbpf APIs. Let's just move it into
> libbpf_internal.h, given it's used in few files.
>
> Also, Martin's patches add STRUCT_OPS, which do need btf_attach, but
> don't set attach_prog_fd. So maybe something like
> libbpf_need_attach_prog_btf() for a name to be a bit more specific?
Updated for the next revision. Thanks!
>
>
> > #ifdef __cplusplus
> > } /* extern "C" */
> > #endif
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index b20f82e58989..b0b27d8e5a37 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -3738,7 +3738,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > load_attr.insns = insns;
> > load_attr.insns_cnt = insns_cnt;
> > load_attr.license = license;
> > - if (prog->type == BPF_PROG_TYPE_TRACING) {
> > + if (needs_btf_attach(prog->type)) {
> > load_attr.attach_prog_fd = prog->attach_prog_fd;
> > load_attr.attach_btf_id = prog->attach_btf_id;
> > } else {
> > @@ -3983,7 +3983,7 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> >
> > bpf_program__set_type(prog, prog_type);
> > bpf_program__set_expected_attach_type(prog, attach_type);
> > - if (prog_type == BPF_PROG_TYPE_TRACING) {
> > + if (needs_btf_attach(prog_type)) {
> > err = libbpf_find_attach_btf_id(prog->section_name,
> > attach_type,
> > attach_prog_fd);
> > @@ -4933,6 +4933,7 @@ bool bpf_program__is_##NAME(const struct bpf_program *prog) \
> > } \
> >
> > BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
> > +BPF_PROG_TYPE_FNS(lsm, BPF_PROG_TYPE_LSM);
> > BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
> > BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
> > BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
> > @@ -5009,6 +5010,8 @@ static const struct {
> > BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> > BPF_PROG_SEC("lwt_xmit", BPF_PROG_TYPE_LWT_XMIT),
> > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
> > + BPF_PROG_BTF("lsm/", BPF_PROG_TYPE_LSM,
> > + BPF_LSM_MAC),
>
> Is is supposed to be attachable same as BPF_PROG_TYPE_TRACING
> programs? If yes, please define auto-attaching function, similar to
> SEC_DEF("raw_tp") few lines below this one.
Nice! rebased and updated.
>
> > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB,
> > BPF_CGROUP_INET_INGRESS),
> > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
> > @@ -5119,32 +5122,39 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
> > return -ESRCH;
> > }
> >
> > -#define BTF_PREFIX "btf_trace_"
> > +static inline int __btf__typdef_with_prefix(struct btf *btf, const char *name,
>
> typo: typdef -> typedef
>
> But actually let's generalize it to pass BTF_KIND as another param, I
> think I have a need for this (we might want to do that for structs,
> not just typedef->func_proto).
> Following btf__find_by_name_kind() naming, it probably should be
> called btf__find_by_prefix_kind()?
Thanks! Good idea, updated. Should this be moved to btf.c?
>
> > + const char *prefix)
> > +{
> > +
> > + size_t prefix_len = strlen(prefix);
> > + char btf_type_name[128];
> > +
> > + strcpy(btf_type_name, prefix);
> > + strncat(btf_type_name, name, sizeof(btf_type_name) - (prefix_len + 1));
>
> at this point snprintf(btf_type_name, "%s%.*%s", prefix,
> sizeof(btf_type_name) - prefix_len - 1, name) looks like a better and
> cleaner alternative.
I just changed it to:
snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
- KP
>
> > + return btf__find_by_name_kind(btf, btf_type_name, BTF_KIND_TYPEDEF);
> > +}
> > +
> > +#define BTF_TRACE_PREFIX "btf_trace_"
> > +#define BTF_LSM_PREFIX "lsm_btf_"
> > +
>
> [...]
^ permalink raw reply
* Re: [PATCH bpf-next v1 06/13] bpf: lsm: Init Hooks and create files in securityfs
From: KP Singh @ 2020-01-03 23:53 UTC (permalink / raw)
To: Kees Cook
Cc: Andrii Nakryiko, open list, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, James Morris, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <201912301119.B475C474@keescook>
On 30-Dez 11:20, Kees Cook wrote:
> On Mon, Dec 30, 2019 at 04:37:11PM +0100, KP Singh wrote:
> > On 23-Dec 22:28, Andrii Nakryiko wrote:
> > > On Fri, Dec 20, 2019 at 7:43 AM KP Singh <kpsingh@chromium.org> wrote:
> > > [...]
> >
> > Good catch! You're right. These macros will not be there in v2 as
> > we move to using trampolines based callbacks.
>
> Speaking of which -- is the BPF trampoline code correctly designed to be
> W^X?
Thanks for pointing this out!
I don't think this is the case as of now.
The dispatcher logic and the tracing programs allocate one page where
one half of it is used for the active trampoline and the other half is
used as a staging area for a future replacement. I sent a patch as an
attempt to fix this:
https://lore.kernel.org/bpf/20200103234725.22846-1-kpsingh@chromium.org/T/#u
- KP
>
> --
> Kees Cook
^ permalink raw reply
* [PATCH bpf-next] bpf: Make trampolines W^X
From: KP Singh @ 2020-01-03 23:47 UTC (permalink / raw)
To: linux-kernel, bpf, x86, linux-security-module
Cc: Kees Cook, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, Thomas Garnier, Florent Revest,
Brendan Jackman, Jann Horn, Matthew Garrett, Michael Halcrow
From: KP Singh <kpsingh@google.com>
The image for the BPF trampolines is allocated with
bpf_jit_alloc_exe_page which marks this allocated page executable. This
means that the allocated memory is W and X at the same time making it
susceptible to WX based attacks.
Since the allocated memory is shared between two trampolines (the
current and the next), 2 pages must be allocated to adhere to W^X and
the following sequence is obeyed where trampolines are modified:
- Mark memory as non executable (set_memory_nx). While module_alloc for
x86 allocates the memory as PAGE_KERNEL and not PAGE_KERNEL_EXEC, not
all implementations of module_alloc do so
- Mark the memory as read/write (set_memory_rw)
- Modify the trampoline
- Mark the memory as read-only (set_memory_ro)
- Mark the memory as executable (set_memory_x)
There's a window between the modification of the trampoline and setting
the trampoline as read-only where an attacker and ~could~ change the
contents of the memory. This can be further improved by adding more
verification after the memory is marked as read-only.
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: KP Singh <kpsingh@google.com>
---
arch/x86/net/bpf_jit_comp.c | 2 +-
include/linux/bpf.h | 1 -
kernel/bpf/dispatcher.c | 30 +++++++++++++++++++++++----
kernel/bpf/trampoline.c | 41 +++++++++++++++++++------------------
4 files changed, 48 insertions(+), 26 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 4c8a2d1f8470..a510f8260322 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1527,7 +1527,7 @@ int arch_prepare_bpf_trampoline(void *image, struct btf_func_model *m, u32 flags
* Another half is an area for next trampoline.
* Make sure the trampoline generation logic doesn't overflow.
*/
- if (WARN_ON_ONCE(prog - (u8 *)image > PAGE_SIZE / 2 - BPF_INSN_SAFETY))
+ if (WARN_ON_ONCE(prog - (u8 *)image > PAGE_SIZE - BPF_INSN_SAFETY))
return -EFAULT;
return 0;
}
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b14e51d56a82..3be8b1b0166d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -502,7 +502,6 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
int bpf_trampoline_link_prog(struct bpf_prog *prog);
int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
void bpf_trampoline_put(struct bpf_trampoline *tr);
-void *bpf_jit_alloc_exec_page(void);
#define BPF_DISPATCHER_INIT(name) { \
.mutex = __MUTEX_INITIALIZER(name.mutex), \
.func = &name##func, \
diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
index 204ee61a3904..f4589da3bb34 100644
--- a/kernel/bpf/dispatcher.c
+++ b/kernel/bpf/dispatcher.c
@@ -93,13 +93,34 @@ int __weak arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs)
static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
{
s64 ips[BPF_DISPATCHER_MAX] = {}, *ipsp = &ips[0];
- int i;
+ int i, ret;
for (i = 0; i < BPF_DISPATCHER_MAX; i++) {
if (d->progs[i].prog)
*ipsp++ = (s64)(uintptr_t)d->progs[i].prog->bpf_func;
}
- return arch_prepare_bpf_dispatcher(image, &ips[0], d->num_progs);
+
+ /* First make the page non-executable and then make it RW to avoid it
+ * from being W+X. While x86's implementation of module_alloc
+ * allocates memory as non-executable, not all implementations do so.
+ * Till these are fixed, explicitly mark the memory as NX.
+ */
+ set_memory_nx((unsigned long)image, 1);
+ set_memory_rw((unsigned long)image, 1);
+
+ ret = arch_prepare_bpf_dispatcher(image, &ips[0], d->num_progs);
+ if (ret)
+ return ret;
+
+ /* First make the page read-only, and only then make it executable to
+ * prevent it from being W+X in between.
+ */
+ set_memory_ro((unsigned long)image, 1);
+ /* More checks can be done here to ensure that nothing was changed
+ * between arch_prepare_bpf_dispatcher and set_memory_ro.
+ */
+ set_memory_x((unsigned long)image, 1);
+ return 0;
}
static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
@@ -113,7 +134,7 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
noff = 0;
} else {
old = d->image + d->image_off;
- noff = d->image_off ^ (PAGE_SIZE / 2);
+ noff = d->image_off ^ PAGE_SIZE;
}
new = d->num_progs ? d->image + noff : NULL;
@@ -140,10 +161,11 @@ void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
mutex_lock(&d->mutex);
if (!d->image) {
- d->image = bpf_jit_alloc_exec_page();
+ d->image = bpf_jit_alloc_exec(2 * PAGE_SIZE);
if (!d->image)
goto out;
}
+ set_vm_flush_reset_perms(d->image);
prev_num_progs = d->num_progs;
changed |= bpf_dispatcher_remove_prog(d, from);
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 505f4e4b31d2..ff6a92ef8945 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -14,22 +14,6 @@ static struct hlist_head trampoline_table[TRAMPOLINE_TABLE_SIZE];
/* serializes access to trampoline_table */
static DEFINE_MUTEX(trampoline_mutex);
-void *bpf_jit_alloc_exec_page(void)
-{
- void *image;
-
- image = bpf_jit_alloc_exec(PAGE_SIZE);
- if (!image)
- return NULL;
-
- set_vm_flush_reset_perms(image);
- /* Keep image as writeable. The alternative is to keep flipping ro/rw
- * everytime new program is attached or detached.
- */
- set_memory_x((long)image, 1);
- return image;
-}
-
struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
{
struct bpf_trampoline *tr;
@@ -50,12 +34,13 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
goto out;
/* is_root was checked earlier. No need for bpf_jit_charge_modmem() */
- image = bpf_jit_alloc_exec_page();
+ image = bpf_jit_alloc_exec(2 * PAGE_SIZE);
if (!image) {
kfree(tr);
tr = NULL;
goto out;
}
+ set_vm_flush_reset_perms(image);
tr->key = key;
INIT_HLIST_NODE(&tr->hlist);
@@ -125,14 +110,14 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
}
/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
- * bytes on x86. Pick a number to fit into PAGE_SIZE / 2
+ * bytes on x86. Pick a number to fit into PAGE_SIZE.
*/
#define BPF_MAX_TRAMP_PROGS 40
static int bpf_trampoline_update(struct bpf_trampoline *tr)
{
- void *old_image = tr->image + ((tr->selector + 1) & 1) * PAGE_SIZE/2;
- void *new_image = tr->image + (tr->selector & 1) * PAGE_SIZE/2;
+ void *old_image = tr->image + ((tr->selector + 1) & 1) * PAGE_SIZE;
+ void *new_image = tr->image + (tr->selector & 1) * PAGE_SIZE;
struct bpf_prog *progs_to_run[BPF_MAX_TRAMP_PROGS];
int fentry_cnt = tr->progs_cnt[BPF_TRAMP_FENTRY];
int fexit_cnt = tr->progs_cnt[BPF_TRAMP_FEXIT];
@@ -160,6 +145,13 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
if (fexit_cnt)
flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
+
+ /* First make the page non-executable and then make it RW to avoid it
+ * from being being W+X.
+ */
+ set_memory_nx((unsigned long)new_image, 1);
+ set_memory_rw((unsigned long)new_image, 1);
+
err = arch_prepare_bpf_trampoline(new_image, &tr->func.model, flags,
fentry, fentry_cnt,
fexit, fexit_cnt,
@@ -167,6 +159,15 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
if (err)
goto out;
+ /* First make the page read-only, and only then make it executable to
+ * prevent it from being W+X in between.
+ */
+ set_memory_ro((unsigned long)new_image, 1);
+ /* More checks can be done here to ensure that nothing was changed
+ * between arch_prepare_bpf_trampoline and set_memory_ro.
+ */
+ set_memory_x((unsigned long)new_image, 1);
+
if (tr->selector)
/* progs already running at this address */
err = modify_fentry(tr, old_image, new_image);
--
2.20.1
^ permalink raw reply related
* [PATCH v13 26/25] Audit: Multiple LSM support in audit rules
From: Casey Schaufler @ 2020-01-03 18:53 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: keescook, john.johansen, penguin-kernel, paul, sds,
linux-audit@redhat.com, linux-integrity, Mimi Zohar
In-Reply-To: <20191224235939.7483-1-casey@schaufler-ca.com>
With multiple possible security modules supporting audit rule
it is necessary to keep separate data for each module in the
audit rules. This affects IMA as well, as it re-uses the audit
rule list mechanisms.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-audit@redhat.com
---
include/linux/audit.h | 4 +++-
include/linux/security.h | 8 +++----
kernel/auditfilter.c | 26 +++++++++++----------
kernel/auditsc.c | 12 +++++-----
security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++----------
security/security.c | 34 +++++++++++++++++++++++----
6 files changed, 80 insertions(+), 40 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2ce0e8da3922..d4213c471801 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -11,6 +11,7 @@
#include <linux/sched.h>
#include <linux/ptrace.h>
+#include <linux/security.h>
#include <uapi/linux/audit.h>
#define AUDIT_INO_UNSET ((unsigned long)-1)
@@ -64,8 +65,9 @@ struct audit_field {
kuid_t uid;
kgid_t gid;
struct {
+ bool lsm_isset;
char *lsm_str;
- void *lsm_rule;
+ void *lsm_rules[LSMBLOB_ENTRIES];
};
};
u32 op;
diff --git a/include/linux/security.h b/include/linux/security.h
index 26967055a002..0bf71dd74a9c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1887,8 +1887,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
int security_audit_rule_known(struct audit_krule *krule);
int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
- void *lsmrule);
-void security_audit_rule_free(void *lsmrule);
+ void **lsmrule);
+void security_audit_rule_free(void **lsmrule);
#else
@@ -1904,12 +1904,12 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
}
static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
- u32 op, void *lsmrule)
+ u32 op, void **lsmrule)
{
return 0;
}
-static inline void security_audit_rule_free(void *lsmrule)
+static inline void security_audit_rule_free(void **lsmrule)
{ }
#endif /* CONFIG_SECURITY */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bf28bb599b6d..0f351d1f6ef9 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -74,7 +74,7 @@ static void audit_free_lsm_field(struct audit_field *f)
case AUDIT_OBJ_LEV_LOW:
case AUDIT_OBJ_LEV_HIGH:
kfree(f->lsm_str);
- security_audit_rule_free(f->lsm_rule);
+ security_audit_rule_free(f->lsm_rules);
}
}
@@ -517,7 +517,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
entry->rule.buflen += f->val;
err = security_audit_rule_init(f->type, f->op, str,
- (void **)&f->lsm_rule);
+ f->lsm_rules);
/* Keep currently invalid fields around in case they
* become valid after a policy reload. */
if (err == -EINVAL) {
@@ -528,8 +528,10 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
if (err) {
kfree(str);
goto exit_free;
- } else
+ } else {
+ f->lsm_isset = true;
f->lsm_str = str;
+ }
break;
case AUDIT_WATCH:
str = audit_unpack_string(&bufp, &remain, f->val);
@@ -767,7 +769,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
return 0;
}
-/* Duplicate LSM field information. The lsm_rule is opaque, so must be
+/* Duplicate LSM field information. The lsm_rules is opaque, so must be
* re-initialized. */
static inline int audit_dupe_lsm_field(struct audit_field *df,
struct audit_field *sf)
@@ -781,9 +783,9 @@ static inline int audit_dupe_lsm_field(struct audit_field *df,
return -ENOMEM;
df->lsm_str = lsm_str;
- /* our own (refreshed) copy of lsm_rule */
+ /* our own (refreshed) copy of lsm_rules */
ret = security_audit_rule_init(df->type, df->op, df->lsm_str,
- (void **)&df->lsm_rule);
+ df->lsm_rules);
/* Keep currently invalid fields around in case they
* become valid after a policy reload. */
if (ret == -EINVAL) {
@@ -835,7 +837,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
new->tree = old->tree;
memcpy(new->fields, old->fields, sizeof(struct audit_field) * fcount);
- /* deep copy this information, updating the lsm_rule fields, because
+ /* deep copy this information, updating the lsm_rules fields, because
* the originals will all be freed when the old rule is freed. */
for (i = 0; i < fcount; i++) {
switch (new->fields[i].type) {
@@ -1354,11 +1356,11 @@ int audit_filter(int msgtype, unsigned int listtype)
case AUDIT_SUBJ_TYPE:
case AUDIT_SUBJ_SEN:
case AUDIT_SUBJ_CLR:
- if (f->lsm_rule) {
+ if (f->lsm_isset) {
security_task_getsecid(current, &blob);
result = security_audit_rule_match(
&blob, f->type,
- f->op, f->lsm_rule);
+ f->op, f->lsm_rules);
}
break;
case AUDIT_EXE:
@@ -1385,7 +1387,7 @@ int audit_filter(int msgtype, unsigned int listtype)
return ret;
}
-static int update_lsm_rule(struct audit_krule *r)
+static int update_lsm_rules(struct audit_krule *r)
{
struct audit_entry *entry = container_of(r, struct audit_entry, rule);
struct audit_entry *nentry;
@@ -1417,7 +1419,7 @@ static int update_lsm_rule(struct audit_krule *r)
return err;
}
-/* This function will re-initialize the lsm_rule field of all applicable rules.
+/* This function will re-initialize the lsm_rules field of all applicable rules.
* It will traverse the filter lists serarching for rules that contain LSM
* specific filter fields. When such a rule is found, it is copied, the
* LSM field is re-initialized, and the old rule is replaced with the
@@ -1432,7 +1434,7 @@ int audit_update_lsm_rules(void)
for (i = 0; i < AUDIT_NR_FILTERS; i++) {
list_for_each_entry_safe(r, n, &audit_rules_list[i], list) {
- int res = update_lsm_rule(r);
+ int res = update_lsm_rules(r);
if (!err)
err = res;
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 28fea2e73040..b9f81ef64c39 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -638,7 +638,7 @@ static int audit_filter_rules(struct task_struct *tsk,
match for now to avoid losing information that
may be wanted. An error message will also be
logged upon error */
- if (f->lsm_rule) {
+ if (f->lsm_isset) {
if (need_sid) {
security_task_getsecid(tsk, &blob);
need_sid = 0;
@@ -646,7 +646,7 @@ static int audit_filter_rules(struct task_struct *tsk,
result = security_audit_rule_match(&blob,
f->type,
f->op,
- f->lsm_rule);
+ f->lsm_rules);
}
break;
case AUDIT_OBJ_USER:
@@ -656,21 +656,21 @@ static int audit_filter_rules(struct task_struct *tsk,
case AUDIT_OBJ_LEV_HIGH:
/* The above note for AUDIT_SUBJ_USER...AUDIT_SUBJ_CLR
also applies here */
- if (f->lsm_rule) {
+ if (f->lsm_isset) {
/* Find files that match */
if (name) {
result = security_audit_rule_match(
&name->oblob,
f->type,
f->op,
- f->lsm_rule);
+ f->lsm_rules);
} else if (ctx) {
list_for_each_entry(n, &ctx->names_list, list) {
if (security_audit_rule_match(
&n->oblob,
f->type,
f->op,
- f->lsm_rule)) {
+ f->lsm_rules)) {
++result;
break;
}
@@ -681,7 +681,7 @@ static int audit_filter_rules(struct task_struct *tsk,
break;
if (security_audit_rule_match(&ctx->ipc.oblob,
f->type, f->op,
- f->lsm_rule))
+ f->lsm_rules))
++result;
}
break;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1c617ae74558..227993b8422d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -74,7 +74,7 @@ struct ima_rule_entry {
bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
int pcr;
struct {
- void *rule; /* LSM file metadata specific */
+ void *rules[LSMBLOB_ENTRIES];
void *args_p; /* audit value */
int type; /* audit type */
} lsm[MAX_LSM_RULES];
@@ -82,6 +82,16 @@ struct ima_rule_entry {
struct ima_template_desc *template;
};
+static inline bool ima_lsm_isset(void *rules[])
+{
+ int i;
+
+ for (i = 0; i < LSMBLOB_ENTRIES; i++)
+ if (rules[i])
+ return true;
+ return false;
+}
+
/*
* Without LSM specific knowledge, the default policy can only be
* written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
@@ -252,9 +262,11 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
static void ima_lsm_free_rule(struct ima_rule_entry *entry)
{
int i;
+ int r;
for (i = 0; i < MAX_LSM_RULES; i++) {
- kfree(entry->lsm[i].rule);
+ for (r = 0; r < LSMBLOB_ENTRIES; r++)
+ kfree(entry->lsm[i].rules[r]);
kfree(entry->lsm[i].args_p);
}
kfree(entry);
@@ -277,7 +289,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
memset(nentry->lsm, 0, sizeof_field(struct ima_rule_entry, lsm));
for (i = 0; i < MAX_LSM_RULES; i++) {
- if (!entry->lsm[i].rule)
+ if (!ima_lsm_isset(entry->lsm[i].rules))
continue;
nentry->lsm[i].type = entry->lsm[i].type;
@@ -289,7 +301,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
result = security_filter_rule_init(nentry->lsm[i].type,
Audit_equal,
nentry->lsm[i].args_p,
- &nentry->lsm[i].rule);
+ nentry->lsm[i].rules);
if (result == -EINVAL)
pr_warn("ima: rule for LSM \'%d\' is undefined\n",
entry->lsm[i].type);
@@ -329,7 +341,7 @@ static void ima_lsm_update_rules(void)
list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
needs_update = 0;
for (i = 0; i < MAX_LSM_RULES; i++) {
- if (entry->lsm[i].rule) {
+ if (ima_lsm_isset(entry->lsm[i].rules)) {
needs_update = 1;
break;
}
@@ -415,7 +427,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
int rc = 0;
struct lsmblob blob;
- if (!rule->lsm[i].rule)
+ if (!ima_lsm_isset(rule->lsm[i].rules))
continue;
switch (i) {
@@ -426,7 +438,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
rc = security_filter_rule_match(&blob,
rule->lsm[i].type,
Audit_equal,
- rule->lsm[i].rule);
+ rule->lsm[i].rules);
break;
case LSM_SUBJ_USER:
case LSM_SUBJ_ROLE:
@@ -434,7 +446,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
rc = security_filter_rule_match(&blob,
rule->lsm[i].type,
Audit_equal,
- rule->lsm[i].rule);
+ rule->lsm[i].rules);
default:
break;
}
@@ -811,7 +823,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
{
int result;
- if (entry->lsm[lsm_rule].rule)
+ if (ima_lsm_isset(entry->lsm[lsm_rule].rules))
return -EINVAL;
entry->lsm[lsm_rule].args_p = match_strdup(args);
@@ -822,8 +834,8 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
result = security_filter_rule_init(entry->lsm[lsm_rule].type,
Audit_equal,
entry->lsm[lsm_rule].args_p,
- &entry->lsm[lsm_rule].rule);
- if (!entry->lsm[lsm_rule].rule) {
+ entry->lsm[lsm_rule].rules);
+ if (!ima_lsm_isset(entry->lsm[lsm_rule].rules)) {
kfree(entry->lsm[lsm_rule].args_p);
return -EINVAL;
}
@@ -1470,7 +1482,7 @@ int ima_policy_show(struct seq_file *m, void *v)
}
for (i = 0; i < MAX_LSM_RULES; i++) {
- if (entry->lsm[i].rule) {
+ if (ima_lsm_isset(entry->lsm[i].rules)) {
switch (i) {
case LSM_OBJ_USER:
seq_printf(m, pt(Opt_obj_user),
diff --git a/security/security.c b/security/security.c
index e94de64e114c..4be490512ed2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2831,7 +2831,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
{
- return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
+ struct security_hook_list *hp;
+ bool one_is_good = false;
+ int rc = 0;
+ int trc;
+
+ hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
+ if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+ continue;
+ trc = hp->hook.audit_rule_init(field, op, rulestr,
+ &lsmrule[hp->lsmid->slot]);
+ if (trc == 0)
+ one_is_good = true;
+ else
+ rc = trc;
+ }
+ if (one_is_good)
+ return 0;
+ return rc;
}
int security_audit_rule_known(struct audit_krule *krule)
@@ -2839,13 +2856,19 @@ int security_audit_rule_known(struct audit_krule *krule)
return call_int_hook(audit_rule_known, 0, krule);
}
-void security_audit_rule_free(void *lsmrule)
+void security_audit_rule_free(void **lsmrule)
{
- call_void_hook(audit_rule_free, lsmrule);
+ struct security_hook_list *hp;
+
+ hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
+ if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+ continue;
+ hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
+ }
}
int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
- void *lsmrule)
+ void **lsmrule)
{
struct security_hook_list *hp;
int rc;
@@ -2854,7 +2877,8 @@ int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
continue;
rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
- field, op, lsmrule);
+ field, op,
+ &lsmrule[hp->lsmid->slot]);
if (rc != 0)
return rc;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] ima: Add a space after printing a LSM rule for readability
From: Mimi Zohar @ 2020-01-03 17:11 UTC (permalink / raw)
To: clayc, linux-kernel
Cc: linux-security-module, linux-integrity, dmitry.kasatkin, jmorris,
serge
In-Reply-To: <1578037863-7102-1-git-send-email-clayc@hpe.com>
On Fri, 2020-01-03 at 15:51 +0800, clayc@hpe.com wrote:
> From: Clay Chang <clayc@hpe.com>
Normally this "From" line is only seen when the sender isn't the patch
author. Any ideas what happened?
>
> When reading ima_policy from securityfs, there is a missing
> space between output string of LSM rules.
>
> Signed-off-by: Clay Chang <clayc@hpe.com>
Good catch! IMA policy rules based on LSM labels are used to
constrain which files are in policy. Normally a single LSM label is
enough (e.g. dont_measure obj_type=auditd_log_t). Could you include
in this patch description a use case where multiple LSM labels are
needed?
thanks,
Mimi
> ---
> security/integrity/ima/ima_policy.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index ef8dfd47c7e3..1a266e4f99bc 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1496,6 +1496,7 @@ int ima_policy_show(struct seq_file *m, void *v)
> (char *)entry->lsm[i].args_p);
> break;
> }
> + seq_puts(m, " ");
> }
> }
> if (entry->template)
^ permalink raw reply
* Re: [RFC PATCH] selinux: deprecate disabling SELinux and runtime
From: Paul Moore @ 2020-01-03 15:55 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Linux Security Module list
In-Reply-To: <CAFqZXNubaXZtF-yN6tMBuM+AGmSy=1nTcTimFfXaok32GY3aYA@mail.gmail.com>
On Fri, Jan 3, 2020 at 4:32 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Jan 2, 2020 at 10:38 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Jan 2, 2020 at 4:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality ...
...
> > > Looks reasonable, informal ACK from me.
> >
> > Thanks. You want to make that a formal ACK? ;)
>
> Sure, if you find it useful :)
>
> Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
Yes, it is useful, thank you.
For this patch your ACK is particularly significant because you are
representing RH here (I'm assuming you are still the RH SELinux kernel
person) and we are deprecating a feature used by Fedora. In my
opinion it would be a mistake to merge a deprecation patch without the
ACKs of those who rely on the feature targeted for removal (although
in some cases it may need to be done regardless).
I also really dislike merging my own patches without at least one
other Acked-by/Reviewed-by tag for the simple reason that I believe
every patch should have at least two people (author and at least one
reviewer) who agree that the patch is reasonable. Of course there are
exceptions for trivial and critical fixes, e.g. 15b590a81fcd
("selinux: ensure the policy has been loaded before reading the sidtab
stats"), but I like to keep those as the exception rather than the
rule. Just because someone is listed in the MAINTAINERS file
shouldn't mean they are exempt from the normal review process.
Generally speaking, one of the more useful things one can do from an
upstream perspective is to review and test patches that are submitted
to the list. We are a community driven project after all, and the
community aspect shouldn't be limited to just the development of
patches ;)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH] selinux: deprecate disabling SELinux and runtime
From: Ondrej Mosnacek @ 2020-01-03 9:32 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Linux Security Module list
In-Reply-To: <CAHC9VhTHroatmHKt3Saru18TktFY8EXjsxkx-pWvx87-RUx8HA@mail.gmail.com>
On Thu, Jan 2, 2020 at 10:38 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jan 2, 2020 at 4:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote:
> > > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The
> > > code was originally developed to make it easier for Linux
> > > distributions to support architectures where adding parameters to the
> > > kernel command line was difficult. Unfortunately, supporting runtime
> > > disable meant we had to make some security trade-offs when it came to
> > > the LSM hooks, as documented in the Kconfig help text:
> > >
> > > NOTE: selecting this option will disable the '__ro_after_init'
> > > kernel hardening feature for security hooks. Please consider
> > > using the selinux=0 boot parameter instead of enabling this
> > > option.
> > >
> > > Fortunately it looks as if that the original motivation for the
> > > runtime disable functionality is gone, and Fedora/RHEL appears to be
> > > the only major distribution enabling this capability at build time
> > > so we are now taking steps to remove it entirely from the kernel.
> > > The first step is to mark the functionality as deprecated and print
> > > an error when it is used (what this patch is doing). As Fedora/RHEL
> > > makes progress in transitioning the distribution away from runtime
> > > disable, we will introduce follow-up patches over several kernel
> > > releases which will block for increasing periods of time when the
> > > runtime disable is used. Finally we will remove the option entirely
> > > once we believe all users have moved to the kernel cmdline approach.
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> >
> > Looks reasonable, informal ACK from me.
>
> Thanks. You want to make that a formal ACK? ;)
Sure, if you find it useful :)
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH v6 07/10] proc: flush task dcache entries from all procfs instances
From: Alexey Gladkov @ 2020-01-03 8:56 UTC (permalink / raw)
To: J Freyensee
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, Stephen Rothwell
In-Reply-To: <8d85ba43-0759-358e-137d-246107bac747@gmail.com>
On Mon, Dec 30, 2019 at 02:03:29PM -0800, J Freyensee wrote:
> > +#ifdef CONFIG_PROC_FS
> > +static inline void pidns_proc_lock(struct pid_namespace *pid_ns)
> > +{
> > + down_write(&pid_ns->rw_proc_mounts);
> > +}
> > +
> > +static inline void pidns_proc_unlock(struct pid_namespace *pid_ns)
> > +{
> > + up_write(&pid_ns->rw_proc_mounts);
> > +}
> > +
> > +static inline void pidns_proc_lock_shared(struct pid_namespace *pid_ns)
> > +{
> > + down_read(&pid_ns->rw_proc_mounts);
> > +}
> > +
> > +static inline void pidns_proc_unlock_shared(struct pid_namespace *pid_ns)
> > +{
> > + up_read(&pid_ns->rw_proc_mounts);
> > +}
> > +#else /* !CONFIG_PROC_FS */
> > +
> Apologies for my newbie question. I couldn't help but notice all these
> function calls are assuming that the parameter struct pid_namespace *pid_ns
> will never be NULL. Is that a good assumption?
These inline helpers are introduced to improve readability. They only make
sense inside procfs. I don't think that defensive programming is useful
here.
--
Rgrds, legion
^ permalink raw reply
* [PATCH] ima: Add a space after printing a LSM rule for readability
From: clayc @ 2020-01-03 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: linux-security-module, linux-integrity, zohar, dmitry.kasatkin,
jmorris, serge, Clay Chang
From: Clay Chang <clayc@hpe.com>
When reading ima_policy from securityfs, there is a missing
space between output string of LSM rules.
Signed-off-by: Clay Chang <clayc@hpe.com>
---
security/integrity/ima/ima_policy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ef8dfd47c7e3..1a266e4f99bc 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1496,6 +1496,7 @@ int ima_policy_show(struct seq_file *m, void *v)
(char *)entry->lsm[i].args_p);
break;
}
+ seq_puts(m, " ");
}
}
if (entry->template)
--
2.18.1
^ permalink raw reply related
* Re: [PATCH v13 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Casey Schaufler @ 2020-01-02 23:36 UTC (permalink / raw)
To: Mimi Zohar, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, sds,
Janne Karhunen
In-Reply-To: <1577797998.5874.75.camel@linux.ibm.com>
On 12/31/2019 5:13 AM, Mimi Zohar wrote:
> [Cc'ing Janne Karhunen based on his recent work updating IMA policy
> rules LSM id's - commit b16942455193 ("ima: use the lsm policy update
> notifier")]
>
> On Tue, 2019-12-24 at 15:59 -0800, Casey Schaufler wrote:
>> diff --git a/security/security.c b/security/security.c
>> index 87fc70f77660..12e1e6223233 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -439,7 +439,7 @@ static int lsm_append(const char *new, char **result)
>> /*
>> * Current index to use while initializing the lsmblob secid list.
>> */
>> -static int lsm_slot __initdata;
>> +static int lsm_slot __lsm_ro_after_init;
>>
>> /**
>> * security_add_hooks - Add a modules hooks to the hook lists.
>> @@ -2412,9 +2412,21 @@ void security_audit_rule_free(void *lsmrule)
>> call_void_hook(audit_rule_free, lsmrule);
>> }
>>
>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>> + void *lsmrule)
>> {
>> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>> + struct security_hook_list *hp;
>> + int rc;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> + continue;
>> + rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
>> + field, op, lsmrule);
> IMA's policy rules may be written in terms of LSM labels. On IMA
> policy initialization and, subsequently, when the LSM policy is
> updated, IMA correlates LSM labels with LSM ids. Doesn't
> security_audit_rule_init() also need to be updated to walk the LSMs?
Yes. I've got a change in test.
>
> The basic assumption with security_audit_rule_match() is that there
> isn't any naming overlap. Is that guaranteed?
No. A valid SELinux label is also a valid Smack label. If someone
asks to see subj_user=whatever_t both module will look for it.
> With this change, do
> the IMA policy rules now need to be LSM qualified?
I have a change for that in test, too.
>
> Mimi
>
>> + if (rc != 0)
>> + return rc;
>> + }
>> + return 0;
>> }
>> #endif /* CONFIG_AUDIT */
^ permalink raw reply
* Re: [RFC PATCH] selinux: deprecate disabling SELinux and runtime
From: Paul Moore @ 2020-01-02 21:38 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Linux Security Module list
In-Reply-To: <CAFqZXNvXuWx-kCJeZKOgx4NSesCvnC63kHf6-=_SrFLH4xubag@mail.gmail.com>
On Thu, Jan 2, 2020 at 4:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote:
> > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The
> > code was originally developed to make it easier for Linux
> > distributions to support architectures where adding parameters to the
> > kernel command line was difficult. Unfortunately, supporting runtime
> > disable meant we had to make some security trade-offs when it came to
> > the LSM hooks, as documented in the Kconfig help text:
> >
> > NOTE: selecting this option will disable the '__ro_after_init'
> > kernel hardening feature for security hooks. Please consider
> > using the selinux=0 boot parameter instead of enabling this
> > option.
> >
> > Fortunately it looks as if that the original motivation for the
> > runtime disable functionality is gone, and Fedora/RHEL appears to be
> > the only major distribution enabling this capability at build time
> > so we are now taking steps to remove it entirely from the kernel.
> > The first step is to mark the functionality as deprecated and print
> > an error when it is used (what this patch is doing). As Fedora/RHEL
> > makes progress in transitioning the distribution away from runtime
> > disable, we will introduce follow-up patches over several kernel
> > releases which will block for increasing periods of time when the
> > runtime disable is used. Finally we will remove the option entirely
> > once we believe all users have moved to the kernel cmdline approach.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Looks reasonable, informal ACK from me.
Thanks. You want to make that a formal ACK? ;)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] security: apparmor: Fix a possible sleep-in-atomic-context bug in find_attach()
From: John Johansen @ 2020-01-02 19:19 UTC (permalink / raw)
To: Al Viro, Jia-Ju Bai; +Cc: jmorris, serge, linux-security-module, linux-kernel
In-Reply-To: <20191226223056.GQ4203@ZenIV.linux.org.uk>
On 12/26/19 2:30 PM, Al Viro wrote:
> On Tue, Dec 17, 2019 at 09:12:20PM +0800, Jia-Ju Bai wrote:
>> The kernel may sleep while holding a RCU lock.
>> The function call path (from bottom to top) in Linux 4.19 is:
>>
>> security/apparmor/domain.c, 331:
>> vfs_getxattr_alloc(GFP_KERNEL) in aa_xattrs_match
>> security/apparmor/domain.c, 425:
>> aa_xattrs_match in __attach_match
>> security/apparmor/domain.c, 485:
>> __attach_match in find_attach
>> security/apparmor/domain.c, 484:
>> rcu_read_lock in find_attach
>>
>> vfs_getxattr_alloc(GFP_KERNEL) can sleep at runtime.
>>
>> To fix this possible bug, GFP_KERNEL is replaced with GFP_ATOMIC for
>> vfs_getxattr_alloc().
>>
>> This bug is found by a static analysis tool STCheck written by myself.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> ---
>> security/apparmor/domain.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>> index 9be7ccb8379e..60b54ce57d1f 100644
>> --- a/security/apparmor/domain.c
>> +++ b/security/apparmor/domain.c
>> @@ -325,7 +325,7 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
>>
>> for (i = 0; i < profile->xattr_count; i++) {
>> size = vfs_getxattr_alloc(d, profile->xattrs[i], &value,
>> - value_size, GFP_KERNEL);
>> + value_size, GFP_ATOMIC);
>
> <stares>
>
> How can that possibly make any sense? We are, by the look of it, trying
> to read extended attributes of some sort here. How the hell can that
> possibly hope to be non-blocking?
>
it can't
> <goes to read>
> Yup, vfs_getxattr_alloc() does call xattr ->get() method, which certainly
> can cause IO. GFP_ATOMIC affects only the allocation done in
> vfs_getxattr_alloc() itself, ->get() call doesn't even see it.
>
> AFAICS, that "fix" is pure cargo-culting; if that code *can* be called in
> non-blocking context, we are fucked, GFP_ATOMIC or no GFP_ATOMIC.
>
> Let's look at your call chain... find_attach() calls __attach_match()
> under rcu_read_lock(). Yes, it does, and by the look of the function
> itself it does expect to be called thus.
>
> Call of aa_xattrs_match() in there. Present, no obvious dropping/regaining
> rcu_read_lock() around it. Conditional upon perm & MAY_EXEC, but that
> doesn't seem to be provably excludable by the arguments of this particular
> call. And yes, aa_xattrs_match() is very obviously blocking.
>
yep this is broken. this needs to either break the rcu_read_lock or be switched
to a lock that can sleep.
> So you've caught a real bug, but the "fix" doesn't fix anything whatsoever;
> reading xattrs *does* block, no matter what.
>
> By the look at git blame, we have commit 8e51f9087f4024d20f70f4d9831e1f45d8088331
> Author: Matthew Garrett <mjg59@google.com>
> Date: Thu Feb 8 12:37:19 2018 -0800
>
> apparmor: Add support for attaching profiles via xattr, presence and value
>
> Make it possible to tie Apparmor profiles to the presence of one or more
> extended attributes, and optionally their values. An example usecase for
> this is to automatically transition to a more privileged Apparmor profile
> if an executable has a valid IMA signature, which can then be appraised
> by the IMA subsystem.
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Signed-off-by: John Johansen <john.johansen@canonical.com>
>
> to thank for that. And by the time of that commit the call chain already
> existed, complete with rcu_read_lock().
>
> AFAICS, it really is buggered by design - you can't read xattrs in such
> context. Again, GFP_ATOMIC is useless here - the problem is not in
> allocation, it's IO, possibly over network, etc.
>
indeed, below is a first pass at breaking the rcu_read_lock done against 5.5-rc4
> Morever, *anything* that passes GFP_ATOMIC to vfs_getxattr_alloc() is
> deeply suspect - it's almost certainly cargo-culting in attempt to
> do inherently blocking operation in non-blocking context. <greps>
> No GFP_ATOMIC (thankfully), but there's a bunch of GFP_NOFS and
> I really wonder if _those_ make any sense here...
>
---
commit b5b26347adae89f45a871f30f53d4f17b37cf2d4
Author: John Johansen <john.johansen@canonical.com>
Date: Thu Jan 2 05:31:22 2020 -0800
apparmor: fix aa_xattrs_match() may sleep while holding a RCU lock
aa_xattrs_match() is unfortunately calling vfs_getxattr_alloc() from a
context protected by an rcu_read_lock. This can not be done as
vfs_getxattr_alloc() may sleep regardles of the gfp_t value being
passed to it.
Fix this by breaking the rcu_read_lock on the policy search when the
xattr match feature is requested and restarting the search if policy
changes occur.
Fixes: 8e51f9087f40 ("apparmor: Add support for attaching profiles via xattr, presence and value")
Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: John Johansen <john.johansen@canonical.com>
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 09996f2552ee..47aff8700547 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -623,7 +623,7 @@ static __poll_t ns_revision_poll(struct file *file, poll_table *pt)
void __aa_bump_ns_revision(struct aa_ns *ns)
{
- ns->revision++;
+ WRITE_ONCE(ns->revision, ns->revision + 1);
wake_up_interruptible(&ns->wait);
}
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 9be7ccb8379e..6ceb74e0f789 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -317,6 +317,7 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
if (!bprm || !profile->xattr_count)
return 0;
+ might_sleep();
/* transition from exec match to xattr set */
state = aa_dfa_null_transition(profile->xmatch, state);
@@ -361,10 +362,11 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
}
/**
- * __attach_match_ - find an attachment match
+ * find_attach - do attachment search for unconfined processes
* @bprm - binprm structure of transitioning task
- * @name - to match against (NOT NULL)
+ * @ns: the current namespace (NOT NULL)
* @head - profile list to walk (NOT NULL)
+ * @name - to match against (NOT NULL)
* @info - info message if there was an error (NOT NULL)
*
* Do a linear search on the profiles in the list. There is a matching
@@ -374,12 +376,11 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
*
* Requires: @head not be shared or have appropriate locks held
*
- * Returns: profile or NULL if no match found
+ * Returns: label or NULL if no match found
*/
-static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
- const char *name,
- struct list_head *head,
- const char **info)
+static struct aa_label *find_attach(const struct linux_binprm *bprm,
+ struct aa_ns *ns, struct list_head *head,
+ const char *name, const char **info)
{
int candidate_len = 0, candidate_xattrs = 0;
bool conflict = false;
@@ -388,6 +389,8 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
AA_BUG(!name);
AA_BUG(!head);
+ rcu_read_lock();
+restart:
list_for_each_entry_rcu(profile, head, base.list) {
if (profile->label.flags & FLAG_NULL &&
&profile->label == ns_unconfined(profile->ns))
@@ -413,16 +416,32 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
perm = dfa_user_allow(profile->xmatch, state);
/* any accepting state means a valid match. */
if (perm & MAY_EXEC) {
- int ret;
+ int ret = 0;
if (count < candidate_len)
continue;
- ret = aa_xattrs_match(bprm, profile, state);
- /* Fail matching if the xattrs don't match */
- if (ret < 0)
- continue;
-
+ if (bprm && profile->xattr_count) {
+ long rev = READ_ONCE(ns->revision);
+
+ if (!aa_get_profile_not0(profile))
+ goto restart;
+ rcu_read_unlock();
+ ret = aa_xattrs_match(bprm, profile,
+ state);
+ rcu_read_lock();
+ aa_put_profile(profile);
+ if (rev !=
+ READ_ONCE(ns->revision))
+ /* policy changed */
+ goto restart;
+ /*
+ * Fail matching if the xattrs don't
+ * match
+ */
+ if (ret < 0)
+ continue;
+ }
/*
* TODO: allow for more flexible best match
*
@@ -445,43 +464,28 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
candidate_xattrs = ret;
conflict = false;
}
- } else if (!strcmp(profile->base.name, name))
+ } else if (!strcmp(profile->base.name, name)) {
/*
* old exact non-re match, without conditionals such
* as xattrs. no more searching required
*/
- return profile;
+ candidate = profile;
+ goto out;
+ }
}
- if (conflict) {
- *info = "conflicting profile attachments";
+ if (!candidate || conflict) {
+ if (conflict)
+ *info = "conflicting profile attachments";
+ rcu_read_unlock();
return NULL;
}
- return candidate;
-}
-
-/**
- * find_attach - do attachment search for unconfined processes
- * @bprm - binprm structure of transitioning task
- * @ns: the current namespace (NOT NULL)
- * @list: list to search (NOT NULL)
- * @name: the executable name to match against (NOT NULL)
- * @info: info message if there was an error
- *
- * Returns: label or NULL if no match found
- */
-static struct aa_label *find_attach(const struct linux_binprm *bprm,
- struct aa_ns *ns, struct list_head *list,
- const char *name, const char **info)
-{
- struct aa_profile *profile;
-
- rcu_read_lock();
- profile = aa_get_profile(__attach_match(bprm, name, list, info));
+out:
+ candidate = aa_get_newest_profile(candidate);
rcu_read_unlock();
- return profile ? &profile->label : NULL;
+ return &candidate->label;
}
static const char *next_name(int xtype, const char *name)
^ permalink raw reply related
* Re: [RFC PATCH] selinux: deprecate disabling SELinux and runtime
From: Ondrej Mosnacek @ 2020-01-02 9:23 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Linux Security Module list
In-Reply-To: <157678334821.158235.2125894638773393579.stgit@chester>
On Thu, Dec 19, 2019 at 8:22 PM Paul Moore <paul@paul-moore.com> wrote:
> Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The
> code was originally developed to make it easier for Linux
> distributions to support architectures where adding parameters to the
> kernel command line was difficult. Unfortunately, supporting runtime
> disable meant we had to make some security trade-offs when it came to
> the LSM hooks, as documented in the Kconfig help text:
>
> NOTE: selecting this option will disable the '__ro_after_init'
> kernel hardening feature for security hooks. Please consider
> using the selinux=0 boot parameter instead of enabling this
> option.
>
> Fortunately it looks as if that the original motivation for the
> runtime disable functionality is gone, and Fedora/RHEL appears to be
> the only major distribution enabling this capability at build time
> so we are now taking steps to remove it entirely from the kernel.
> The first step is to mark the functionality as deprecated and print
> an error when it is used (what this patch is doing). As Fedora/RHEL
> makes progress in transitioning the distribution away from runtime
> disable, we will introduce follow-up patches over several kernel
> releases which will block for increasing periods of time when the
> runtime disable is used. Finally we will remove the option entirely
> once we believe all users have moved to the kernel cmdline approach.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
Looks reasonable, informal ACK from me.
> ---
> security/selinux/Kconfig | 3 +++
> security/selinux/selinuxfs.c | 6 ++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> index 996d35d950f7..580ac24c7aa1 100644
> --- a/security/selinux/Kconfig
> +++ b/security/selinux/Kconfig
> @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE
> using the selinux=0 boot parameter instead of enabling this
> option.
>
> + WARNING: this option is deprecated and will be removed in a future
> + kernel release.
> +
> If you are unsure how to answer this question, answer N.
>
> config SECURITY_SELINUX_DEVELOP
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 278417e67b4c..adbe2dd35202 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
> int new_value;
> int enforcing;
>
> + /* NOTE: we are now officially considering runtime disable as
> + * deprecated, and using it will become increasingly painful
> + * (e.g. sleeping/blocking) as we progress through future
> + * kernel releases until eventually it is removed */
> + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n");
> +
> if (count >= PAGE_SIZE)
> return -ENOMEM;
>
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH v1 - RFC] ima: export the measurement list when needed
From: Janne Karhunen @ 2020-01-01 6:49 UTC (permalink / raw)
To: david.safford
Cc: Mimi Zohar, linux-integrity, linux-security-module, Ken Goldman,
monty.wiseman
In-Reply-To: <f2bc130034b6e1ca66c3f18dfa3a4fa68fcbc82a.camel@gmail.com>
On Tue, Dec 24, 2019 at 5:35 PM <david.safford@gmail.com> wrote:
> > That is a good question. I went this way as it did not feel right to
> > me that the kernel would depend on periodic, reliable userspace
> > functionality to stay running (we would have a circular dependency).
> > The thing is, once the kernel starts to run low on memory, it may
> > kill
> > that periodic daemon flushing the data for reasons unrelated to IMA.
> >
>
> I'm happy with either way (kernel writing, or userspace reading) the
> data, but with the v1 patch, there is no way for userspace to force
> that the list be flushed - it only flushes on full. I think it is
> important for userspace to be able to trigger a flush, such as just
> prior to a kexec, or prior to an attestation.
Indeed, will add in v2.
> Perhaps you could simply remove the length test in ima_export_list(),
> and export anytime the filename is provided? This could simplify
> attestation clients, which could ask for different files each time
> (list.1, list.2...), for automatic log maintenance. Since the template
> format does not have sequence numbers, this would also help keep
> track which records have already been seen.
Yes, will do something like this. Holidays cause some latency here,
but I will send an update next week.
--
Janne
^ permalink raw reply
* Re: [GIT PULL] tomoyo fixes for 5.5
From: pr-tracker-bot @ 2019-12-31 19:45 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Linus Torvalds, linux-security-module
In-Reply-To: <8483f2c2-626d-382f-3994-ee29daebff75@i-love.sakura.ne.jp>
The pull request you sent on Mon, 30 Dec 2019 20:31:40 +0900:
> git://git.osdn.net/gitroot/tomoyo/tomoyo-test1.git master
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c5c928c667cd1e34cbcac6af5b7c2f9f4512d612
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v12 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Casey Schaufler @ 2019-12-31 17:36 UTC (permalink / raw)
To: Mimi Zohar, casey.schaufler, jmorris, linux-security-module,
selinux, Matthew Garrett
Cc: keescook, john.johansen, penguin-kernel, paul, sds,
Casey Schaufler
In-Reply-To: <1577812497.5874.97.camel@linux.ibm.com>
On 12/31/2019 9:14 AM, Mimi Zohar wrote:
> [Cc'ing Matthew Garret based on the additional bprm call to
> process_measurement() - commit d906c10d8a31 ("IMA: Support using new
> creds in appraisal policy")]
>
> On Tue, 2019-12-24 at 15:18 -0800, Casey Schaufler wrote:
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index f19a895ad7cd..193ddd55420b 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -414,6 +414,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> for (i = 0; i < MAX_LSM_RULES; i++) {
>> int rc = 0;
>> u32 osid;
>> + struct lsmblob blob;
>>
>> if (!rule->lsm[i].rule)
>> continue;
>> @@ -423,7 +424,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> case LSM_OBJ_ROLE:
>> case LSM_OBJ_TYPE:
>> security_inode_getsecid(inode, &osid);
>> - rc = security_filter_rule_match(osid,
>> + lsmblob_init(&blob, osid);
>> + rc = security_filter_rule_match(&blob,
>> rule->lsm[i].type,
>> Audit_equal,
>> rule->lsm[i].rule);
>> @@ -431,7 +433,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> case LSM_SUBJ_USER:
>> case LSM_SUBJ_ROLE:
>> case LSM_SUBJ_TYPE:
>> - rc = security_filter_rule_match(secid,
>> + lsmblob_init(&blob, secid);
>> + rc = security_filter_rule_match(&blob,
> On the bprm hook, IMA calls process_measurement() twice. The first
> time the secid is passed as an argument based on a call to
> security_task_getsecid(), while the second time it is based on
> security_cred_getsecid(). process_measurement() passes the correct
> secid converted to a blob, but instead of using the passed variable,
> this code uses the locally defined blob field. A later patch removes
> the the lsmblob_init(), leaving the local blob uninitialized.
> Something is terribly wrong here.
I can see that there's significant work required on audit rule
filtering. security_audit_rule_init() isn't going to work correctly
the way it is.
I'll admit that the aliasing of audit_rule to filter_rule had me
very confused for some time.
>
> Mimi
>
>> rule->lsm[i].type,
>> Audit_equal,
>> rule->lsm[i].rule);
>
>
^ permalink raw reply
* Re: [PATCH v12 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Mimi Zohar @ 2019-12-31 17:14 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, Matthew Garrett
Cc: keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20191224231915.7208-4-casey@schaufler-ca.com>
[Cc'ing Matthew Garret based on the additional bprm call to
process_measurement() - commit d906c10d8a31 ("IMA: Support using new
creds in appraisal policy")]
On Tue, 2019-12-24 at 15:18 -0800, Casey Schaufler wrote:
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index f19a895ad7cd..193ddd55420b 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -414,6 +414,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> for (i = 0; i < MAX_LSM_RULES; i++) {
> int rc = 0;
> u32 osid;
> + struct lsmblob blob;
>
> if (!rule->lsm[i].rule)
> continue;
> @@ -423,7 +424,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_OBJ_ROLE:
> case LSM_OBJ_TYPE:
> security_inode_getsecid(inode, &osid);
> - rc = security_filter_rule_match(osid,
> + lsmblob_init(&blob, osid);
> + rc = security_filter_rule_match(&blob,
> rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rule);
> @@ -431,7 +433,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_SUBJ_USER:
> case LSM_SUBJ_ROLE:
> case LSM_SUBJ_TYPE:
> - rc = security_filter_rule_match(secid,
> + lsmblob_init(&blob, secid);
> + rc = security_filter_rule_match(&blob,
On the bprm hook, IMA calls process_measurement() twice. The first
time the secid is passed as an argument based on a call to
security_task_getsecid(), while the second time it is based on
security_cred_getsecid(). process_measurement() passes the correct
secid converted to a blob, but instead of using the passed variable,
this code uses the locally defined blob field. A later patch removes
the the lsmblob_init(), leaving the local blob uninitialized.
Something is terribly wrong here.
Mimi
> rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rule);
>
^ permalink raw reply
* Re: [PATCH v13 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Mimi Zohar @ 2019-12-31 13:13 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, sds,
Janne Karhunen
In-Reply-To: <20191224235939.7483-4-casey@schaufler-ca.com>
[Cc'ing Janne Karhunen based on his recent work updating IMA policy
rules LSM id's - commit b16942455193 ("ima: use the lsm policy update
notifier")]
On Tue, 2019-12-24 at 15:59 -0800, Casey Schaufler wrote:
> diff --git a/security/security.c b/security/security.c
> index 87fc70f77660..12e1e6223233 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -439,7 +439,7 @@ static int lsm_append(const char *new, char **result)
> /*
> * Current index to use while initializing the lsmblob secid list.
> */
> -static int lsm_slot __initdata;
> +static int lsm_slot __lsm_ro_after_init;
>
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> @@ -2412,9 +2412,21 @@ void security_audit_rule_free(void *lsmrule)
> call_void_hook(audit_rule_free, lsmrule);
> }
>
> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
> + void *lsmrule)
> {
> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> + struct security_hook_list *hp;
> + int rc;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> + continue;
> + rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
> + field, op, lsmrule);
IMA's policy rules may be written in terms of LSM labels. On IMA
policy initialization and, subsequently, when the LSM policy is
updated, IMA correlates LSM labels with LSM ids. Doesn't
security_audit_rule_init() also need to be updated to walk the LSMs?
The basic assumption with security_audit_rule_match() is that there
isn't any naming overlap. Is that guaranteed? With this change, do
the IMA policy rules now need to be LSM qualified?
Mimi
> + if (rc != 0)
> + return rc;
> + }
> + return 0;
> }
> #endif /* CONFIG_AUDIT */
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: Mickaël Salaün @ 2019-12-31 12:11 UTC (permalink / raw)
To: Kees Cook
Cc: KP Singh, linux-kernel, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, James Morris, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Florent Revest,
Brendan Jackman, Martin KaFai Lau, Song Liu, Yonghong Song,
Serge E. Hallyn, Mauro Carvalho Chehab, David S. Miller,
Greg Kroah-Hartman, Nicolas Ferre, Stanislav Fomichev,
Quentin Monnet, Andrey Ignatov, Joe Stringer,
Mickaël Salaün
In-Reply-To: <201912301128.B37D55AB44@keescook>
On 30/12/2019 20:30, Kees Cook wrote:
> On Fri, Dec 20, 2019 at 11:46:47PM +0100, Mickaël Salaün wrote:
>> I'm working on a version of Landlock without eBPF, but still with the
>> initial sought properties: safe unprivileged composability, modularity, and
>> dynamic update. I'll send this version soon.
>>
>> I hope that the work and experience from Landlock to bring eBPF to LSM will
>> continue to be used through KRSI. Landlock will now focus on the
>> unprivileged sandboxing part, without eBPF. Stay tuned!
>
> Will it end up looking at all like pledge? I'm still struggling to come
> up with a sensible pledge-like design on top of seccomp, especially
> given the need to have it very closely tied to the running libc...
>
Yes, it's similar to Pledge/Unveil but with fine-grained control (and a
more flexible design). And because it is not tied to syscall, there is
no similar issues than with seccomp and libc. In fact, there is no more
relationship with seccomp neither. The version I'm working on is similar
in principle to the patch series v10 [1], without the usage complexity
brought by eBPF, but with a more polished file-based access-control. The
demo from LSS 2018 [2] gives an overview of the possibilities.
[1] https://lore.kernel.org/lkml/20190721213116.23476-1-mic@digikod.net/
[2] https://landlock.io/talks/2018-08-27_landlock-lss_demo-1-web.mkv
^ permalink raw reply
* Re: [GIT PULL] tomoyo fixes for 5.5
From: Tetsuo Handa @ 2019-12-31 1:36 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-security-module
In-Reply-To: <CAHk-=wgNYqYSdD530KWQ9gtTeEvBd_Frn54Xc45B3D8PPL8ijA@mail.gmail.com>
On 2019/12/31 5:14, Linus Torvalds wrote:
> On Mon, Dec 30, 2019 at 3:32 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> This is my first time for sending pull requests. It seems that most people
>> create a tag signed with GPG key but a few people send pull requests on
>> master branch without signing with GPG key. Did I follow necessary steps?
>
> I do require the gpg signed tag for non-kernel.org pull requests like this.
>
> I trust the security at kernel.org - it requires 2FA and a gpg key
> just to even push to a git repo there at all - but even there I
> _prefer_ tags. But outside of kernel.org I absolutely do want to see a
> signed tag for a pull request, not just a master branch.
I see. I did the following and got a tag signed with my GPG key. Did I do what you want?
$ git tag -s tomoyo-fixes-for-5.5
$ git push --tags
$ git request-pull tomoyo-fixes-for-5.5 git://git.osdn.net/gitroot/tomoyo/tomoyo-test1.git
The following changes since commit 6bd5ce6089b561f5392460bfb654dea89356ab1b:
tomoyo: Suppress RCU warning at list_for_each_entry_rcu(). (2019-12-16 23:02:27 +0900)
are available in the git repository at:
git://git.osdn.net/gitroot/tomoyo/tomoyo-test1.git tags/tomoyo-fixes-for-5.5
for you to fetch changes up to 6bd5ce6089b561f5392460bfb654dea89356ab1b:
tomoyo: Suppress RCU warning at list_for_each_entry_rcu(). (2019-12-16 23:02:27 +0900)
----------------------------------------------------------------
Two bugfix patches for 5.5.
tomoyo: Suppress RCU warning at list_for_each_entry_rcu().
tomoyo: Don't use nifty names on sockets.
----------------------------------------------------------------
>
> Side note: I don't actually require the pgp key to be something I have
> a direct path to, and if you can't get big set of signatures on yours,
> that's fine for initial pull requests. The key ends up still being a
> kind of identity, and we can work on getting the proper web of trust
> built up over time.
>
> Linus
>
^ permalink raw reply
* Re: [PATCH v6 07/10] proc: flush task dcache entries from all procfs instances
From: J Freyensee @ 2019-12-30 22:03 UTC (permalink / raw)
To: Alexey Gladkov, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: 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, Stephen Rothwell
In-Reply-To: <20191225125151.1950142-8-gladkov.alexey@gmail.com>
snip
.
.
.
>
> +#ifdef CONFIG_PROC_FS
> +static inline void pidns_proc_lock(struct pid_namespace *pid_ns)
> +{
> + down_write(&pid_ns->rw_proc_mounts);
> +}
> +
> +static inline void pidns_proc_unlock(struct pid_namespace *pid_ns)
> +{
> + up_write(&pid_ns->rw_proc_mounts);
> +}
> +
> +static inline void pidns_proc_lock_shared(struct pid_namespace *pid_ns)
> +{
> + down_read(&pid_ns->rw_proc_mounts);
> +}
> +
> +static inline void pidns_proc_unlock_shared(struct pid_namespace *pid_ns)
> +{
> + up_read(&pid_ns->rw_proc_mounts);
> +}
> +#else /* !CONFIG_PROC_FS */
> +
Apologies for my newbie question. I couldn't help but notice all these
function calls are assuming that the parameter struct pid_namespace
*pid_ns will never be NULL. Is that a good assumption?
I don't have the background in this code to answer on my own, but I
thought I'd raise the question.
Thanks,
Jay
^ 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