* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: James Morris @ 2020-01-08 18:58 UTC (permalink / raw)
To: Stephen Smalley
Cc: Kees Cook, KP Singh, Casey Schaufler, open list, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <c4e6cdf2-1233-fc82-ca01-ba84d218f5aa@tycho.nsa.gov>
On Wed, 8 Jan 2020, Stephen Smalley wrote:
> This appears to impose a very different standard to this eBPF-based LSM than
> has been applied to the existing LSMs, e.g. we are required to preserve
> SELinux policy compatibility all the way back to Linux 2.6.0 such that new
> kernel with old policy does not break userspace. If that standard isn't being
> applied to the eBPF-based LSM, it seems to inhibit its use in major Linux
> distros, since otherwise users will in fact start experiencing breakage on the
> first such incompatible change. Not arguing for or against, just trying to
> make sure I understand correctly...
A different standard would be applied here vs. a standard LSM like
SELinux, which are retrofitted access control systems.
I see KRSI as being more of a debugging / analytical API, rather than an
access control system. You could of course build such a system with KRSI
but it would need to provide a layer of abstraction for general purpose
users.
So yes this would be a special case, as its real value is in being a
special case, i.e. dynamic security telemetry.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] LSM: Delete hooks in reverse order for avoiding race
From: James Morris @ 2020-01-08 19:31 UTC (permalink / raw)
To: Huaisheng Ye
Cc: Kees Cook, Casey Schaufler, efremov, Paul Moore, omosnace,
David Howells, joel, tyu1, linux-kernel, Huaisheng Ye,
linux-security-module
In-Reply-To: <20200108083430.57412-1-yehs2007@zoho.com>
Please cc the LSM list with LSM related patches.
On Wed, 8 Jan 2020, Huaisheng Ye wrote:
> From: Huaisheng Ye <yehs1@lenovo.com>
>
> There is small possibility as race condition when selinux_disable
> has been triggered. security_delete_hooks deletes all selinux hooks
> from security_hook_heads, but there are some selinux functions which
> are being called at the same time.
>
> Here is a panic accident scene from 4.18 based kernel,
>
> [ 26.654494] SELinux: Disabled at runtime.
> [ 26.654507] BUG: unable to handle kernel NULL pointer dereference
> at 0000000000000020
> [ 26.654508] PGD 0 P4D 0
> [ 26.654510] Oops: 0002 [#1] SMP NOPTI
> [ 26.654512] CPU: 53 PID: 2614 Comm: systemd-cgroups Tainted: G
> OE --------- - - 4.18.0-80.el8.x86_64 #1
> [ 26.654512] Hardware name: Lenovo ThinkSystem SR850P
> -[7D2H]-/-[7D2H]-, BIOS -[TEE145P-1.10]- 12/06/2019
> [ 26.654519] RIP: 0010:selinux_socket_post_create+0x80/0x390
> [ 26.654520] Code: e9 95 6a 89 00 bd 16 00 00 00 c7 44 24 04 01
> 00 00 00 45 85 c0 0f 85 f6 00 00 00 8b 56 14 85 d2 0f 84 26 01 00
> 00 89 54 24 04 <66> 41 89 6c 24 20 31 c0 41 89 54 24 1c 41 c6 44
> 24 22 01 49 8b 4d
> [ 26.654521] RSP: 0018:ffffbf515cc63e48 EFLAGS: 00010246
> [ 26.654522] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000019
> [ 26.654522] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ffffffffab46f680
> [ 26.654523] RBP: 0000000000000019 R08: 0000000000000000 R09: ffffbf515cc63e4c
> [ 26.654523] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> [ 26.654524] R13: ffff97d7bb6cbc80 R14: 0000000000000001 R15: ffff97d7bb6cbc80
> [ 26.654525] FS: 00007f5c608ea380(0000) GS:ffff97d7bf140000(0000) knlGS:0000000000000000
> [ 26.654525] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 26.654526] CR2: 0000000000000020 CR3: 0000011ebc934004 CR4: 00000000007606e0
> [ 26.654527] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 26.654528] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 26.654528] PKRU: 55555554
> [ 26.654528] Call Trace:
> [ 26.654535] security_socket_post_create+0x42/0x60
> [ 26.654537] SELinux: Unregistering netfilter hooks
> [ 26.654542] __sock_create+0x106/0x1a0
> [ 26.654545] __sys_socket+0x57/0xe0
> [ 26.654547] __x64_sys_socket+0x16/0x20
> [ 26.654551] do_syscall_64+0x5b/0x1b0
> [ 26.654554] entry_SYSCALL_64_after_hwframe+0x65/0xca
>
> The root cause is that, selinux_inode_alloc_security has been deleted
> firstly from security_hook_heads, so security_inode_alloc directly
> return 0, that means the value of pointer inode->i_security equalling
> to NULL.
>
> But selinux_socket_post_create hasn't been deleted at that moment, so
> which would involked by mistake. Inside the function, pointer isec
> needs to point to inode->i_security, then a NULL pointer defect happens.
>
> For current upstream kernel, because of commit
> afb1cbe37440c7f38b9cf46fc331cc9dfd5cce21
> the inode security has been moved out to LSM infrastructure from
> individual security modules like selinux.
>
> But this patch still can be applied for solving similar issue when
> security_delete_hooks has been used. Also for stable branch v4.19,
> the inode security still need to be created in individual modules.
>
> The patch has been verified by Lenovo SR850P server through overnight
> reboot cycles.
>
> Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
> ---
> include/linux/lsm_hooks.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 20d8cf1..57cb2ac 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2164,7 +2164,7 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
> int i;
>
> for (i = 0; i < count; i++)
> - hlist_del_rcu(&hooks[i].list);
> + hlist_del_rcu(&hooks[count - 1 - i].list);
> }
> #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: Stephen Smalley @ 2020-01-08 19:33 UTC (permalink / raw)
To: James Morris
Cc: Kees Cook, KP Singh, Casey Schaufler, open list, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <alpine.LRH.2.21.2001090551000.27794@namei.org>
On 1/8/20 1:58 PM, James Morris wrote:
> On Wed, 8 Jan 2020, Stephen Smalley wrote:
>
>> This appears to impose a very different standard to this eBPF-based LSM than
>> has been applied to the existing LSMs, e.g. we are required to preserve
>> SELinux policy compatibility all the way back to Linux 2.6.0 such that new
>> kernel with old policy does not break userspace. If that standard isn't being
>> applied to the eBPF-based LSM, it seems to inhibit its use in major Linux
>> distros, since otherwise users will in fact start experiencing breakage on the
>> first such incompatible change. Not arguing for or against, just trying to
>> make sure I understand correctly...
>
> A different standard would be applied here vs. a standard LSM like
> SELinux, which are retrofitted access control systems.
>
> I see KRSI as being more of a debugging / analytical API, rather than an
> access control system. You could of course build such a system with KRSI
> but it would need to provide a layer of abstraction for general purpose
> users.
>
> So yes this would be a special case, as its real value is in being a
> special case, i.e. dynamic security telemetry.
The cover letter subject line and the Kconfig help text refer to it as a
BPF-based "MAC and Audit policy". It has an enforce config option that
enables the bpf programs to deny access, providing access control. IIRC,
in the earlier discussion threads, the BPF maintainers suggested that
Smack and other LSMs could be entirely re-implemented via it in the
future, and that such an implementation would be more optimal.
Again, not arguing for or against, but wondering if people fully
understand the implications. If it ends up being useful, people will
build access control systems with it, and it directly exposes a lot of
kernel internals to userspace. There was a lot of concern originally
about the LSM hook interface becoming a stable ABI and/or about it being
misused. Exposing that interface along with every kernel data structure
exposed through it to userspace seems like a major leap. Even if the
mainline kernel doesn't worry about any kind of stable interface
guarantees for it, the distros might be forced to provide some kABI
guarantees for it to appease ISVs and users...
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Edgecombe, Rick P @ 2020-01-08 20:52 UTC (permalink / raw)
To: peterz@infradead.org, luto@kernel.org, Hansen, Dave,
nadav.amit@gmail.com
Cc: songliubraving@fb.com, linux-kernel@vger.kernel.org,
daniel@iogearbox.net, keescook@chromium.org, jeyu@kernel.org,
bpf@vger.kernel.org, kuznet@ms2.inr.ac.ru, ast@kernel.org,
mjg59@google.com, thgarnie@chromium.org, kpsingh@chromium.org,
linux-security-module@vger.kernel.org, x86@kernel.org,
revest@chromium.org, jannh@google.com, namit@vmware.com,
jackmanb@chromium.org, kafai@fb.com, yhs@fb.com,
davem@davemloft.net, yoshfuji@linux-ipv6.org, mhalcrow@google.com,
andriin@fb.com
In-Reply-To: <CALCETrV_tGk=B3Hw0h9viW45wMqB_W+rwWzx6LnC3-vSATOUOA@mail.gmail.com>
On Wed, 2020-01-08 at 00:41 -0800, Andy Lutomirski wrote:
> > On Jan 7, 2020, at 9:01 AM, Edgecombe, Rick P <rick.p.edgecombe@intel.com>
> > wrote:
> >
> > CC Nadav and Jessica.
> >
> > On Mon, 2020-01-06 at 15:36 -1000, Andy Lutomirski wrote:
> > > > On Jan 6, 2020, at 12:25 PM, Edgecombe, Rick P <
> > > > rick.p.edgecombe@intel.com>
> > > > wrote:
> > > >
> > > > On Sat, 2020-01-04 at 09:49 +0900, Andy Lutomirski wrote:
> > > > > > > > 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.
> > > >
> > > > So if I understand this right, some trampolines have been added that are
> > > > currently set as RWX at modification time AND left that way during
> > > > runtime?
> > > > The
> > > > discussion on the order of set_memory_() calls in the commit message
> > > > made me
> > > > think that this was just a modification time thing at first.
> > >
> > > I’m not sure what the status quo is.
> > >
> > > We really ought to have a genuinely good API for allocation and
> > > initialization
> > > of text. We can do so much better than set_memory_blahblah.
> > >
> > > FWIW, I have some ideas about making kernel flushes cheaper. It’s
> > > currently
> > > blocked on finding some time and on tglx’s irqtrace work.
> > >
> >
> > Makes sense to me. I guess there are 6 types of text allocations now:
> > - These two BPF trampolines
> > - BPF JITs
> > - Modules
> > - Kprobes
> > - Ftrace
> >
> > All doing (or should be doing) pretty much the same thing. I believe Jessica
> > had
> > said at one point that she didn't like all the other features using
> > module_alloc() as it was supposed to be just for real modules. Where would
> > the
> > API live?
>
> New header? This shouldn’t matter that much.
>
> Here are two strawman proposals. All of this is very rough -- the
> actual data structures and signatures are likely problematic for
> multiple reasons.
>
> --- First proposal ---
>
> struct text_allocation {
> void *final_addr;
> struct page *pages;
> int npages;
> };
>
> int text_alloc(struct text_allocation *out, size_t size);
>
> /* now final_addr is not accessible and pages is writable. */
>
> int text_freeze(struct text_allocation *alloc);
>
> /* now pages are not accessible and final_addr is RO. Alternatively,
> pages are RO and final_addr is unmapped. */
>
> int text_finish(struct text_allocation *alloc);
>
> /* now final_addr is RX. All done. */
>
> This gets it with just one flush and gives a chance to double-check in
> case of race attacks from other CPUs. Double-checking is annoying,
> though.
>
> --- Second proposal ---
>
> struct text_allocation {
> void *final_addr;
> /* lots of opaque stuff including an mm_struct */
> /* optional: list of struct page, but this isn't obviously useful */
> };
>
> int text_alloc(struct text_allocation *out, size_t size);
>
> /* Memory is allocated. There is no way to access it at all right
> now. The memory is RO or not present in the direct map. */
>
> void __user *text_activate_mapping(struct text_allocation *out);
>
> /* Now the text is RW at *user* address given by return value.
> Preemption is off if required by use_temporary_mm(). Real user memory
> cannot be accessed. */
>
> void text_deactivate_mapping(struct text_allocation *alloc);
>
> /* Now the memory is inaccessible again. */
>
> void text_finalize(struct text_allocation *alloc);
>
> /* Now it's RX or XO at the final address. */
>
>
> Pros of second approach:
>
> - Inherently immune to cross-CPU attack. No double-check.
>
> - If we ever implement a cache of non-direct-mapped, unaliased pages,
> then it works with no flushes at all. We could even relax it a bit to
> allow non-direct-mapped pages that may have RX / XO aliases but no W
> aliases.
>
> - Can easily access without worrying about page boundaries.
>
> Cons:
>
> - The use of a temporary mm is annoying -- you can't copy from user
> memory, for example.
Probably the first proposal is better for usages where there is a signature that
can be checked like modules, because you could more easily check the signature
after the text is RO. I guess leaving the direct map as RO could work for the
second option too. Both would probably require significant changes to module
signature verification though.
Just a minor point/clarification, but outside of an enhanced signed module case,
I think the cross-CPU attack mitigation can't be full. For example, attacking
the verified BPF byte code (which is apparently planned to no longer be RO), or
the pointers being loaded into these trampolines. There is always going to be
some writable source or pointer to the source, and unless there is a way to
verify the end RO result, it's an un-winnable game of whack-a-mole to do it in
full. Still the less exposed surfaces the better since the writes we are
worrying about in this case are probably not fully arbitrary.
I don't see why it would be so bad to require copying data to the kernel before
sending it through this process. Nothing copies to final allocation directly
from userspace today, and from a perf perspective, how bad is an extra copy when
we are saving TLB shootdowns? Are you thinking to protect the data that's being
loaded from other CPUs?
Otherwise, could we lazily clone/sync the original mm into the temporary one to
allow this? (possibly totally misguided idea)
FWIW, I really like the idea of a cache of unmapped or RO pages. Potentially
several optimizations we could do there.
If this API should be cross platform, we might want to abstract the copy itself
as well, since other arch's might have non __user solutions for copying data in.
Unless someone else wants to, I can probably take a look at a first cut of this
after I get the current thing I'm working on out. Probably better to let the
dust settle on the ftrace changes as well.
^ permalink raw reply
* RE: [External] Re: [PATCH] LSM: Delete hooks in reverse order for avoiding race
From: Huaisheng HS1 Ye @ 2020-01-09 3:22 UTC (permalink / raw)
To: Ondrej Mosnacek, James Morris
Cc: Kees Cook, Casey Schaufler, efremov@ispras.ru, Paul Moore,
David Howells, joel@joelfernandes.org, Tzu ting Yu1,
Linux kernel mailing list, SElinux list,
Linux Security Module list, Huaisheng Ye, Chris McDermott2
In-Reply-To: <CAFqZXNujo1px1=JVc+Chr_trVDRpwcXv7pqWVSxi+yifvWoMuA@mail.gmail.com>
> -----Original Message-----
> From: Ondrej Mosnacek <omosnace@redhat.com>
> Sent: Wednesday, January 8, 2020 7:43 PM
>
> Hi,
>
> On Wed, Jan 8, 2020 at 9:51 AM Huaisheng Ye <yehs2007@zoho.com> wrote:
> > From: Huaisheng Ye <yehs1@lenovo.com>
> >
> > There is small possibility as race condition when selinux_disable has
> > been triggered. security_delete_hooks deletes all selinux hooks from
> > security_hook_heads, but there are some selinux functions which are
> > being called at the same time.
> >
> > Here is a panic accident scene from 4.18 based kernel,
> >
> > [ 26.654494] SELinux: Disabled at runtime.
> > [ 26.654507] BUG: unable to handle kernel NULL pointer dereference
> > at 0000000000000020
> > [ 26.654508] PGD 0 P4D 0
> > [ 26.654510] Oops: 0002 [#1] SMP NOPTI
> > [ 26.654512] CPU: 53 PID: 2614 Comm: systemd-cgroups Tainted: G
> > OE --------- - - 4.18.0-80.el8.x86_64 #1
> > [ 26.654512] Hardware name: Lenovo ThinkSystem SR850P
> > -[7D2H]-/-[7D2H]-, BIOS -[TEE145P-1.10]- 12/06/2019
> > [ 26.654519] RIP: 0010:selinux_socket_post_create+0x80/0x390
> > [ 26.654520] Code: e9 95 6a 89 00 bd 16 00 00 00 c7 44 24 04 01
> > 00 00 00 45 85 c0 0f 85 f6 00 00 00 8b 56 14 85 d2 0f 84 26 01 00
> > 00 89 54 24 04 <66> 41 89 6c 24 20 31 c0 41 89 54 24 1c 41 c6 44
> > 24 22 01 49 8b 4d
> > [ 26.654521] RSP: 0018:ffffbf515cc63e48 EFLAGS: 00010246
> > [ 26.654522] RAX: 0000000000000000 RBX: 0000000000000001 RCX:
> 0000000000000019
> > [ 26.654522] RDX: 0000000000000001 RSI: 0000000000000001 RDI:
> ffffffffab46f680
> > [ 26.654523] RBP: 0000000000000019 R08: 0000000000000000 R09:
> ffffbf515cc63e4c
> > [ 26.654523] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000000
> > [ 26.654524] R13: ffff97d7bb6cbc80 R14: 0000000000000001 R15:
> ffff97d7bb6cbc80
> > [ 26.654525] FS: 00007f5c608ea380(0000) GS:ffff97d7bf140000(0000)
> knlGS:0000000000000000
> > [ 26.654525] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 26.654526] CR2: 0000000000000020 CR3: 0000011ebc934004 CR4:
> 00000000007606e0
> > [ 26.654527] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> > [ 26.654528] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> > [ 26.654528] PKRU: 55555554
> > [ 26.654528] Call Trace:
> > [ 26.654535] security_socket_post_create+0x42/0x60
> > [ 26.654537] SELinux: Unregistering netfilter hooks
> > [ 26.654542] __sock_create+0x106/0x1a0
> > [ 26.654545] __sys_socket+0x57/0xe0
> > [ 26.654547] __x64_sys_socket+0x16/0x20
> > [ 26.654551] do_syscall_64+0x5b/0x1b0
> > [ 26.654554] entry_SYSCALL_64_after_hwframe+0x65/0xca
> >
> > The root cause is that, selinux_inode_alloc_security has been deleted
> > firstly from security_hook_heads, so security_inode_alloc directly
> > return 0, that means the value of pointer inode->i_security equalling
> > to NULL.
> >
> > But selinux_socket_post_create hasn't been deleted at that moment, so
> > which would involked by mistake. Inside the function, pointer isec
> > needs to point to inode->i_security, then a NULL pointer defect happens.
> >
> > For current upstream kernel, because of commit
> > afb1cbe37440c7f38b9cf46fc331cc9dfd5cce21
> > the inode security has been moved out to LSM infrastructure from
> > individual security modules like selinux.
> >
> > But this patch still can be applied for solving similar issue when
> > security_delete_hooks has been used. Also for stable branch v4.19, the
> > inode security still need to be created in individual modules.
>
> Thank you for the patch, however there are already existing proposed patches to
> fix this issue, see [1], [2], and [3]. At the moment it looks like the SELinux
> hooks reorder approach ([1]) will be accepted as a temporary solution (the SELinux
> runtime disable is being deprecated [4] in favor of properly disabling SELinux
> by setting
> selinux=0 on the kernel command line).
Hi Ondrej,
Thanks for introduction, and yes "selinux=0" is the simplest and effective workaround
for this defect.
I used it before as workaround and the panic issue couldn't be reproduced again.
>
> Your approach unfortunately isn't robust (depends on assumptions about how hooks
> are ordered by LSMs) nor complete (even the inverse order still has some race
> conditions that may lead to a crash - e.g.
> selinux_bpf_map() vs. selinux_bpf_map_alloc()).
>
> Also, please, don't forget to Cc the LSM/SELinux mailing lists
> (linux-security-module@vger.kernel.org/selinux@vger.kernel.org,
> respectively) for patches related to the LSM framework/SELinux.
Got it, I am new to selinux module.
Cheers,
Huaisheng Ye
Lenovo
>
> [1]
> https://lore.kernel.org/selinux/20191209075756.123157-1-omosnace@redhat.com/
> T/
> [2]
> https://lore.kernel.org/selinux/20191211140833.939845-1-omosnace@redhat.com/
> T/
> [3]
> https://lore.kernel.org/selinux/20200107133154.588958-1-omosnace@redhat.com/
> T/
> [4]
> https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?
> h=next&id=89b223bfb8a89731bea4c84982b5d2ad7ba460e3
>
> >
> > The patch has been verified by Lenovo SR850P server through overnight
> > reboot cycles.
> >
> > Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
> > ---
> > include/linux/lsm_hooks.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 20d8cf1..57cb2ac 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -2164,7 +2164,7 @@ static inline void security_delete_hooks(struct
> security_hook_list *hooks,
> > int i;
> >
> > for (i = 0; i < count; i++)
> > - hlist_del_rcu(&hooks[i].list);
> > + hlist_del_rcu(&hooks[count - 1 - i].list);
> > }
> > #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
> >
> > --
> > 1.8.3.1
> >
> >
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security
> Technologies Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Andy Lutomirski @ 2020-01-09 6:48 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: peterz@infradead.org, luto@kernel.org, Hansen, Dave,
nadav.amit@gmail.com, songliubraving@fb.com,
linux-kernel@vger.kernel.org, daniel@iogearbox.net,
keescook@chromium.org, jeyu@kernel.org, bpf@vger.kernel.org,
kuznet@ms2.inr.ac.ru, ast@kernel.org, mjg59@google.com,
thgarnie@chromium.org, kpsingh@chromium.org,
linux-security-module@vger.kernel.org, x86@kernel.org,
revest@chromium.org, jannh@google.com, namit@vmware.com,
jackmanb@chromium.org, kafai@fb.com, yhs@fb.com,
davem@davemloft.net, yoshfuji@linux-ipv6.org, mhalcrow@google.com,
andriin@fb.com
In-Reply-To: <400be86aab208d0e50a237cdbd3195763396e3ed.camel@intel.com>
> On Jan 8, 2020, at 10:52 AM, Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote:
>
> On Wed, 2020-01-08 at 00:41 -0800, Andy Lutomirski wrote:
>>> On Jan 7, 2020, at 9:01 AM, Edgecombe, Rick P <rick.p.edgecombe@intel.com>
>>> wrote:
>>>
>>> CC Nadav and Jessica.
>>>
>>> On Mon, 2020-01-06 at 15:36 -1000, Andy Lutomirski wrote:
>>>>> On Jan 6, 2020, at 12:25 PM, Edgecombe, Rick P <
>>>>> rick.p.edgecombe@intel.com>
>>>>> wrote:
>>>>>
>>>>> On Sat, 2020-01-04 at 09:49 +0900, Andy Lutomirski wrote:
>>>>>>>>> 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.
>>>>>
>>>>> So if I understand this right, some trampolines have been added that are
>>>>> currently set as RWX at modification time AND left that way during
>>>>> runtime?
>>>>> The
>>>>> discussion on the order of set_memory_() calls in the commit message
>>>>> made me
>>>>> think that this was just a modification time thing at first.
>>>>
>>>> I’m not sure what the status quo is.
>>>>
>>>> We really ought to have a genuinely good API for allocation and
>>>> initialization
>>>> of text. We can do so much better than set_memory_blahblah.
>>>>
>>>> FWIW, I have some ideas about making kernel flushes cheaper. It’s
>>>> currently
>>>> blocked on finding some time and on tglx’s irqtrace work.
>>>>
>>>
>>> Makes sense to me. I guess there are 6 types of text allocations now:
>>> - These two BPF trampolines
>>> - BPF JITs
>>> - Modules
>>> - Kprobes
>>> - Ftrace
>>>
>>> All doing (or should be doing) pretty much the same thing. I believe Jessica
>>> had
>>> said at one point that she didn't like all the other features using
>>> module_alloc() as it was supposed to be just for real modules. Where would
>>> the
>>> API live?
>>
>> New header? This shouldn’t matter that much.
>>
>> Here are two strawman proposals. All of this is very rough -- the
>> actual data structures and signatures are likely problematic for
>> multiple reasons.
>>
>> --- First proposal ---
>>
>> struct text_allocation {
>> void *final_addr;
>> struct page *pages;
>> int npages;
>> };
>>
>> int text_alloc(struct text_allocation *out, size_t size);
>>
>> /* now final_addr is not accessible and pages is writable. */
>>
>> int text_freeze(struct text_allocation *alloc);
>>
>> /* now pages are not accessible and final_addr is RO. Alternatively,
>> pages are RO and final_addr is unmapped. */
>>
>> int text_finish(struct text_allocation *alloc);
>>
>> /* now final_addr is RX. All done. */
>>
>> This gets it with just one flush and gives a chance to double-check in
>> case of race attacks from other CPUs. Double-checking is annoying,
>> though.
>>
>> --- Second proposal ---
>>
>> struct text_allocation {
>> void *final_addr;
>> /* lots of opaque stuff including an mm_struct */
>> /* optional: list of struct page, but this isn't obviously useful */
>> };
>>
>> int text_alloc(struct text_allocation *out, size_t size);
>>
>> /* Memory is allocated. There is no way to access it at all right
>> now. The memory is RO or not present in the direct map. */
>>
>> void __user *text_activate_mapping(struct text_allocation *out);
>>
>> /* Now the text is RW at *user* address given by return value.
>> Preemption is off if required by use_temporary_mm(). Real user memory
>> cannot be accessed. */
>>
>> void text_deactivate_mapping(struct text_allocation *alloc);
>>
>> /* Now the memory is inaccessible again. */
>>
>> void text_finalize(struct text_allocation *alloc);
>>
>> /* Now it's RX or XO at the final address. */
>>
>>
>> Pros of second approach:
>>
>> - Inherently immune to cross-CPU attack. No double-check.
>>
>> - If we ever implement a cache of non-direct-mapped, unaliased pages,
>> then it works with no flushes at all. We could even relax it a bit to
>> allow non-direct-mapped pages that may have RX / XO aliases but no W
>> aliases.
>>
>> - Can easily access without worrying about page boundaries.
>>
>> Cons:
>>
>> - The use of a temporary mm is annoying -- you can't copy from user
>> memory, for example.
>
> Probably the first proposal is better for usages where there is a signature that
> can be checked like modules, because you could more easily check the signature
> after the text is RO. I guess leaving the direct map as RO could work for the
> second option too. Both would probably require significant changes to module
> signature verification though.
This sounds complicated — for decent performance, we want to apply
alternatives before we make the text RO, at which point verifying the
signature is awkward at best.
>
> Just a minor point/clarification, but outside of an enhanced signed module case,
> I think the cross-CPU attack mitigation can't be full. For example, attacking
> the verified BPF byte code (which is apparently planned to no longer be RO), or
> the pointers being loaded into these trampolines. There is always going to be
> some writable source or pointer to the source, and unless there is a way to
> verify the end RO result, it's an un-winnable game of whack-a-mole to do it in
> full. Still the less exposed surfaces the better since the writes we are
> worrying about in this case are probably not fully arbitrary.
We could use hypervisor- or CR3-based protection. But I agree this is
tricky and not strictly on topic :)
>
> I don't see why it would be so bad to require copying data to the kernel before
> sending it through this process. Nothing copies to final allocation directly
> from userspace today, and from a perf perspective, how bad is an extra copy when
> we are saving TLB shootdowns? Are you thinking to protect the data that's being
> loaded from other CPUs?
Hmm. If there’s a way to make loading stall, then the cross-cpu attack
is a nice way to write shell code, so mitigating this has at least
some value.
>
> Otherwise, could we lazily clone/sync the original mm into the temporary one to
> allow this? (possibly totally misguided idea)
That involves allocating a virtual address at a safe position to make
this work. On architectures like s390, I don’t even know if this is
possible. Even on x86, it’s awkward. I think it’s easier to just say
that, while the temporary mapping is active, user memory is
inaccessible.
>
> FWIW, I really like the idea of a cache of unmapped or RO pages. Potentially
> several optimizations we could do there.
>
I guess we would track these pages by the maximum permissions than any
current or unmapped but unflushed alias has. This lets us get totally
unmapped or RO pages out of the cache. Or even RX — we could
potentially allocate, free, and reallocate text without flushing.
> If this API should be cross platform, we might want to abstract the copy itself
> as well, since other arch's might have non __user solutions for copying data in.
Agreed, although maybe all arches would use “user” mappings.
>
> Unless someone else wants to, I can probably take a look at a first cut of this
> after I get the current thing I'm working on out. Probably better to let the
> dust settle on the ftrace changes as well.
That would be great!
Do you know why the change_page_attr code currently does
vm_unmap_aliases? This is yet more extra expense. I assume the idea
is that, if we’re changing cache attributes on a non-self-snoop
machine, we need to kill stale aliases, and we should also kill them
if we’re reducing permissions. But we currently do it excessively.
We should also consider improving vm_unmap_aliases(). As a practical
matter, vm_unmap_aliases() often does a global flush, but it can't be
relied on. On the other hand, a global flush initiated for other
reasons won't tell the vmap code that aliases have been zapped.
If the locking is okay, we could maybe get away with zapping aliases
from the normal flush code. Alternatively, we could do something
lockless, e.g.:
atomic64_t kernel_tlb_gen, flushed_kernel_tlb_gen;
flush_tlb_kernel_range(), etc increment kernel_tlb_gen before flushing
and then update flushed_kernel_tlb_gen to match after flushing.
The vmap code immediately removes PTEs when unmaps occur (which it may
very well do right now -- I haven't checked) but also tracks the
kernel_tlb_gen associated with each record of an
unmapped-but-not-zapped area. Then we split vm_unmap_aliases() into a
variant that unmaps all aliases and a variant that merely promises to
unmap at least one alias. The former does what the current code does
except that it skips the IPI if all areas in question have tlb_gen <
flushed_kernel_tlb_gen. The latter clears all areas with tlb_gen <
flushed_kernel_tlb_gen and, if there weren't any, does
flush_tlb_kernel_range() and flushes everything.
(Major caveat: this is wrong for the case where
flush_tlb_kernel_range() only flushes some but not all of the kernel.
So this needs considerable work if it's actually going to me useful.
The plain old "take locks and clean up" approach might be a better
bet.)
--Andy
^ permalink raw reply
* Re: [PATCH v4 2/9] perf/core: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2020-01-09 11:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <20200108160713.GI2844@hirez.programming.kicks-ass.net>
On 08.01.2020 19:07, Peter Zijlstra wrote:
> On Wed, Dec 18, 2019 at 12:25:35PM +0300, Alexey Budankov wrote:
>>
>> Open access to perf_events monitoring for CAP_SYS_PERFMON privileged
>> processes. For backward compatibility reasons access to perf_events
>> subsystem remains open for CAP_SYS_ADMIN privileged processes but
>> CAP_SYS_ADMIN usage for secure perf_events monitoring is discouraged
>> with respect to CAP_SYS_PERFMON capability.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>> include/linux/perf_event.h | 6 +++---
>> kernel/events/core.c | 6 +++---
>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 34c7c6910026..f46acd69425f 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -1285,7 +1285,7 @@ static inline int perf_is_paranoid(void)
>>
>> static inline int perf_allow_kernel(struct perf_event_attr *attr)
>> {
>> - if (sysctl_perf_event_paranoid > 1 && !capable(CAP_SYS_ADMIN))
>> + if (sysctl_perf_event_paranoid > 1 && !perfmon_capable())
>> return -EACCES;
>>
>> return security_perf_event_open(attr, PERF_SECURITY_KERNEL);
>> @@ -1293,7 +1293,7 @@ static inline int perf_allow_kernel(struct perf_event_attr *attr)
>>
>> static inline int perf_allow_cpu(struct perf_event_attr *attr)
>> {
>> - if (sysctl_perf_event_paranoid > 0 && !capable(CAP_SYS_ADMIN))
>> + if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
>> return -EACCES;
>>
>> return security_perf_event_open(attr, PERF_SECURITY_CPU);
>> @@ -1301,7 +1301,7 @@ static inline int perf_allow_cpu(struct perf_event_attr *attr)
>>
>> static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
>> {
>> - if (sysctl_perf_event_paranoid > -1 && !capable(CAP_SYS_ADMIN))
>> + if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
>> return -EPERM;
>>
>> return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
>
> These are OK I suppose.
>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 059ee7116008..d9db414f2197 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -9056,7 +9056,7 @@ static int perf_kprobe_event_init(struct perf_event *event)
>> if (event->attr.type != perf_kprobe.type)
>> return -ENOENT;
>>
>> - if (!capable(CAP_SYS_ADMIN))
>> + if (!perfmon_capable())
>> return -EACCES;
>>
>> /*
>
> This one only allows attaching to already extant kprobes, right? It does
> not allow creation of kprobes.
This unblocks creation of local trace kprobes and uprobes by CAP_SYS_PERFMON
privileged process, exactly the same as for CAP_SYS_ADMIN privileged process.
>
>> @@ -9116,7 +9116,7 @@ static int perf_uprobe_event_init(struct perf_event *event)
>> if (event->attr.type != perf_uprobe.type)
>> return -ENOENT;
>>
>> - if (!capable(CAP_SYS_ADMIN))
>> + if (!perfmon_capable())
>> return -EACCES;
>>
>> /*
>
> Idem, I presume.
>
>> @@ -11157,7 +11157,7 @@ SYSCALL_DEFINE5(perf_event_open,
>> }
>>
>> if (attr.namespaces) {
>> - if (!capable(CAP_SYS_ADMIN))
>> + if (!perfmon_capable())
>> return -EACCES;
>> }
>
> And given we basically make the entire kernel observable with this CAP,
> busting namespaces shoulnd't be a problem either.
>
> So yeah, I suppose that works.
>
^ permalink raw reply
* [PATCH] ima: ima/lsm policy rule loading logic bug fixes
From: Janne Karhunen @ 2020-01-09 14:08 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Casey Schaufler, Konsta Karsisto
Keep the ima policy rules around from the beginning even
if they appear invalid at the time of loading, as they
may become active after the lsm policy load. In other
words, now the lsm and the ima can be initialized in any
order and the handling logic is the same as with the lsm
rule reload event.
Patch also fixes the rule re-use during the lsm policy
reload and makes some prints a bit more human readable.
Cc: Casey Schaufler <casey@schaufler-ca.com>
Reported-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
security/integrity/ima/ima_policy.c | 44 ++++++++++++++---------------
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a4dde9d575b2..4022c7736fc3 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -265,7 +265,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
{
struct ima_rule_entry *nentry;
- int i, result;
+ int i;
nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
if (!nentry)
@@ -279,7 +279,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
memset(nentry->lsm, 0, FIELD_SIZEOF(struct ima_rule_entry, lsm));
for (i = 0; i < MAX_LSM_RULES; i++) {
- if (!entry->lsm[i].rule)
+ if (!entry->lsm[i].args_p)
continue;
nentry->lsm[i].type = entry->lsm[i].type;
@@ -288,13 +288,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
if (!nentry->lsm[i].args_p)
goto out_err;
- result = security_filter_rule_init(nentry->lsm[i].type,
- Audit_equal,
- nentry->lsm[i].args_p,
- &nentry->lsm[i].rule);
- if (result == -EINVAL)
- pr_warn("ima: rule for LSM \'%d\' is undefined\n",
- entry->lsm[i].type);
+ security_filter_rule_init(nentry->lsm[i].type,
+ Audit_equal,
+ nentry->lsm[i].args_p,
+ &nentry->lsm[i].rule);
+ if (!nentry->lsm[i].rule)
+ pr_warn("rule for LSM \'%s\' is undefined\n",
+ (char *)entry->lsm[i].args_p);
}
return nentry;
@@ -331,7 +331,9 @@ 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 (entry->lsm[i].args_p) {
+ pr_info("rule for LSM \'%s\' needs update\n",
+ (char *)entry->lsm[i].args_p);
needs_update = 1;
break;
}
@@ -341,8 +343,7 @@ static void ima_lsm_update_rules(void)
result = ima_lsm_update_rule(entry);
if (result) {
- pr_err("ima: lsm rule update error %d\n",
- result);
+ pr_err("lsm rule update error %d\n", result);
return;
}
}
@@ -865,8 +866,6 @@ static const match_table_t policy_tokens = {
static int ima_lsm_rule_init(struct ima_rule_entry *entry,
substring_t *args, int lsm_rule, int audit_type)
{
- int result;
-
if (entry->lsm[lsm_rule].rule)
return -EINVAL;
@@ -875,16 +874,15 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
return -ENOMEM;
entry->lsm[lsm_rule].type = audit_type;
- 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) {
- kfree(entry->lsm[lsm_rule].args_p);
- return -EINVAL;
- }
+ 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)
+ pr_warn("rule for LSM \'%s\' is undefined\n",
+ (char *)entry->lsm[lsm_rule].args_p);
- return result;
+ return 0;
}
static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v13 19/25] NET: Store LSM netlabel data in a lsmblob
From: Stephen Smalley @ 2020-01-09 14:34 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191224235939.7483-20-casey@schaufler-ca.com>
On 12/24/19 6:59 PM, Casey Schaufler wrote:
> Netlabel uses LSM interfaces requiring an lsmblob and
> the internal storage is used to pass information between
> these interfaces, so change the internal data from a secid
> to a lsmblob. Update the netlabel interfaces and their
> callers to accommodate the change. This requires that the
> modules using netlabel use the lsm_id.slot to access the
> correct secid when using netlabel.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> include/net/netlabel.h | 8 ++--
> net/ipv4/cipso_ipv4.c | 23 +++++++-----
> net/netlabel/netlabel_kapi.c | 6 +--
> net/netlabel/netlabel_unlabeled.c | 57 +++++++++++------------------
> net/netlabel/netlabel_unlabeled.h | 2 +-
> security/selinux/hooks.c | 2 +-
> security/selinux/include/security.h | 1 +
> security/selinux/netlabel.c | 2 +-
> security/selinux/ss/services.c | 4 +-
> security/smack/smack.h | 1 +
> security/smack/smack_lsm.c | 5 ++-
> security/smack/smackfs.c | 10 +++--
> 12 files changed, 59 insertions(+), 62 deletions(-)
>
> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
> index 43ae50337685..73fc25b4042b 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -166,7 +166,7 @@ struct netlbl_lsm_catmap {
> * @attr.mls: MLS sensitivity label
> * @attr.mls.cat: MLS category bitmap
> * @attr.mls.lvl: MLS sensitivity level
> - * @attr.secid: LSM specific secid token
> + * @attr.lsmblob: LSM specific data
> *
> * Description:
> * This structure is used to pass security attributes between NetLabel and the
> @@ -201,7 +201,7 @@ struct netlbl_lsm_secattr {
> struct netlbl_lsm_catmap *cat;
> u32 lvl;
> } mls;
> - u32 secid;
> + struct lsmblob lsmblob;
> } attr;
> };
>
> @@ -415,7 +415,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_cfg_unlbl_static_del(struct net *net,
> const char *dev_name,
> @@ -523,7 +523,7 @@ static inline int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> return -ENOSYS;
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 376882215919..adb9dffc3952 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -106,15 +106,17 @@ int cipso_v4_rbm_strictvalid = 1;
> /* Base length of the local tag (non-standard tag).
> * Tag definition (may change between kernel versions)
> *
> - * 0 8 16 24 32
> - * +----------+----------+----------+----------+
> - * | 10000000 | 00000110 | 32-bit secid value |
> - * +----------+----------+----------+----------+
> - * | in (host byte order)|
> - * +----------+----------+
> - *
> + * 0 8 16 16 + sizeof(struct lsmblob)
> + * +----------+----------+---------------------+
> + * | 10000000 | 00000110 | LSM blob data |
> + * +----------+----------+---------------------+
> + *
> + * All secid and flag fields are in host byte order.
> + * The lsmblob structure size varies depending on which
> + * Linux security modules are built in the kernel.
> + * The data is opaque.
> */
> -#define CIPSO_V4_TAG_LOC_BLEN 6
> +#define CIPSO_V4_TAG_LOC_BLEN (2 + sizeof(struct lsmblob))
>
> /*
> * Helper Functions
> @@ -1467,7 +1469,8 @@ static int cipso_v4_gentag_loc(const struct cipso_v4_doi *doi_def,
>
> buffer[0] = CIPSO_V4_TAG_LOCAL;
> buffer[1] = CIPSO_V4_TAG_LOC_BLEN;
> - *(u32 *)&buffer[2] = secattr->attr.secid;
> + memcpy(&buffer[2], &secattr->attr.lsmblob,
> + sizeof(secattr->attr.lsmblob));
>
> return CIPSO_V4_TAG_LOC_BLEN;
> }
> @@ -1487,7 +1490,7 @@ static int cipso_v4_parsetag_loc(const struct cipso_v4_doi *doi_def,
> const unsigned char *tag,
> struct netlbl_lsm_secattr *secattr)
> {
> - secattr->attr.secid = *(u32 *)&tag[2];
> + memcpy(&secattr->attr.lsmblob, &tag[2], sizeof(secattr->attr.lsmblob));
> secattr->flags |= NETLBL_SECATTR_SECID;
>
> return 0;
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 409a3ae47ce2..f2ebd43a7992 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -196,7 +196,7 @@ int netlbl_cfg_unlbl_map_add(const char *domain,
> * @addr: IP address in network byte order (struct in[6]_addr)
> * @mask: address mask in network byte order (struct in[6]_addr)
> * @family: address family
> - * @secid: LSM secid value for the entry
> + * @lsmblob: LSM data value for the entry
> * @audit_info: NetLabel audit information
> *
> * Description:
> @@ -210,7 +210,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> u32 addr_len;
> @@ -230,7 +230,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
>
> return netlbl_unlhsh_add(net,
> dev_name, addr, mask, addr_len,
> - secid, audit_info);
> + lsmblob, audit_info);
> }
>
> /**
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index c03fe9a4f7b9..3b0f07b59436 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -66,7 +66,7 @@ struct netlbl_unlhsh_tbl {
> #define netlbl_unlhsh_addr4_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr4, list)
> struct netlbl_unlhsh_addr4 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af4list list;
> struct rcu_head rcu;
> @@ -74,7 +74,7 @@ struct netlbl_unlhsh_addr4 {
> #define netlbl_unlhsh_addr6_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr6, list)
> struct netlbl_unlhsh_addr6 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af6list list;
> struct rcu_head rcu;
> @@ -219,7 +219,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> * @iface: the associated interface entry
> * @addr: IPv4 address in network byte order
> * @mask: IPv4 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -230,7 +230,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> const struct in_addr *addr,
> const struct in_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr4 *entry;
> @@ -242,7 +242,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> entry->list.addr = addr->s_addr & mask->s_addr;
> entry->list.mask = mask->s_addr;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af4list_add(&entry->list, &iface->addr4_list);
> @@ -259,7 +259,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> * @iface: the associated interface entry
> * @addr: IPv6 address in network byte order
> * @mask: IPv6 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -270,7 +270,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> const struct in6_addr *addr,
> const struct in6_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr6 *entry;
> @@ -286,7 +286,7 @@ static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> entry->list.addr.s6_addr32[3] &= mask->s6_addr32[3];
> entry->list.mask = *mask;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af6list_add(&entry->list, &iface->addr6_list);
> @@ -365,7 +365,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> int ret_val;
> @@ -374,7 +374,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> if (addr_len != sizeof(struct in_addr) &&
> addr_len != sizeof(struct in6_addr))
> @@ -407,7 +406,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in_addr *addr4 = addr;
> const struct in_addr *mask4 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, secid);
> + ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, lsmblob);
> if (audit_buf != NULL)
> netlbl_af4list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -420,7 +419,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in6_addr *addr6 = addr;
> const struct in6_addr *mask6 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, secid);
> + ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, lsmblob);
> if (audit_buf != NULL)
> netlbl_af6list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -437,8 +436,7 @@ int netlbl_unlhsh_add(struct net *net,
> unlhsh_add_return:
> rcu_read_unlock();
> if (audit_buf != NULL) {
> - lsmblob_init(&blob, secid);
> - if (security_secid_to_secctx(&blob, &context) == 0) {
> + if (security_secid_to_secctx(lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -473,7 +471,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af4list_remove(addr->s_addr, mask->s_addr,
> @@ -493,10 +490,8 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> addr->s_addr, mask->s_addr);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -537,7 +532,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af6list_remove(addr, mask, &iface->addr6_list);
> @@ -556,10 +550,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> addr, mask);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -913,9 +905,8 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, blob.secid[0],
> + dev_name, addr, mask, addr_len, &blob,
> &audit_info);
> }
>
> @@ -963,10 +954,8 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - NULL, addr, mask, addr_len, blob.secid[0],
> - &audit_info);
> + NULL, addr, mask, addr_len, &blob, &audit_info);
> }
>
> /**
> @@ -1078,8 +1067,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct net_device *dev;
> struct lsmcontext context;
> void *data;
> - u32 secid;
> - struct lsmblob blob;
> + struct lsmblob *lsmb;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> cb_arg->seq, &netlbl_unlabel_gnl_family,
> @@ -1117,7 +1105,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr4->secid;
> + lsmb = (struct lsmblob *)&addr4->lsmblob;
> } else {
> ret_val = nla_put_in6_addr(cb_arg->skb,
> NLBL_UNLABEL_A_IPV6ADDR,
> @@ -1131,11 +1119,10 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr6->secid;
> + lsmb = (struct lsmblob *)&addr6->lsmblob;
> }
>
> - lsmblob_init(&blob, secid);
> - ret_val = security_secid_to_secctx(&blob, &context);
> + ret_val = security_secid_to_secctx(lsmb, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> @@ -1487,7 +1474,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr4_list);
> if (addr4 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr4_entry(addr4)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr4_entry(addr4)->lsmblob;
> break;
> }
> #if IS_ENABLED(CONFIG_IPV6)
> @@ -1500,7 +1487,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr6_list);
> if (addr6 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr6_entry(addr6)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr6_entry(addr6)->lsmblob;
> break;
> }
> #endif /* IPv6 */
> diff --git a/net/netlabel/netlabel_unlabeled.h b/net/netlabel/netlabel_unlabeled.h
> index 058e3a285d56..168920780994 100644
> --- a/net/netlabel/netlabel_unlabeled.h
> +++ b/net/netlabel/netlabel_unlabeled.h
> @@ -211,7 +211,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_unlhsh_remove(struct net *net,
> const char *dev_name,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b8501ca3c8f3..cd4743331800 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6871,7 +6871,7 @@ static int selinux_perf_event_write(struct perf_event *event)
> }
> #endif
>
> -static struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> +struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> .lsm = "selinux",
> .slot = LSMBLOB_NEEDED
> };
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ae840634e3c7..741ba0e6dd83 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -70,6 +70,7 @@
> struct netlbl_lsm_secattr;
>
> extern int selinux_enabled;
> +extern struct lsm_id selinux_lsmid;
>
> /* Policy capabilities */
> enum {
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 6a94b31b5472..d8d7603ab14e 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -108,7 +108,7 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_getattr(
> return NULL;
>
> if ((secattr->flags & NETLBL_SECATTR_SECID) &&
> - (secattr->attr.secid == sid))
> + (secattr->attr.lsmblob.secid[selinux_lsmid.slot] == sid))
> return secattr;
>
> return NULL;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index a5813c7629c1..2b7680903b6b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3599,7 +3599,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
> if (secattr->flags & NETLBL_SECATTR_CACHE)
> *sid = *(u32 *)secattr->cache->data;
> else if (secattr->flags & NETLBL_SECATTR_SECID)
> - *sid = secattr->attr.secid;
> + *sid = secattr->attr.lsmblob.secid[selinux_lsmid.slot];
> else if (secattr->flags & NETLBL_SECATTR_MLS_LVL) {
> rc = -EIDRM;
> ctx = sidtab_search(sidtab, SECINITSID_NETMSG);
> @@ -3672,7 +3672,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> if (secattr->domain == NULL)
> goto out;
>
> - secattr->attr.secid = sid;
> + secattr->attr.lsmblob.secid[selinux_lsmid.slot] = sid;
> secattr->flags |= NETLBL_SECATTR_DOMAIN_CPY | NETLBL_SECATTR_SECID;
> mls_export_netlbl_lvl(policydb, ctx, secattr);
> rc = mls_export_netlbl_cat(policydb, ctx, secattr);
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 2836540f9577..6e76b6b33063 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -316,6 +316,7 @@ void smk_destroy_label_list(struct list_head *list);
> * Shared data.
> */
> extern int smack_enabled;
> +extern struct lsm_id smack_lsmid;
> extern int smack_cipso_direct;
> extern int smack_cipso_mapped;
> extern struct smack_known *smack_net_ambient;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 9737ead06b39..9ce67e03ac49 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3775,7 +3775,8 @@ static struct smack_known *smack_from_secattr(struct netlbl_lsm_secattr *sap,
> /*
> * Looks like a fallback, which gives us a secid.
> */
> - return smack_from_secid(sap->attr.secid);
> + return smack_from_secid(
> + sap->attr.lsmblob.secid[smack_lsmid.slot]);
> /*
> * Without guidance regarding the smack value
> * for the packet fall back on the network
> @@ -4592,7 +4593,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_sock = sizeof(struct socket_smack),
> };
>
> -static struct lsm_id smack_lsmid __lsm_ro_after_init = {
> +struct lsm_id smack_lsmid __lsm_ro_after_init = {
> .lsm = "smack",
> .slot = LSMBLOB_NEEDED
> };
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index e3e05c04dbd1..d10e9c96717e 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1122,6 +1122,7 @@ static void smk_net4addr_insert(struct smk_net4addr *new)
> static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> + struct lsmblob lsmblob;
> struct smk_net4addr *snp;
> struct sockaddr_in newname;
> char *smack;
> @@ -1253,10 +1254,13 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> * this host so that incoming packets get labeled.
> * but only if we didn't get the special CIPSO option
> */
> - if (rc == 0 && skp != NULL)
> + if (rc == 0 && skp != NULL) {
> + lsmblob_init(&lsmblob, 0);
> + lsmblob.secid[smack_lsmid.slot] = snp->smk_label->smk_secid;
> rc = netlbl_cfg_unlbl_static_add(&init_net, NULL,
> - &snp->smk_host, &snp->smk_mask, PF_INET,
> - snp->smk_label->smk_secid, &audit_info);
> + &snp->smk_host, &snp->smk_mask, PF_INET, &lsmblob,
> + &audit_info);
> + }
>
> if (rc == 0)
> rc = count;
>
^ permalink raw reply
* Re: [PATCH] ima: ima/lsm policy rule loading logic bug fixes
From: Mimi Zohar @ 2020-01-09 14:54 UTC (permalink / raw)
To: Janne Karhunen, linux-integrity, linux-security-module
Cc: Casey Schaufler, Konsta Karsisto
In-Reply-To: <20200109140821.17902-1-janne.karhunen@gmail.com>
On Thu, 2020-01-09 at 16:08 +0200, Janne Karhunen wrote:
> Keep the ima policy rules around from the beginning even
> if they appear invalid at the time of loading, as they
> may become active after the lsm policy load. In other
> words, now the lsm and the ima can be initialized in any
> order and the handling logic is the same as with the lsm
> rule reload event.
>
> Patch also fixes the rule re-use during the lsm policy
> reload and makes some prints a bit more human readable.
Thanks, Janne. What do you think about adding a single sentence at
the end of this patch description? Something along the lines of,
"With these changes, there no need to defer loading a custom IMA
policy, based on LSM rules, until after the LSM policy has been
initialized."
The line length, here, is a bit short. According to section "14) the
canonical path format" of Documentation/process/submitting-
patches.rst, the body of the explanation shouldl be line wrapped at 75
columns.
>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Reported-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
Please include a "Fixes" tag as well. Otherwise,
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH v13 26/25] Audit: Multiple LSM support in audit rules
From: Mimi Zohar @ 2020-01-09 16:33 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, sds,
linux-audit@redhat.com, linux-integrity
In-Reply-To: <ee5e4cea-b6c1-fa12-30de-8fc9007d69e9@schaufler-ca.com>
Hi Casey,
On Fri, 2020-01-03 at 10:53 -0800, Casey Schaufler wrote:
> 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.
While reviewing this patch, I realized there was a bug in the base IMA
code. With Janne's bug fix, that he just posted, I think this patch
can now be simplified.
My main concern is the number of warning messages that will be
generated. Any time a new LSM policy is loaded, the labels will be
re-evaulated whether or not they are applicable to the particular LSM,
causing unnecessary warnings.
Mimi
^ permalink raw reply
* Re: [PATCH bpf-next v1 12/13] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
From: Andrii Nakryiko @ 2020-01-09 17:59 UTC (permalink / raw)
To: KP Singh
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: <20200104000955.GB23487@chromium.org>
On Fri, Jan 3, 2020 at 4:09 PM KP Singh <kpsingh@chromium.org> wrote:
>
> 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.
Yes, ideally all common types should be shared in a common header. My
initial implementation actually supported dumping out all the types in
a generated skeleton, but that was inconvenient in a lot of cases, so
I dropped that.
>
> - KP
>
> >
> > > + return 0;
> > > +}
> > > --
> > > 2.20.1
> > >
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: James Morris @ 2020-01-09 18:11 UTC (permalink / raw)
To: Stephen Smalley
Cc: Kees Cook, KP Singh, Casey Schaufler, open list, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <e59607cc-1a84-cbdd-5117-7efec86b11ff@tycho.nsa.gov>
On Wed, 8 Jan 2020, Stephen Smalley wrote:
> The cover letter subject line and the Kconfig help text refer to it as a
> BPF-based "MAC and Audit policy". It has an enforce config option that
> enables the bpf programs to deny access, providing access control. IIRC, in
> the earlier discussion threads, the BPF maintainers suggested that Smack and
> other LSMs could be entirely re-implemented via it in the future, and that
> such an implementation would be more optimal.
In this case, the eBPF code is similar to a kernel module, rather than a
loadable policy file. It's a loadable mechanism, rather than a policy, in
my view.
This would be similar to the difference between iptables rules and
loadable eBPF networking code. I'd be interested to know how the
eBPF networking scenarios are handled wrt kernel ABI.
> Again, not arguing for or against, but wondering if people fully understand
> the implications. If it ends up being useful, people will build access
> control systems with it, and it directly exposes a lot of kernel internals to
> userspace. There was a lot of concern originally about the LSM hook interface
> becoming a stable ABI and/or about it being misused. Exposing that interface
> along with every kernel data structure exposed through it to userspace seems
> like a major leap.
Agreed this is a leap, although I'm not sure I'd characterize it as
exposure to userspace -- it allows dynamic extension of the LSM API from
userland, but the code is executed in the kernel.
KP: One thing I'd like to understand better is the attack surface
introduced by this. IIUC, the BTF fields are read only, so the eBPF code
should not be able to modify any LSM parameters, correct?
> Even if the mainline kernel doesn't worry about any kind
> of stable interface guarantees for it, the distros might be forced to provide
> some kABI guarantees for it to appease ISVs and users...
How is this handled currently for other eBPF use-cases?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: Greg Kroah-Hartman @ 2020-01-09 18:23 UTC (permalink / raw)
To: James Morris
Cc: Stephen Smalley, Kees Cook, KP Singh, Casey Schaufler, open list,
bpf, linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer,
Paul Moore
In-Reply-To: <alpine.LRH.2.21.2001100437550.21515@namei.org>
On Fri, Jan 10, 2020 at 05:11:38AM +1100, James Morris wrote:
> On Wed, 8 Jan 2020, Stephen Smalley wrote:
>
> > The cover letter subject line and the Kconfig help text refer to it as a
> > BPF-based "MAC and Audit policy". It has an enforce config option that
> > enables the bpf programs to deny access, providing access control. IIRC, in
> > the earlier discussion threads, the BPF maintainers suggested that Smack and
> > other LSMs could be entirely re-implemented via it in the future, and that
> > such an implementation would be more optimal.
>
> In this case, the eBPF code is similar to a kernel module, rather than a
> loadable policy file. It's a loadable mechanism, rather than a policy, in
> my view.
>
> This would be similar to the difference between iptables rules and
> loadable eBPF networking code. I'd be interested to know how the
> eBPF networking scenarios are handled wrt kernel ABI.
I already know of some people who pre-compile ebpf programs based on a
number of "supported" kernel versions and then load the needed one at
runtime.
Messy, yes, but you are right, ebpf code is much more similiar to a
kernel module than userspace code at the moment.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: Stephen Smalley @ 2020-01-09 18:58 UTC (permalink / raw)
To: James Morris
Cc: Kees Cook, KP Singh, Casey Schaufler, open list, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <alpine.LRH.2.21.2001100437550.21515@namei.org>
On 1/9/20 1:11 PM, James Morris wrote:
> On Wed, 8 Jan 2020, Stephen Smalley wrote:
>
>> The cover letter subject line and the Kconfig help text refer to it as a
>> BPF-based "MAC and Audit policy". It has an enforce config option that
>> enables the bpf programs to deny access, providing access control. IIRC, in
>> the earlier discussion threads, the BPF maintainers suggested that Smack and
>> other LSMs could be entirely re-implemented via it in the future, and that
>> such an implementation would be more optimal.
>
> In this case, the eBPF code is similar to a kernel module, rather than a
> loadable policy file. It's a loadable mechanism, rather than a policy, in
> my view.
I thought you frowned on dynamically loadable LSMs for both security and
correctness reasons? And a traditional security module would necessarily
fall under GPL; is the eBPF code required to be likewise? If not, KRSI
is a gateway for proprietary LSMs...
> This would be similar to the difference between iptables rules and
> loadable eBPF networking code. I'd be interested to know how the
> eBPF networking scenarios are handled wrt kernel ABI.
>
>
>> Again, not arguing for or against, but wondering if people fully understand
>> the implications. If it ends up being useful, people will build access
>> control systems with it, and it directly exposes a lot of kernel internals to
>> userspace. There was a lot of concern originally about the LSM hook interface
>> becoming a stable ABI and/or about it being misused. Exposing that interface
>> along with every kernel data structure exposed through it to userspace seems
>> like a major leap.
>
> Agreed this is a leap, although I'm not sure I'd characterize it as
> exposure to userspace -- it allows dynamic extension of the LSM API from
> userland, but the code is executed in the kernel.
>
> KP: One thing I'd like to understand better is the attack surface
> introduced by this. IIUC, the BTF fields are read only, so the eBPF code
> should not be able to modify any LSM parameters, correct?
>
>
>> Even if the mainline kernel doesn't worry about any kind
>> of stable interface guarantees for it, the distros might be forced to provide
>> some kABI guarantees for it to appease ISVs and users...
>
> How is this handled currently for other eBPF use-cases?
>
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: James Morris @ 2020-01-09 19:07 UTC (permalink / raw)
To: Stephen Smalley
Cc: Kees Cook, KP Singh, Casey Schaufler, open list, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <e90e03e3-b92f-6e1a-132f-1b648d9d2139@tycho.nsa.gov>
On Thu, 9 Jan 2020, Stephen Smalley wrote:
> On 1/9/20 1:11 PM, James Morris wrote:
> > On Wed, 8 Jan 2020, Stephen Smalley wrote:
> >
> > > The cover letter subject line and the Kconfig help text refer to it as a
> > > BPF-based "MAC and Audit policy". It has an enforce config option that
> > > enables the bpf programs to deny access, providing access control. IIRC,
> > > in
> > > the earlier discussion threads, the BPF maintainers suggested that Smack
> > > and
> > > other LSMs could be entirely re-implemented via it in the future, and that
> > > such an implementation would be more optimal.
> >
> > In this case, the eBPF code is similar to a kernel module, rather than a
> > loadable policy file. It's a loadable mechanism, rather than a policy, in
> > my view.
>
> I thought you frowned on dynamically loadable LSMs for both security and
> correctness reasons?
Evaluating the security impact of this is the next step. My understanding
is that eBPF via BTF is constrained to read only access to hook
parameters, and that its behavior would be entirely restrictive.
I'd like to understand the security impact more fully, though. Can the
eBPF code make arbitrary writes to the kernel, or read anything other than
the correctly bounded LSM hook parameters?
> And a traditional security module would necessarily fall
> under GPL; is the eBPF code required to be likewise? If not, KRSI is a
> gateway for proprietary LSMs...
Right, we do not want this to be a GPL bypass.
If these issues can be resolved, this may be a "safe" way to support
loadable LSM applications.
Again, I'd be interested in knowing how this is is handled in the
networking stack (keeping in mind that LSM is a much more invasive API,
and may not be directly comparable).
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: KP Singh @ 2020-01-09 19:11 UTC (permalink / raw)
To: James Morris
Cc: Stephen Smalley, Kees Cook, Casey Schaufler, open list, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <alpine.LRH.2.21.2001100437550.21515@namei.org>
On 10-Jan 05:11, James Morris wrote:
> On Wed, 8 Jan 2020, Stephen Smalley wrote:
>
> > The cover letter subject line and the Kconfig help text refer to it as a
> > BPF-based "MAC and Audit policy". It has an enforce config option that
> > enables the bpf programs to deny access, providing access control. IIRC, in
> > the earlier discussion threads, the BPF maintainers suggested that Smack and
> > other LSMs could be entirely re-implemented via it in the future, and that
> > such an implementation would be more optimal.
>
> In this case, the eBPF code is similar to a kernel module, rather than a
> loadable policy file. It's a loadable mechanism, rather than a policy, in
> my view.
>
> This would be similar to the difference between iptables rules and
> loadable eBPF networking code. I'd be interested to know how the
> eBPF networking scenarios are handled wrt kernel ABI.
>
>
> > Again, not arguing for or against, but wondering if people fully understand
> > the implications. If it ends up being useful, people will build access
> > control systems with it, and it directly exposes a lot of kernel internals to
> > userspace. There was a lot of concern originally about the LSM hook interface
> > becoming a stable ABI and/or about it being misused. Exposing that interface
> > along with every kernel data structure exposed through it to userspace seems
> > like a major leap.
>
> Agreed this is a leap, although I'm not sure I'd characterize it as
> exposure to userspace -- it allows dynamic extension of the LSM API from
> userland, but the code is executed in the kernel.
>
> KP: One thing I'd like to understand better is the attack surface
> introduced by this. IIUC, the BTF fields are read only, so the eBPF code
> should not be able to modify any LSM parameters, correct?
>
That's correct, the verifier does not allow writes to BTF types:
from kernel/bpf/verifier.c:
case PTR_TO_BTF_ID:
if (type == BPF_WRITE) {
verbose(env, "Writes through BTF pointers are not allowed\n");
return -EINVAL;
}
We can also add additional checks on top of those added by the
verifier using the verifier_ops that each BPF program type can define.
- KP
>
> > Even if the mainline kernel doesn't worry about any kind
> > of stable interface guarantees for it, the distros might be forced to provide
> > some kABI guarantees for it to appease ISVs and users...
>
> How is this handled currently for other eBPF use-cases?
>
> --
> James Morris
> <jmorris@namei.org>
>
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: KP Singh @ 2020-01-09 19:43 UTC (permalink / raw)
To: James Morris
Cc: Stephen Smalley, Kees Cook, KP Singh, Casey Schaufler, open list,
bpf, linux-security-module, Alexei Starovoitov, Daniel Borkmann,
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, Paul Moore
In-Reply-To: <alpine.LRH.2.21.2001100558550.31925@namei.org>
On 10-Jan 06:07, James Morris wrote:
> On Thu, 9 Jan 2020, Stephen Smalley wrote:
>
> > On 1/9/20 1:11 PM, James Morris wrote:
> > > On Wed, 8 Jan 2020, Stephen Smalley wrote:
> > >
> > > > The cover letter subject line and the Kconfig help text refer to it as a
> > > > BPF-based "MAC and Audit policy". It has an enforce config option that
> > > > enables the bpf programs to deny access, providing access control. IIRC,
> > > > in
> > > > the earlier discussion threads, the BPF maintainers suggested that Smack
> > > > and
> > > > other LSMs could be entirely re-implemented via it in the future, and that
> > > > such an implementation would be more optimal.
> > >
> > > In this case, the eBPF code is similar to a kernel module, rather than a
> > > loadable policy file. It's a loadable mechanism, rather than a policy, in
> > > my view.
> >
> > I thought you frowned on dynamically loadable LSMs for both security and
> > correctness reasons?
Based on the feedback from the lists we've updated the design for v2.
In v2, LSM hook callbacks are allocated dynamically using BPF
trampolines, appended to a separate security_hook_heads and run
only after the statically allocated hooks.
The security_hook_heads for all the other LSMs (SELinux, AppArmor etc)
still remains __lsm_ro_after_init and cannot be modified. We are still
working on v2 (not ready for review yet) but the general idea can be
seen here:
https://github.com/sinkap/linux-krsi/blob/patch/v1/trampoline_prototype/security/bpf/lsm.c
>
> Evaluating the security impact of this is the next step. My understanding
> is that eBPF via BTF is constrained to read only access to hook
> parameters, and that its behavior would be entirely restrictive.
>
> I'd like to understand the security impact more fully, though. Can the
> eBPF code make arbitrary writes to the kernel, or read anything other than
> the correctly bounded LSM hook parameters?
>
As mentioned, the BPF verifier does not allow writes to BTF types.
> > And a traditional security module would necessarily fall
> > under GPL; is the eBPF code required to be likewise? If not, KRSI is a
> > gateway for proprietary LSMs...
>
> Right, we do not want this to be a GPL bypass.
This is not intended to be a GPL bypass and the BPF verifier checks
for license compatibility of the loaded program with GPL.
- KP
>
> If these issues can be resolved, this may be a "safe" way to support
> loadable LSM applications.
>
> Again, I'd be interested in knowing how this is is handled in the
> networking stack (keeping in mind that LSM is a much more invasive API,
> and may not be directly comparable).
>
> --
> James Morris
> <jmorris@namei.org>
>
^ permalink raw reply
* Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
From: Stephen Smalley @ 2020-01-09 19:47 UTC (permalink / raw)
To: KP Singh, James Morris
Cc: Kees Cook, Casey Schaufler, open list, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, 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,
Paul Moore
In-Reply-To: <20200109194302.GA85350@google.com>
On 1/9/20 2:43 PM, KP Singh wrote:
> On 10-Jan 06:07, James Morris wrote:
>> On Thu, 9 Jan 2020, Stephen Smalley wrote:
>>
>>> On 1/9/20 1:11 PM, James Morris wrote:
>>>> On Wed, 8 Jan 2020, Stephen Smalley wrote:
>>>>
>>>>> The cover letter subject line and the Kconfig help text refer to it as a
>>>>> BPF-based "MAC and Audit policy". It has an enforce config option that
>>>>> enables the bpf programs to deny access, providing access control. IIRC,
>>>>> in
>>>>> the earlier discussion threads, the BPF maintainers suggested that Smack
>>>>> and
>>>>> other LSMs could be entirely re-implemented via it in the future, and that
>>>>> such an implementation would be more optimal.
>>>>
>>>> In this case, the eBPF code is similar to a kernel module, rather than a
>>>> loadable policy file. It's a loadable mechanism, rather than a policy, in
>>>> my view.
>>>
>>> I thought you frowned on dynamically loadable LSMs for both security and
>>> correctness reasons?
>
> Based on the feedback from the lists we've updated the design for v2.
>
> In v2, LSM hook callbacks are allocated dynamically using BPF
> trampolines, appended to a separate security_hook_heads and run
> only after the statically allocated hooks.
>
> The security_hook_heads for all the other LSMs (SELinux, AppArmor etc)
> still remains __lsm_ro_after_init and cannot be modified. We are still
> working on v2 (not ready for review yet) but the general idea can be
> seen here:
>
> https://github.com/sinkap/linux-krsi/blob/patch/v1/trampoline_prototype/security/bpf/lsm.c
>
>>
>> Evaluating the security impact of this is the next step. My understanding
>> is that eBPF via BTF is constrained to read only access to hook
>> parameters, and that its behavior would be entirely restrictive.
>>
>> I'd like to understand the security impact more fully, though. Can the
>> eBPF code make arbitrary writes to the kernel, or read anything other than
>> the correctly bounded LSM hook parameters?
>>
>
> As mentioned, the BPF verifier does not allow writes to BTF types.
>
>>> And a traditional security module would necessarily fall
>>> under GPL; is the eBPF code required to be likewise? If not, KRSI is a
>>> gateway for proprietary LSMs...
>>
>> Right, we do not want this to be a GPL bypass.
>
> This is not intended to be a GPL bypass and the BPF verifier checks
> for license compatibility of the loaded program with GPL.
IIUC, it checks that the program is GPL compatible if it uses a function
marked GPL-only. But what specifically is marked GPL-only that is
required for eBPF programs using KRSI?
>
> - KP
>
>>
>> If these issues can be resolved, this may be a "safe" way to support
>> loadable LSM applications.
>>
>> Again, I'd be interested in knowing how this is is handled in the
>> networking stack (keeping in mind that LSM is a much more invasive API,
>> and may not be directly comparable).
>>
>> --
>> James Morris
>> <jmorris@namei.org>
>>
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Edgecombe, Rick P @ 2020-01-10 1:00 UTC (permalink / raw)
To: luto@kernel.org
Cc: songliubraving@fb.com, linux-kernel@vger.kernel.org,
daniel@iogearbox.net, peterz@infradead.org, keescook@chromium.org,
bpf@vger.kernel.org, jeyu@kernel.org, kuznet@ms2.inr.ac.ru,
mjg59@google.com, nadav.amit@gmail.com, ast@kernel.org,
thgarnie@chromium.org, kpsingh@chromium.org,
linux-security-module@vger.kernel.org, x86@kernel.org,
revest@chromium.org, jannh@google.com, namit@vmware.com,
jackmanb@chromium.org, Hansen, Dave, kafai@fb.com, yhs@fb.com,
davem@davemloft.net, yoshfuji@linux-ipv6.org, mhalcrow@google.com,
andriin@fb.com
In-Reply-To: <CALCETrXXJhkNXmjTX_8VEO39+uE4XECtm=QNTDh1DpncXKhKhw@mail.gmail.com>
On Wed, 2020-01-08 at 22:48 -0800, Andy Lutomirski wrote:
> > On Jan 8, 2020, at 10:52 AM, Edgecombe, Rick P <rick.p.edgecombe@intel.com>
> > wrote:
> >
> > On Wed, 2020-01-08 at 00:41 -0800, Andy Lutomirski wrote:
> > > > On Jan 7, 2020, at 9:01 AM, Edgecombe, Rick P <
> > > > rick.p.edgecombe@intel.com>
> > > > wrote:
> > > >
> > > > CC Nadav and Jessica.
> > > >
> > > > On Mon, 2020-01-06 at 15:36 -1000, Andy Lutomirski wrote:
> > > > > > On Jan 6, 2020, at 12:25 PM, Edgecombe, Rick P <
> > > > > > rick.p.edgecombe@intel.com>
> > > > > > wrote:
> > > > > >
> > > > > > On Sat, 2020-01-04 at 09:49 +0900, Andy Lutomirski wrote:
> > > > > > > > > > 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.
> > > > > >
> > > > > > So if I understand this right, some trampolines have been added that
> > > > > > are
> > > > > > currently set as RWX at modification time AND left that way during
> > > > > > runtime?
> > > > > > The
> > > > > > discussion on the order of set_memory_() calls in the commit message
> > > > > > made me
> > > > > > think that this was just a modification time thing at first.
> > > > >
> > > > > I’m not sure what the status quo is.
> > > > >
> > > > > We really ought to have a genuinely good API for allocation and
> > > > > initialization
> > > > > of text. We can do so much better than set_memory_blahblah.
> > > > >
> > > > > FWIW, I have some ideas about making kernel flushes cheaper. It’s
> > > > > currently
> > > > > blocked on finding some time and on tglx’s irqtrace work.
> > > > >
> > > >
> > > > Makes sense to me. I guess there are 6 types of text allocations now:
> > > > - These two BPF trampolines
> > > > - BPF JITs
> > > > - Modules
> > > > - Kprobes
> > > > - Ftrace
> > > >
> > > > All doing (or should be doing) pretty much the same thing. I believe
> > > > Jessica
> > > > had
> > > > said at one point that she didn't like all the other features using
> > > > module_alloc() as it was supposed to be just for real modules. Where
> > > > would
> > > > the
> > > > API live?
> > >
> > > New header? This shouldn’t matter that much.
> > >
> > > Here are two strawman proposals. All of this is very rough -- the
> > > actual data structures and signatures are likely problematic for
> > > multiple reasons.
> > >
> > > --- First proposal ---
> > >
> > > struct text_allocation {
> > > void *final_addr;
> > > struct page *pages;
> > > int npages;
> > > };
> > >
> > > int text_alloc(struct text_allocation *out, size_t size);
> > >
> > > /* now final_addr is not accessible and pages is writable. */
> > >
> > > int text_freeze(struct text_allocation *alloc);
> > >
> > > /* now pages are not accessible and final_addr is RO. Alternatively,
> > > pages are RO and final_addr is unmapped. */
> > >
> > > int text_finish(struct text_allocation *alloc);
> > >
> > > /* now final_addr is RX. All done. */
> > >
> > > This gets it with just one flush and gives a chance to double-check in
> > > case of race attacks from other CPUs. Double-checking is annoying,
> > > though.
> > >
> > > --- Second proposal ---
> > >
> > > struct text_allocation {
> > > void *final_addr;
> > > /* lots of opaque stuff including an mm_struct */
> > > /* optional: list of struct page, but this isn't obviously useful */
> > > };
> > >
> > > int text_alloc(struct text_allocation *out, size_t size);
> > >
> > > /* Memory is allocated. There is no way to access it at all right
> > > now. The memory is RO or not present in the direct map. */
> > >
> > > void __user *text_activate_mapping(struct text_allocation *out);
> > >
> > > /* Now the text is RW at *user* address given by return value.
> > > Preemption is off if required by use_temporary_mm(). Real user memory
> > > cannot be accessed. */
> > >
> > > void text_deactivate_mapping(struct text_allocation *alloc);
> > >
> > > /* Now the memory is inaccessible again. */
> > >
> > > void text_finalize(struct text_allocation *alloc);
> > >
> > > /* Now it's RX or XO at the final address. */
> > >
> > >
> > > Pros of second approach:
> > >
> > > - Inherently immune to cross-CPU attack. No double-check.
> > >
> > > - If we ever implement a cache of non-direct-mapped, unaliased pages,
> > > then it works with no flushes at all. We could even relax it a bit to
> > > allow non-direct-mapped pages that may have RX / XO aliases but no W
> > > aliases.
> > >
> > > - Can easily access without worrying about page boundaries.
> > >
> > > Cons:
> > >
> > > - The use of a temporary mm is annoying -- you can't copy from user
> > > memory, for example.
> >
> > Probably the first proposal is better for usages where there is a signature
> > that
> > can be checked like modules, because you could more easily check the
> > signature
> > after the text is RO. I guess leaving the direct map as RO could work for
> > the
> > second option too. Both would probably require significant changes to module
> > signature verification though.
>
> This sounds complicated — for decent performance, we want to apply
> alternatives before we make the text RO, at which point verifying the
> signature is awkward at best.
>
> >
> > Just a minor point/clarification, but outside of an enhanced signed module
> > case,
> > I think the cross-CPU attack mitigation can't be full. For example,
> > attacking
> > the verified BPF byte code (which is apparently planned to no longer be RO),
> > or
> > the pointers being loaded into these trampolines. There is always going to
> > be
> > some writable source or pointer to the source, and unless there is a way to
> > verify the end RO result, it's an un-winnable game of whack-a-mole to do it
> > in
> > full. Still the less exposed surfaces the better since the writes we are
> > worrying about in this case are probably not fully arbitrary.
>
> We could use hypervisor- or CR3-based protection. But I agree this is
> tricky and not strictly on topic :)
>
> >
> > I don't see why it would be so bad to require copying data to the kernel
> > before
> > sending it through this process. Nothing copies to final allocation directly
> > from userspace today, and from a perf perspective, how bad is an extra copy
> > when
> > we are saving TLB shootdowns? Are you thinking to protect the data that's
> > being
> > loaded from other CPUs?
>
> Hmm. If there’s a way to make loading stall, then the cross-cpu attack
> is a nice way to write shell code, so mitigating this has at least
> some value.
>
> >
> > Otherwise, could we lazily clone/sync the original mm into the temporary one
> > to
> > allow this? (possibly totally misguided idea)
>
> That involves allocating a virtual address at a safe position to make
> this work. On architectures like s390, I don’t even know if this is
> possible. Even on x86, it’s awkward. I think it’s easier to just say
> that, while the temporary mapping is active, user memory is
> inaccessible.
>
> >
> > FWIW, I really like the idea of a cache of unmapped or RO pages. Potentially
> > several optimizations we could do there.
> >
>
> I guess we would track these pages by the maximum permissions than any
> current or unmapped but unflushed alias has. This lets us get totally
> unmapped or RO pages out of the cache. Or even RX — we could
> potentially allocate, free, and reallocate text without flushing.
>
> > If this API should be cross platform, we might want to abstract the copy
> > itself
> > as well, since other arch's might have non __user solutions for copying data
> > in.
>
> Agreed, although maybe all arches would use “user” mappings.
>
> >
> > Unless someone else wants to, I can probably take a look at a first cut of
> > this
> > after I get the current thing I'm working on out. Probably better to let the
> > dust settle on the ftrace changes as well.
>
> That would be great!
>
> Do you know why the change_page_attr code currently does
> vm_unmap_aliases? This is yet more extra expense. I assume the idea
> is that, if we’re changing cache attributes on a non-self-snoop
> machine, we need to kill stale aliases, and we should also kill them
> if we’re reducing permissions. But we currently do it excessively.
I had assumed the RO/RW alias reason, but now I went and looked. I guess it was
first motivated by:
"XEN and PAT and such do not like deferred TLB flushing because they can't
always handle multiple aliasing virtual addresses to a physical address.
They now call vm_unmap_aliases() in order to flush any deferred mappings.
That call is very expensive (well, actually not a lot more expensive than
a single vunmap under the old scheme), however it should be OK if not
called too often."
https://github.com/torvalds/linux/commit/db64fe02258f1507e13fe5212a989922323685ce
For PAT, it must be like you are saying with changes in cache properties...
For XEN page table paravirt not sure if the pageattr.c call was involved or not,
but in any case it looks like it is no longer relevant. First it was changed to
eagerly flushing when under XEN pv (d2cb214551de8180542a04ec8c86c0c9412c5124),
and then just eagerly marking vmalloc PTEs as unmapped so I guess XEN can see
this and do what it needs to do (64141da587241301ce8638cc945f8b67853156ec).
For "and such"...not sure.
But flushing RW aliases for new RO memory seems valid too even if it wasn't the
intention.
> We should also consider improving vm_unmap_aliases(). As a practical
> matter, vm_unmap_aliases() often does a global flush, but it can't be
> relied on. On the other hand, a global flush initiated for other
> reasons won't tell the vmap code that aliases have been zapped.
>
> If the locking is okay, we could maybe get away with zapping aliases
> from the normal flush code. Alternatively, we could do something
> lockless, e.g.:
>
> atomic64_t kernel_tlb_gen, flushed_kernel_tlb_gen;
>
> flush_tlb_kernel_range(), etc increment kernel_tlb_gen before flushing
> and then update flushed_kernel_tlb_gen to match after flushing.
>
> The vmap code immediately removes PTEs when unmaps occur (which it may
> very well do right now -- I haven't checked) but also tracks the
> kernel_tlb_gen associated with each record of an
> unmapped-but-not-zapped area. Then we split vm_unmap_aliases() into a
> variant that unmaps all aliases and a variant that merely promises to
> unmap at least one alias. The former does what the current code does
> except that it skips the IPI if all areas in question have tlb_gen <
> flushed_kernel_tlb_gen. The latter clears all areas with tlb_gen <
> flushed_kernel_tlb_gen and, if there weren't any, does
> flush_tlb_kernel_range() and flushes everything.
>
> (Major caveat: this is wrong for the case where
> flush_tlb_kernel_range() only flushes some but not all of the kernel.
> So this needs considerable work if it's actually going to me useful.
> The plain old "take locks and clean up" approach might be a better
> bet.)
>
Hmm. In normal usage (!DEBUG_PAGE_ALLOC), are kernel range tlb shootdowns common
outside of module space users and lazy vmap stuff? A tlb_gen solution might only
be worth it in cases where something other than vm_unmap_aliases() and helpers
was doing this frequently.
^ permalink raw reply
* [PATCH v2] ima: ima/lsm policy rule loading logic bug fixes
From: Janne Karhunen @ 2020-01-10 7:55 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Casey Schaufler, Konsta Karsisto
Keep the ima policy rules around from the beginning even if they appear
invalid at the time of loading, as they may become active after the lsm
policy load. In other words, now the lsm and the ima can be initialized
in any order and the handling logic is the same as with the lsm rule
reload event. With these changes, there is no need to defer loading a
custom IMA policy, based on LSM rules, until after the LSM policy has
been initialized.
Patch also fixes the rule re-use during the lsm policy reload and makes
some prints a bit more human readable.
Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Cc: Casey Schaufler <casey@schaufler-ca.com>
Reported-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
security/integrity/ima/ima_policy.c | 44 ++++++++++++++---------------
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a4dde9d575b2..4022c7736fc3 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -265,7 +265,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
{
struct ima_rule_entry *nentry;
- int i, result;
+ int i;
nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
if (!nentry)
@@ -279,7 +279,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
memset(nentry->lsm, 0, FIELD_SIZEOF(struct ima_rule_entry, lsm));
for (i = 0; i < MAX_LSM_RULES; i++) {
- if (!entry->lsm[i].rule)
+ if (!entry->lsm[i].args_p)
continue;
nentry->lsm[i].type = entry->lsm[i].type;
@@ -288,13 +288,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
if (!nentry->lsm[i].args_p)
goto out_err;
- result = security_filter_rule_init(nentry->lsm[i].type,
- Audit_equal,
- nentry->lsm[i].args_p,
- &nentry->lsm[i].rule);
- if (result == -EINVAL)
- pr_warn("ima: rule for LSM \'%d\' is undefined\n",
- entry->lsm[i].type);
+ security_filter_rule_init(nentry->lsm[i].type,
+ Audit_equal,
+ nentry->lsm[i].args_p,
+ &nentry->lsm[i].rule);
+ if (!nentry->lsm[i].rule)
+ pr_warn("rule for LSM \'%s\' is undefined\n",
+ (char *)entry->lsm[i].args_p);
}
return nentry;
@@ -331,7 +331,9 @@ 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 (entry->lsm[i].args_p) {
+ pr_info("rule for LSM \'%s\' needs update\n",
+ (char *)entry->lsm[i].args_p);
needs_update = 1;
break;
}
@@ -341,8 +343,7 @@ static void ima_lsm_update_rules(void)
result = ima_lsm_update_rule(entry);
if (result) {
- pr_err("ima: lsm rule update error %d\n",
- result);
+ pr_err("lsm rule update error %d\n", result);
return;
}
}
@@ -865,8 +866,6 @@ static const match_table_t policy_tokens = {
static int ima_lsm_rule_init(struct ima_rule_entry *entry,
substring_t *args, int lsm_rule, int audit_type)
{
- int result;
-
if (entry->lsm[lsm_rule].rule)
return -EINVAL;
@@ -875,16 +874,15 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
return -ENOMEM;
entry->lsm[lsm_rule].type = audit_type;
- 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) {
- kfree(entry->lsm[lsm_rule].args_p);
- return -EINVAL;
- }
+ 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)
+ pr_warn("rule for LSM \'%s\' is undefined\n",
+ (char *)entry->lsm[lsm_rule].args_p);
- return result;
+ return 0;
}
static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] ima: export the measurement list when needed
From: Janne Karhunen @ 2020-01-10 8:48 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Mimi Zohar
Cc: Ken Goldman, david.safford, monty.wiseman
In-Reply-To: <20200108111743.23393-1-janne.karhunen@gmail.com>
On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Some systems can end up carrying lots of entries in the ima
> measurement list. Since every entry is using a bit of kernel
> memory, allow the sysadmin to export the measurement list to
> the filesystem to free up some memory.
Hopefully this addressed comments from everyone. The flush event can
now be triggered by the admin anytime and unique file names can be
used for each flush (log.1, log.2, ...) etc, so getting to the correct
item should be easy.
While it can now be argued that since this is an admin-driven event,
kernel does not need to write the file. However, the intention is to
bring out a second patch a bit later that adds a variable to define
the max number of entries to be kept in the kernel memory and
workqueue based automatic flushing. In those cases the kernel has to
be able to write the file without any help from the admin..
--
Janne
^ permalink raw reply
* [PATCH] selinux: remove redundant msg_msg_alloc_security
From: Huaisheng Ye @ 2020-01-10 9:58 UTC (permalink / raw)
To: paul, sds, eparis, jmorris, serge
Cc: tyu1, linux-security-module, selinux, linux-kernel, Huaisheng Ye
From: Huaisheng Ye <yehs1@lenovo.com>
selinux_msg_msg_alloc_security only calls msg_msg_alloc_security but
do nothing else. And also msg_msg_alloc_security is just used by the
former.
Remove the redundant function to simplify the code.
Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
security/selinux/hooks.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9625b99..fb1b9da 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5882,16 +5882,6 @@ static void ipc_init_security(struct ipc_security_struct *isec, u16 sclass)
isec->sid = current_sid();
}
-static int msg_msg_alloc_security(struct msg_msg *msg)
-{
- struct msg_security_struct *msec;
-
- msec = selinux_msg_msg(msg);
- msec->sid = SECINITSID_UNLABELED;
-
- return 0;
-}
-
static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
u32 perms)
{
@@ -5910,7 +5900,12 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
static int selinux_msg_msg_alloc_security(struct msg_msg *msg)
{
- return msg_msg_alloc_security(msg);
+ struct msg_security_struct *msec;
+
+ msec = selinux_msg_msg(msg);
+ msec->sid = SECINITSID_UNLABELED;
+
+ return 0;
}
/* message queue security operations */
--
1.8.3.1
^ permalink raw reply related
* [PATCH kunit] kunit: building kunit as a module breaks allmodconfig
From: Alan Maguire @ 2020-01-10 11:49 UTC (permalink / raw)
To: skhan, brendanhiggins, gregkh
Cc: rafael, jmorris, serge, knut.omang, linux-kernel,
linux-security-module, kunit-dev, linux-kselftest, sfr,
Alan Maguire
kunit tests that do not support module build should depend
on KUNIT=y rather than just KUNIT in Kconfig, otherwise
they will trigger compilation errors for "make allmodconfig"
builds.
Fixes: 9fe124bf1b77 ("kunit: allow kunit to be loaded as a module")
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/base/Kconfig | 2 +-
security/apparmor/Kconfig | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index c3b3b5c..5f0bc74 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -150,7 +150,7 @@ config DEBUG_TEST_DRIVER_REMOVE
config PM_QOS_KUNIT_TEST
bool "KUnit Test for PM QoS features"
- depends on KUNIT
+ depends on KUNIT=y
config HMEM_REPORTING
bool
diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
index d547930..0fe3368 100644
--- a/security/apparmor/Kconfig
+++ b/security/apparmor/Kconfig
@@ -71,7 +71,7 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
config SECURITY_APPARMOR_KUNIT_TEST
bool "Build KUnit tests for policy_unpack.c"
- depends on KUNIT && SECURITY_APPARMOR
+ depends on KUNIT=y && SECURITY_APPARMOR
help
This builds the AppArmor KUnit tests.
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] selinux: remove redundant msg_msg_alloc_security
From: Stephen Smalley @ 2020-01-10 15:13 UTC (permalink / raw)
To: Huaisheng Ye, paul, eparis, jmorris, serge
Cc: tyu1, linux-security-module, selinux, linux-kernel, Huaisheng Ye
In-Reply-To: <20200110095856.76612-1-yehs2007@zoho.com>
On 1/10/20 4:58 AM, Huaisheng Ye wrote:
> From: Huaisheng Ye <yehs1@lenovo.com>
>
> selinux_msg_msg_alloc_security only calls msg_msg_alloc_security but
> do nothing else. And also msg_msg_alloc_security is just used by the
> former.
>
> Remove the redundant function to simplify the code.
This seems to also be true of other _alloc_security functions, probably
due to historical reasons. Further, at least some of these functions no
longer perform any allocation; they are just initialization functions
now that allocation has been taken to the LSM framework, so possibly
could be renamed and made to return void at some point.
>
> Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/hooks.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9625b99..fb1b9da 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5882,16 +5882,6 @@ static void ipc_init_security(struct ipc_security_struct *isec, u16 sclass)
> isec->sid = current_sid();
> }
>
> -static int msg_msg_alloc_security(struct msg_msg *msg)
> -{
> - struct msg_security_struct *msec;
> -
> - msec = selinux_msg_msg(msg);
> - msec->sid = SECINITSID_UNLABELED;
> -
> - return 0;
> -}
> -
> static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
> u32 perms)
> {
> @@ -5910,7 +5900,12 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
>
> static int selinux_msg_msg_alloc_security(struct msg_msg *msg)
> {
> - return msg_msg_alloc_security(msg);
> + struct msg_security_struct *msec;
> +
> + msec = selinux_msg_msg(msg);
> + msec->sid = SECINITSID_UNLABELED;
> +
> + return 0;
> }
>
> /* message queue security operations */
>
^ 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