* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Peter Zijlstra @ 2020-01-07 9:11 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, Justin Capella, KP Singh, Rick Edgecombe,
linux-kernel, bpf, x86, linux-security-module, Kees Cook,
David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, Thomas Garnier, Florent Revest,
Brendan Jackman, Jann Horn, Matthew Garrett, Michael Halcrow
In-Reply-To: <20200106221317.wpwut2rgw23tdaoo@ast-mbp>
On Mon, Jan 06, 2020 at 02:13:18PM -0800, Alexei Starovoitov wrote:
> On Sun, Jan 05, 2020 at 10:33:54AM +0900, Andy Lutomirski wrote:
> >
> > >> On Jan 4, 2020, at 8:03 PM, Justin Capella <justincapella@gmail.com> wrote:
> > >
> > > I'm rather ignorant about this topic but it would make sense to check prior to making executable from a security standpoint wouldn't it? (In support of the (set_memory_ro + set_memory_x)
> > >
> >
> > Maybe, depends if it’s structured in a way that’s actually helpful from a security perspective.
> >
> > It doesn’t help that set_memory_x and friends are not optimized at all. These functions are very, very, very slow and adversely affect all CPUs.
>
> That was one of the reason it wasn't done in the first.
> Also ftrace trampoline break w^x as well.
Didn't I fix that?
^ permalink raw reply
* [PATCH v2] selinux: deprecate disabling SELinux and runtime
From: Paul Moore @ 2020-01-07 3:30 UTC (permalink / raw)
To: selinux; +Cc: linux-security-module
Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The
code was originally developed to make it easier for Linux
distributions to support architectures where adding parameters to the
kernel command line was difficult. Unfortunately, supporting runtime
disable meant we had to make some security trade-offs when it came to
the LSM hooks, as documented in the Kconfig help text:
NOTE: selecting this option will disable the '__ro_after_init'
kernel hardening feature for security hooks. Please consider
using the selinux=0 boot parameter instead of enabling this
option.
Fortunately it looks as if that the original motivation for the
runtime disable functionality is gone, and Fedora/RHEL appears to be
the only major distribution enabling this capability at build time
so we are now taking steps to remove it entirely from the kernel.
The first step is to mark the functionality as deprecated and print
an error when it is used (what this patch is doing). As Fedora/RHEL
makes progress in transitioning the distribution away from runtime
disable, we will introduce follow-up patches over several kernel
releases which will block for increasing periods of time when the
runtime disable is used. Finally we will remove the option entirely
once we believe all users have moved to the kernel cmdline approach.
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
Documentation/ABI/obsolete/sysfs-selinux-disable | 26 ++++++++++++++++++++++
security/selinux/Kconfig | 3 +++
security/selinux/selinuxfs.c | 6 +++++
3 files changed, 35 insertions(+)
create mode 100644 Documentation/ABI/obsolete/sysfs-selinux-disable
diff --git a/Documentation/ABI/obsolete/sysfs-selinux-disable b/Documentation/ABI/obsolete/sysfs-selinux-disable
new file mode 100644
index 000000000000..c340278e3cf8
--- /dev/null
+++ b/Documentation/ABI/obsolete/sysfs-selinux-disable
@@ -0,0 +1,26 @@
+What: /sys/fs/selinux/disable
+Date: April 2005 (predates git)
+KernelVersion: 2.6.12-rc2 (predates git)
+Contact: selinux@vger.kernel.org
+Description:
+
+ The selinuxfs "disable" node allows SELinux to be disabled at runtime
+ prior to a policy being loaded into the kernel. If disabled via this
+ mechanism, SELinux will remain disabled until the system is rebooted.
+
+ The preferred method of disabling SELinux is via the "selinux=0" boot
+ parameter, but the selinuxfs "disable" node was created to make it
+ easier for systems with primitive bootloaders that did not allow for
+ easy modification of the kernel command line. Unfortunately, allowing
+ for SELinux to be disabled at runtime makes it difficult to secure the
+ kernel's LSM hooks using the "__ro_after_init" feature.
+
+ Thankfully, the need for the SELinux runtime disable appears to be
+ gone, the default Kconfig configuration disables this selinuxfs node,
+ and only one of the major distributions, Fedora, supports disabling
+ SELinux at runtime. Fedora is in the process of removing the
+ selinuxfs "disable" node and once that is complete we will start the
+ slow process of removing this code from the kernel.
+
+ More information on /sys/fs/selinux/disable can be found under the
+ CONFIG_SECURITY_SELINUX_DISABLE Kconfig option.
diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
index 996d35d950f7..580ac24c7aa1 100644
--- a/security/selinux/Kconfig
+++ b/security/selinux/Kconfig
@@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE
using the selinux=0 boot parameter instead of enabling this
option.
+ WARNING: this option is deprecated and will be removed in a future
+ kernel release.
+
If you are unsure how to answer this question, answer N.
config SECURITY_SELINUX_DEVELOP
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 278417e67b4c..adbe2dd35202 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
int new_value;
int enforcing;
+ /* NOTE: we are now officially considering runtime disable as
+ * deprecated, and using it will become increasingly painful
+ * (e.g. sleeping/blocking) as we progress through future
+ * kernel releases until eventually it is removed */
+ pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n");
+
if (count >= PAGE_SIZE)
return -ENOMEM;
^ permalink raw reply related
* Re: [RFC PATCH] selinux: deprecate disabling SELinux and runtime
From: Paul Moore @ 2020-01-07 3:29 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, linux-security-module
In-Reply-To: <a5794ae2-d1e3-4ad3-6a16-2d479f33da16@tycho.nsa.gov>
On Mon, Jan 6, 2020 at 4:25 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/19/19 2:22 PM, Paul Moore wrote:
> > Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The
> > code was originally developed to make it easier for Linux
> > distributions to support architectures where adding parameters to the
> > kernel command line was difficult. Unfortunately, supporting runtime
> > disable meant we had to make some security trade-offs when it came to
> > the LSM hooks, as documented in the Kconfig help text:
> >
> > NOTE: selecting this option will disable the '__ro_after_init'
> > kernel hardening feature for security hooks. Please consider
> > using the selinux=0 boot parameter instead of enabling this
> > option.
> >
> > Fortunately it looks as if that the original motivation for the
> > runtime disable functionality is gone, and Fedora/RHEL appears to be
> > the only major distribution enabling this capability at build time
> > so we are now taking steps to remove it entirely from the kernel.
> > The first step is to mark the functionality as deprecated and print
> > an error when it is used (what this patch is doing). As Fedora/RHEL
> > makes progress in transitioning the distribution away from runtime
> > disable, we will introduce follow-up patches over several kernel
> > releases which will block for increasing periods of time when the
> > runtime disable is used. Finally we will remove the option entirely
> > once we believe all users have moved to the kernel cmdline approach.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> > security/selinux/Kconfig | 3 +++
> > security/selinux/selinuxfs.c | 6 ++++++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> > index 996d35d950f7..580ac24c7aa1 100644
> > --- a/security/selinux/Kconfig
> > +++ b/security/selinux/Kconfig
> > @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE
> > using the selinux=0 boot parameter instead of enabling this
> > option.
> >
> > + WARNING: this option is deprecated and will be removed in a future
> > + kernel release.
> > +
> > If you are unsure how to answer this question, answer N.
> >
> > config SECURITY_SELINUX_DEVELOP
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 278417e67b4c..adbe2dd35202 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
> > int new_value;
> > int enforcing;
> >
> > + /* NOTE: we are now officially considering runtime disable as
> > + * deprecated, and using it will become increasingly painful
> > + * (e.g. sleeping/blocking) as we progress through future
> > + * kernel releases until eventually it is removed */
> > + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n");
>
> Looking for examples of similar deprecations in the kernel, I notice
> that they often use pr_warn_once() or WARN_ONCE() to avoid spamming the
> kernel logs excessively. They also often include the current process
> name to identify the offending process. In this case, it probably
> matters little since this is only done (legitimately) by the init
> process and only once, so up to you whether you bother amending it.
Yes, I saw that too and decided we were better off printing something
each time since it really should only ever happen once on a well
behaved system.
> Also for some interfaces, they appear to document the intent to remove
> it in a file under Documentation/ABI/obsolete/ and then later move that
> to removed/ when fully removed.
Thanks, I wasn't aware of that, and couldn't find anything relevant
while grep'ing under Documentation/process. There used to be a
Documentation/feature-removal.txt (or a file with a similar name)
which tracked these things, but I guess it migrated over to
Documentation/ABI during the last Documentation shuffle a couple of
years ago.
I'll send out an updated patch in just a moment.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Andy Lutomirski @ 2020-01-07 1:36 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kpsingh@chromium.org, songliubraving@fb.com,
linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
keescook@chromium.org, ast@kernel.org, daniel@iogearbox.net,
kuznet@ms2.inr.ac.ru, jannh@google.com, mjg59@google.com,
thgarnie@chromium.org, linux-security-module@vger.kernel.org,
x86@kernel.org, revest@chromium.org, 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: <21bf6bb46544eab79e792980f82520f8fbdae9b5.camel@intel.com>
> 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.
>
> Also, is there a reason you couldn't use text_poke() to modify the trampoline
> with a single flush?
>
Does text_poke to an IPI these days?
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Edgecombe, Rick P @ 2020-01-06 22:25 UTC (permalink / raw)
To: kpsingh@chromium.org, luto@amacapital.net
Cc: songliubraving@fb.com, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org, keescook@chromium.org, ast@kernel.org,
daniel@iogearbox.net, kuznet@ms2.inr.ac.ru, jannh@google.com,
mjg59@google.com, thgarnie@chromium.org,
linux-security-module@vger.kernel.org, x86@kernel.org,
revest@chromium.org, 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: <F25C9071-A7A7-4221-BC49-A769E1677EE1@amacapital.net>
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.
Also, is there a reason you couldn't use text_poke() to modify the trampoline
with a single flush?
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Alexei Starovoitov @ 2020-01-06 22:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Justin Capella, KP Singh, Rick Edgecombe, linux-kernel, bpf, x86,
linux-security-module, Kees Cook, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, Thomas Garnier, Florent Revest, Brendan Jackman,
Jann Horn, Matthew Garrett, Michael Halcrow
In-Reply-To: <768BAF04-BEBF-489A-8737-B645816B262A@amacapital.net>
On Sun, Jan 05, 2020 at 10:33:54AM +0900, Andy Lutomirski wrote:
>
> >> On Jan 4, 2020, at 8:03 PM, Justin Capella <justincapella@gmail.com> wrote:
> >
> > I'm rather ignorant about this topic but it would make sense to check prior to making executable from a security standpoint wouldn't it? (In support of the (set_memory_ro + set_memory_x)
> >
>
> Maybe, depends if it’s structured in a way that’s actually helpful from a security perspective.
>
> It doesn’t help that set_memory_x and friends are not optimized at all. These functions are very, very, very slow and adversely affect all CPUs.
That was one of the reason it wasn't done in the first.
Also ftrace trampoline break w^x as well.
Not sure what is the plan for ftrace, but for bpf trampoline I'm going to switch
to text_poke (without _bp) once tip bits get merged during next merge window.
Then bpf trampoline will be allocated as ro+x and text_poke will be used instead of memcpy.
^ permalink raw reply
* Re: [RFC PATCH] selinux: deprecate disabling SELinux and runtime
From: Stephen Smalley @ 2020-01-06 21:26 UTC (permalink / raw)
To: Paul Moore, selinux; +Cc: linux-security-module
In-Reply-To: <157678334821.158235.2125894638773393579.stgit@chester>
On 12/19/19 2:22 PM, Paul Moore wrote:
> Deprecate the CONFIG_SECURITY_SELINUX_DISABLE functionality. The
> code was originally developed to make it easier for Linux
> distributions to support architectures where adding parameters to the
> kernel command line was difficult. Unfortunately, supporting runtime
> disable meant we had to make some security trade-offs when it came to
> the LSM hooks, as documented in the Kconfig help text:
>
> NOTE: selecting this option will disable the '__ro_after_init'
> kernel hardening feature for security hooks. Please consider
> using the selinux=0 boot parameter instead of enabling this
> option.
>
> Fortunately it looks as if that the original motivation for the
> runtime disable functionality is gone, and Fedora/RHEL appears to be
> the only major distribution enabling this capability at build time
> so we are now taking steps to remove it entirely from the kernel.
> The first step is to mark the functionality as deprecated and print
> an error when it is used (what this patch is doing). As Fedora/RHEL
> makes progress in transitioning the distribution away from runtime
> disable, we will introduce follow-up patches over several kernel
> releases which will block for increasing periods of time when the
> runtime disable is used. Finally we will remove the option entirely
> once we believe all users have moved to the kernel cmdline approach.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> security/selinux/Kconfig | 3 +++
> security/selinux/selinuxfs.c | 6 ++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> index 996d35d950f7..580ac24c7aa1 100644
> --- a/security/selinux/Kconfig
> +++ b/security/selinux/Kconfig
> @@ -42,6 +42,9 @@ config SECURITY_SELINUX_DISABLE
> using the selinux=0 boot parameter instead of enabling this
> option.
>
> + WARNING: this option is deprecated and will be removed in a future
> + kernel release.
> +
> If you are unsure how to answer this question, answer N.
>
> config SECURITY_SELINUX_DEVELOP
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 278417e67b4c..adbe2dd35202 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -281,6 +281,12 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
> int new_value;
> int enforcing;
>
> + /* NOTE: we are now officially considering runtime disable as
> + * deprecated, and using it will become increasingly painful
> + * (e.g. sleeping/blocking) as we progress through future
> + * kernel releases until eventually it is removed */
> + pr_err("SELinux: Runtime disable is deprecated, use selinux=0 on the kernel cmdline.\n");
Looking for examples of similar deprecations in the kernel, I notice
that they often use pr_warn_once() or WARN_ONCE() to avoid spamming the
kernel logs excessively. They also often include the current process
name to identify the offending process. In this case, it probably
matters little since this is only done (legitimately) by the init
process and only once, so up to you whether you bother amending it.
Also for some interfaces, they appear to document the intent to remove
it in a file under Documentation/ABI/obsolete/ and then later move that
to removed/ when fully removed. Regardless,
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> +
> if (count >= PAGE_SIZE)
> return -ENOMEM;
>
>
^ permalink raw reply
* Re: [PATCH v13 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2020-01-06 18:45 UTC (permalink / raw)
To: Casey Schaufler, Simon McVittie
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <1a392962-8a2d-a3f3-5cc1-a51e82ada41b@schaufler-ca.com>
On 1/6/20 1:03 PM, Casey Schaufler wrote:
> On 1/6/2020 9:29 AM, Simon McVittie wrote:
>> On Mon, 06 Jan 2020 at 12:15:57 -0500, Stephen Smalley wrote:
>>> On 12/24/19 6:59 PM, Casey Schaufler wrote:
>>>> The getsockopt SO_PEERSEC provides the LSM based security
>>>> information for a single module, but for reasons of backward
>>>> compatibility cannot include the information for multiple
>>>> modules. A new option SO_PEERCONTEXT is added to report the
>>>> security "context" of multiple modules using a "compound" format
>>>>
>>>> lsm1\0value\0lsm2\0value\0
>>>>
>>>> This is expected to be used by system services, including dbus-daemon.
>>>> The exact format of a compound context has been the subject of
>>>> considerable debate. This format was suggested by Simon McVittie,
>>>> a dbus maintainer with a significant stake in the format being
>>>> usable.
>>> Since upstream AA does not currently ever set the peer label info, there is
>>> no need for this support for stacking upstream AA today, and there is no way
>>> to test this functionality with more than one module present currently in an
>>> upstream kernel. Either fix AA to actually implement the functionality so
>>> it can be tested properly, or drop it from this series please.
>
> I agree that SO_PEERCONTEXT can be deferred until such time as we have
> AppArmor upstream support for SO_PEERSEC.
>
>>> I don't
>>> understand why AA continues to keep this kind of basic and longstanding
>>> downstream functionality out of tree.
>
> Not everyone has the resource commitments of the world's largest
> government. :(
How hard is it to upstream code that is a) entirely contained within the
AA security module, and b) already shipping in Ubuntu kernels for quite
some time? Seems to be more of a lack of an upstream-first philosophy
than a resources issue...
>
>> Alternatively, a pair of tiny in-tree or out-of-tree stackable LSMs
>> that don't make any security decisions, and label every labellable
>> process/socket/thing with something predictable, would make it really
>> easy for both kernel and user-space developers to test this and the
>> user-space code that uses it (D-Bus and others).
>
> Sounds like a fun and educational project. Maybe one of our lurkers
> could do something clever.
>
>>
>> For example, they could label process 1234 and all sockets created by
>> process 1234 with "contexttest1\0pid1234\0contexttest2\0process1234" or
>> something like that.
>>
>> I'd love to see AppArmor in upstream kernels support SO_PEERSEC and
>> SO_PEERCONTEXT, but setting up a development machine to stack AppArmor
>> and SELinux (and still be able to boot, without one or the other LSM
>> forbidding something important) seems likely to be harder than setting
>> it up to load some toy LSMs.
>
>>
>> smcv
>
^ permalink raw reply
* Re: [PATCH v13 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2020-01-06 18:43 UTC (permalink / raw)
To: Simon McVittie
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200106172949.GA80652@horizon>
On 1/6/20 12:29 PM, Simon McVittie wrote:
> On Mon, 06 Jan 2020 at 12:15:57 -0500, Stephen Smalley wrote:
>> On 12/24/19 6:59 PM, Casey Schaufler wrote:
>>> The getsockopt SO_PEERSEC provides the LSM based security
>>> information for a single module, but for reasons of backward
>>> compatibility cannot include the information for multiple
>>> modules. A new option SO_PEERCONTEXT is added to report the
>>> security "context" of multiple modules using a "compound" format
>>>
>>> lsm1\0value\0lsm2\0value\0
>>>
>>> This is expected to be used by system services, including dbus-daemon.
>>> The exact format of a compound context has been the subject of
>>> considerable debate. This format was suggested by Simon McVittie,
>>> a dbus maintainer with a significant stake in the format being
>>> usable.
>>
>> Since upstream AA does not currently ever set the peer label info, there is
>> no need for this support for stacking upstream AA today, and there is no way
>> to test this functionality with more than one module present currently in an
>> upstream kernel. Either fix AA to actually implement the functionality so
>> it can be tested properly, or drop it from this series please. I don't
>> understand why AA continues to keep this kind of basic and longstanding
>> downstream functionality out of tree.
>
> Alternatively, a pair of tiny in-tree or out-of-tree stackable LSMs
> that don't make any security decisions, and label every labellable
> process/socket/thing with something predictable, would make it really
> easy for both kernel and user-space developers to test this and the
> user-space code that uses it (D-Bus and others).
>
> For example, they could label process 1234 and all sockets created by
> process 1234 with "contexttest1\0pid1234\0contexttest2\0process1234" or
> something like that.
>
> I'd love to see AppArmor in upstream kernels support SO_PEERSEC and
> SO_PEERCONTEXT, but setting up a development machine to stack AppArmor
> and SELinux (and still be able to boot, without one or the other LSM
> forbidding something important) seems likely to be harder than setting
> it up to load some toy LSMs.
AA+SELinux with these patches boots fine for me with Fedora; it doesn't
load any policy for AA but you still get a compound context from
/proc/pid/attr/context. Should be similar for booting with a distro
that only enables AA by default; you'll get "kernel" for the SELinux
part of the compound label in the absence of any policy loaded.
^ permalink raw reply
* Re: [PATCH v13 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Casey Schaufler @ 2020-01-06 18:03 UTC (permalink / raw)
To: Simon McVittie, Stephen Smalley
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <20200106172949.GA80652@horizon>
On 1/6/2020 9:29 AM, Simon McVittie wrote:
> On Mon, 06 Jan 2020 at 12:15:57 -0500, Stephen Smalley wrote:
>> On 12/24/19 6:59 PM, Casey Schaufler wrote:
>>> The getsockopt SO_PEERSEC provides the LSM based security
>>> information for a single module, but for reasons of backward
>>> compatibility cannot include the information for multiple
>>> modules. A new option SO_PEERCONTEXT is added to report the
>>> security "context" of multiple modules using a "compound" format
>>>
>>> lsm1\0value\0lsm2\0value\0
>>>
>>> This is expected to be used by system services, including dbus-daemon.
>>> The exact format of a compound context has been the subject of
>>> considerable debate. This format was suggested by Simon McVittie,
>>> a dbus maintainer with a significant stake in the format being
>>> usable.
>> Since upstream AA does not currently ever set the peer label info, there is
>> no need for this support for stacking upstream AA today, and there is no way
>> to test this functionality with more than one module present currently in an
>> upstream kernel. Either fix AA to actually implement the functionality so
>> it can be tested properly, or drop it from this series please.
I agree that SO_PEERCONTEXT can be deferred until such time as we have
AppArmor upstream support for SO_PEERSEC.
>> I don't
>> understand why AA continues to keep this kind of basic and longstanding
>> downstream functionality out of tree.
Not everyone has the resource commitments of the world's largest
government. :(
> Alternatively, a pair of tiny in-tree or out-of-tree stackable LSMs
> that don't make any security decisions, and label every labellable
> process/socket/thing with something predictable, would make it really
> easy for both kernel and user-space developers to test this and the
> user-space code that uses it (D-Bus and others).
Sounds like a fun and educational project. Maybe one of our lurkers
could do something clever.
>
> For example, they could label process 1234 and all sockets created by
> process 1234 with "contexttest1\0pid1234\0contexttest2\0process1234" or
> something like that.
>
> I'd love to see AppArmor in upstream kernels support SO_PEERSEC and
> SO_PEERCONTEXT, but setting up a development machine to stack AppArmor
> and SELinux (and still be able to boot, without one or the other LSM
> forbidding something important) seems likely to be harder than setting
> it up to load some toy LSMs.
>
> smcv
^ permalink raw reply
* Re: [PATCH v13 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Simon McVittie @ 2020-01-06 17:29 UTC (permalink / raw)
To: Stephen Smalley
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <cce57d59-8c68-8ef0-b887-0597492e1833@tycho.nsa.gov>
On Mon, 06 Jan 2020 at 12:15:57 -0500, Stephen Smalley wrote:
> On 12/24/19 6:59 PM, Casey Schaufler wrote:
> > The getsockopt SO_PEERSEC provides the LSM based security
> > information for a single module, but for reasons of backward
> > compatibility cannot include the information for multiple
> > modules. A new option SO_PEERCONTEXT is added to report the
> > security "context" of multiple modules using a "compound" format
> >
> > lsm1\0value\0lsm2\0value\0
> >
> > This is expected to be used by system services, including dbus-daemon.
> > The exact format of a compound context has been the subject of
> > considerable debate. This format was suggested by Simon McVittie,
> > a dbus maintainer with a significant stake in the format being
> > usable.
>
> Since upstream AA does not currently ever set the peer label info, there is
> no need for this support for stacking upstream AA today, and there is no way
> to test this functionality with more than one module present currently in an
> upstream kernel. Either fix AA to actually implement the functionality so
> it can be tested properly, or drop it from this series please. I don't
> understand why AA continues to keep this kind of basic and longstanding
> downstream functionality out of tree.
Alternatively, a pair of tiny in-tree or out-of-tree stackable LSMs
that don't make any security decisions, and label every labellable
process/socket/thing with something predictable, would make it really
easy for both kernel and user-space developers to test this and the
user-space code that uses it (D-Bus and others).
For example, they could label process 1234 and all sockets created by
process 1234 with "contexttest1\0pid1234\0contexttest2\0process1234" or
something like that.
I'd love to see AppArmor in upstream kernels support SO_PEERSEC and
SO_PEERCONTEXT, but setting up a development machine to stack AppArmor
and SELinux (and still be able to boot, without one or the other LSM
forbidding something important) seems likely to be harder than setting
it up to load some toy LSMs.
smcv
^ permalink raw reply
* Re: [PATCH v13 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2020-01-06 17:15 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-24-casey@schaufler-ca.com>
On 12/24/19 6:59 PM, Casey Schaufler wrote:
> The getsockopt SO_PEERSEC provides the LSM based security
> information for a single module, but for reasons of backward
> compatibility cannot include the information for multiple
> modules. A new option SO_PEERCONTEXT is added to report the
> security "context" of multiple modules using a "compound" format
>
> lsm1\0value\0lsm2\0value\0
>
> This is expected to be used by system services, including dbus-daemon.
> The exact format of a compound context has been the subject of
> considerable debate. This format was suggested by Simon McVittie,
> a dbus maintainer with a significant stake in the format being
> usable.
Since upstream AA does not currently ever set the peer label info, there
is no need for this support for stacking upstream AA today, and there is
no way to test this functionality with more than one module present
currently in an upstream kernel. Either fix AA to actually implement
the functionality so it can be tested properly, or drop it from this
series please. I don't understand why AA continues to keep this kind of
basic and longstanding downstream functionality out of tree.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
> cc: linux-api@vger.kernel.org
> ---
> Documentation/security/lsm.rst | 15 ++++
> arch/alpha/include/uapi/asm/socket.h | 1 +
> arch/mips/include/uapi/asm/socket.h | 1 +
> arch/parisc/include/uapi/asm/socket.h | 1 +
> arch/sparc/include/uapi/asm/socket.h | 1 +
> include/linux/lsm_hooks.h | 9 +-
> include/linux/security.h | 10 ++-
> include/uapi/asm-generic/socket.h | 1 +
> net/core/sock.c | 7 +-
> security/apparmor/lsm.c | 20 ++---
> security/security.c | 115 +++++++++++++++++++++++---
> security/selinux/hooks.c | 20 ++---
> security/smack/smack_lsm.c | 31 +++----
> 13 files changed, 170 insertions(+), 62 deletions(-)
>
> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
> index aadf47c808c0..77cc326a52cc 100644
> --- a/Documentation/security/lsm.rst
> +++ b/Documentation/security/lsm.rst
> @@ -199,3 +199,18 @@ capability-related fields:
> - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()`
>
> - ``fs/proc/array.c``::c:func:`task_cap()`
> +
> +LSM External Interfaces
> +=======================
> +
> +The LSM infrastructure does not generally provide external interfaces.
> +The individual security modules provide what external interfaces they
> +require. The infrastructure does provide two interfaces for the special
> +case where multiple security modules provide a process context. This
> +is provided in compound context format.
> +
> +- `lsm1\0value\0lsm2\0value\0`
> +
> +The special file ``/proc/pid/attr/context`` provides the security
> +context of the identified process. The socket option SO_PEERCONTEXT
> +provides the security context of a packet.
> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
> index de6c4df61082..b26fb34e4226 100644
> --- a/arch/alpha/include/uapi/asm/socket.h
> +++ b/arch/alpha/include/uapi/asm/socket.h
> @@ -123,6 +123,7 @@
> #define SO_SNDTIMEO_NEW 67
>
> #define SO_DETACH_REUSEPORT_BPF 68
> +#define SO_PEERCONTEXT 69
>
> #if !defined(__KERNEL__)
>
> diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
> index d0a9ed2ca2d6..10e03507b1ed 100644
> --- a/arch/mips/include/uapi/asm/socket.h
> +++ b/arch/mips/include/uapi/asm/socket.h
> @@ -134,6 +134,7 @@
> #define SO_SNDTIMEO_NEW 67
>
> #define SO_DETACH_REUSEPORT_BPF 68
> +#define SO_PEERCONTEXT 69
>
> #if !defined(__KERNEL__)
>
> diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
> index 10173c32195e..e11df59a84d1 100644
> --- a/arch/parisc/include/uapi/asm/socket.h
> +++ b/arch/parisc/include/uapi/asm/socket.h
> @@ -115,6 +115,7 @@
> #define SO_SNDTIMEO_NEW 0x4041
>
> #define SO_DETACH_REUSEPORT_BPF 0x4042
> +#define SO_PEERCONTEXT 0x4043
>
> #if !defined(__KERNEL__)
>
> diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
> index 8029b681fc7c..5b41ef778040 100644
> --- a/arch/sparc/include/uapi/asm/socket.h
> +++ b/arch/sparc/include/uapi/asm/socket.h
> @@ -116,6 +116,7 @@
> #define SO_SNDTIMEO_NEW 0x0045
>
> #define SO_DETACH_REUSEPORT_BPF 0x0047
> +#define SO_PEERCONTEXT 0x0048
>
> #if !defined(__KERNEL__)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 2bf82e1cf347..2ae10e7f81a7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -880,8 +880,8 @@
> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the
> * socket is associated with an ipsec SA.
> * @sock is the local socket.
> - * @optval userspace memory where the security state is to be copied.
> - * @optlen userspace int where the module should copy the actual length
> + * @optval memory where the security state is to be copied.
> + * @optlen int where the module should copy the actual length
> * of the security state.
> * @len as input is the maximum length to copy to userspace provided
> * by the caller.
> @@ -1724,9 +1724,8 @@ union security_list_options {
> int (*socket_setsockopt)(struct socket *sock, int level, int optname);
> int (*socket_shutdown)(struct socket *sock, int how);
> int (*socket_sock_rcv_skb)(struct sock *sk, struct sk_buff *skb);
> - int (*socket_getpeersec_stream)(struct socket *sock,
> - char __user *optval,
> - int __user *optlen, unsigned len);
> + int (*socket_getpeersec_stream)(struct socket *sock, char **optval,
> + int *optlen, unsigned len);
> int (*socket_getpeersec_dgram)(struct socket *sock,
> struct sk_buff *skb, u32 *secid);
> int (*sk_alloc_security)(struct sock *sk, int family, gfp_t priority);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d7af2bbbc878..26967055a002 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -178,6 +178,7 @@ struct lsmblob {
> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
> #define LSMBLOB_FIRST -5 /* Use the default "display" slot */
> +#define LSMBLOB_COMPOUND -6 /* A compound "display" */
>
> /**
> * lsmblob_init - initialize an lsmblob structure.
> @@ -1396,7 +1397,8 @@ int security_socket_setsockopt(struct socket *sock, int level, int optname);
> int security_socket_shutdown(struct socket *sock, int how);
> int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> - int __user *optlen, unsigned len);
> + int __user *optlen, unsigned len,
> + int display);
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> struct lsmblob *blob);
> int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
> @@ -1530,8 +1532,10 @@ static inline int security_sock_rcv_skb(struct sock *sk,
> return 0;
> }
>
> -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> - int __user *optlen, unsigned len)
> +static inline int security_socket_getpeersec_stream(struct socket *sock,
> + char __user *optval,
> + int __user *optlen,
> + unsigned len, int display)
> {
> return -ENOPROTOOPT;
> }
> diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
> index 77f7c1638eb1..e3a853d53705 100644
> --- a/include/uapi/asm-generic/socket.h
> +++ b/include/uapi/asm-generic/socket.h
> @@ -118,6 +118,7 @@
> #define SO_SNDTIMEO_NEW 67
>
> #define SO_DETACH_REUSEPORT_BPF 68
> +#define SO_PEERCONTEXT 69
>
> #if !defined(__KERNEL__)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 043db3ce023e..63b7eda81a90 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1411,7 +1411,12 @@ int sock_getsockopt(struct socket *sock, int level, int optname,
> break;
>
> case SO_PEERSEC:
> - return security_socket_getpeersec_stream(sock, optval, optlen, len);
> + return security_socket_getpeersec_stream(sock, optval, optlen,
> + len, LSMBLOB_DISPLAY);
> +
> + case SO_PEERCONTEXT:
> + return security_socket_getpeersec_stream(sock, optval, optlen,
> + len, LSMBLOB_COMPOUND);
>
> case SO_MARK:
> v.val = sk->sk_mark;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 16b992235c11..34edfd29c32f 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1078,10 +1078,8 @@ static struct aa_label *sk_peer_label(struct sock *sk)
> *
> * Note: for tcp only valid if using ipsec or cipso on lan
> */
> -static int apparmor_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen,
> - unsigned int len)
> +static int apparmor_socket_getpeersec_stream(struct socket *sock, char **optval,
> + int *optlen, unsigned int len)
> {
> char *name;
> int slen, error = 0;
> @@ -1101,17 +1099,11 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
> if (slen < 0) {
> error = -ENOMEM;
> } else {
> - if (slen > len) {
> + if (slen > len)
> error = -ERANGE;
> - } else if (copy_to_user(optval, name, slen)) {
> - error = -EFAULT;
> - goto out;
> - }
> - if (put_user(slen, optlen))
> - error = -EFAULT;
> -out:
> - kfree(name);
> -
> + else
> + *optval = name;
> + *optlen = slen;
> }
>
> done:
> diff --git a/security/security.c b/security/security.c
> index 6d05222aac9c..80539dfd0245 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -723,6 +723,42 @@ static void __init lsm_early_task(struct task_struct *task)
> panic("%s: Early task alloc failed.\n", __func__);
> }
>
> +/**
> + * append_ctx - append a lsm/context pair to a compound context
> + * @ctx: the existing compound context
> + * @ctxlen: size of the old context, including terminating nul byte
> + * @lsm: new lsm name, nul terminated
> + * @new: new context, possibly nul terminated
> + * @newlen: maximum size of @new
> + *
> + * replace @ctx with a new compound context, appending @newlsm and @new
> + * to @ctx. On exit the new data replaces the old, which is freed.
> + * @ctxlen is set to the new size, which includes a trailing nul byte.
> + *
> + * Returns 0 on success, -ENOMEM if no memory is available.
> + */
> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
> + int newlen)
> +{
> + char *final;
> + int llen;
> +
> + llen = strlen(lsm) + 1;
> + newlen = strnlen(new, newlen) + 1;
> +
> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
> + if (final == NULL)
> + return -ENOMEM;
> + if (*ctxlen)
> + memcpy(final, *ctx, *ctxlen);
> + memcpy(final + *ctxlen, lsm, llen);
> + memcpy(final + *ctxlen + llen, new, newlen);
> + kfree(*ctx);
> + *ctx = final;
> + *ctxlen = *ctxlen + llen + newlen;
> + return 0;
> +}
> +
> /*
> * Hook list operation macros.
> *
> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> - if (lsm == NULL && *display != LSMBLOB_INVALID &&
> - *display != hp->lsmid->slot)
> + if (lsm == NULL && display != NULL &&
> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
> continue;
> return hp->hook.setprocattr(name, value, size);
> }
> @@ -2245,12 +2281,21 @@ void security_release_secctx(struct lsmcontext *cp)
> {
> struct security_hook_list *hp;
>
> + if (cp->slot == LSMBLOB_INVALID)
> + return;
> +
> + if (cp->slot == LSMBLOB_COMPOUND) {
> + kfree(cp->context);
> + goto clear_out;
> + }
> +
> hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> if (cp->slot == hp->lsmid->slot) {
> hp->hook.release_secctx(cp->context, cp->len);
> break;
> }
>
> +clear_out:
> memset(cp, 0, sizeof(*cp));
> }
> EXPORT_SYMBOL(security_release_secctx);
> @@ -2383,17 +2428,67 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> EXPORT_SYMBOL(security_sock_rcv_skb);
>
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> - int __user *optlen, unsigned len)
> + int __user *optlen, unsigned len,
> + int display)
> {
> - int display = lsm_task_display(current);
> struct security_hook_list *hp;
> + char *final = NULL;
> + char *cp;
> + int rc = 0;
> + unsigned finallen = 0;
> + unsigned clen = 0;
>
> - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
> - list)
> - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> - return hp->hook.socket_getpeersec_stream(sock, optval,
> - optlen, len);
> - return -ENOPROTOOPT;
> + switch (display) {
> + case LSMBLOB_DISPLAY:
> + rc = -ENOPROTOOPT;
> + display = lsm_task_display(current);
> + hlist_for_each_entry(hp,
> + &security_hook_heads.socket_getpeersec_stream,
> + list)
> + if (display == LSMBLOB_INVALID ||
> + display == hp->lsmid->slot) {
> + rc = hp->hook.socket_getpeersec_stream(sock,
> + &final, &finallen, len);
> + break;
> + }
> + break;
> + case LSMBLOB_COMPOUND:
> + /*
> + * A compound context, in the form [lsm\0value\0]...
> + */
> + hlist_for_each_entry(hp,
> + &security_hook_heads.socket_getpeersec_stream,
> + list) {
> + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen,
> + len);
> + if (rc == -EINVAL || rc == -ENOPROTOOPT) {
> + rc = 0;
> + continue;
> + }
> + if (rc) {
> + kfree(final);
> + return rc;
> + }
> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
> + cp, clen);
> + }
> + if (final == NULL)
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (finallen > len)
> + rc = -ERANGE;
> + else if (copy_to_user(optval, final, finallen))
> + rc = -EFAULT;
> +
> + if (put_user(finallen, optlen))
> + rc = -EFAULT;
> +
> + kfree(final);
> + return rc;
> }
>
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index cd4743331800..c3e6fd3f8c56 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5056,10 +5056,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> return err;
> }
>
> -static int selinux_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen,
> - unsigned int len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval,
> + int *optlen, unsigned int len)
> {
> int err = 0;
> char *scontext;
> @@ -5079,18 +5077,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
> if (err)
> return err;
>
> - if (scontext_len > len) {
> + if (scontext_len > len)
> err = -ERANGE;
> - goto out_len;
> - }
> -
> - if (copy_to_user(optval, scontext, scontext_len))
> - err = -EFAULT;
> + else
> + *optval = scontext;
>
> -out_len:
> - if (put_user(scontext_len, optlen))
> - err = -EFAULT;
> - kfree(scontext);
> + *optlen = scontext_len;
> return err;
> }
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 9ce67e03ac49..316c5faf9053 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3957,28 +3957,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> *
> * returns zero on success, an error code otherwise
> */
> -static int smack_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen, unsigned len)
> +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval,
> + int *optlen, unsigned len)
> {
> - struct socket_smack *ssp;
> - char *rcp = "";
> - int slen = 1;
> + struct socket_smack *ssp = smack_sock(sock->sk);
> + char *rcp;
> + int slen;
> int rc = 0;
>
> - ssp = smack_sock(sock->sk);
> - if (ssp->smk_packet != NULL) {
> - rcp = ssp->smk_packet->smk_known;
> - slen = strlen(rcp) + 1;
> + if (ssp->smk_packet == NULL) {
> + *optlen = 0;
> + return -EINVAL;
> }
>
> + rcp = ssp->smk_packet->smk_known;
> + slen = strlen(rcp) + 1;
> if (slen > len)
> rc = -ERANGE;
> - else if (copy_to_user(optval, rcp, slen) != 0)
> - rc = -EFAULT;
> -
> - if (put_user(slen, optlen) != 0)
> - rc = -EFAULT;
> + else {
> + *optval = kstrdup(rcp, GFP_KERNEL);
> + if (*optval == NULL)
> + rc = -ENOMEM;
> + }
> + *optlen = slen;
>
> return rc;
> }
>
^ permalink raw reply
* [PATCH v2] ima: add the ability to query the hash of a given file.
From: Florent Revest @ 2020-01-06 16:25 UTC (permalink / raw)
To: linux-integrity
Cc: kpsingh, mjg59, zohar, nramas, linux-kernel,
linux-security-module, Florent Revest
From: Florent Revest <revest@google.com>
This allows other parts of the kernel (perhaps a stacked LSM allowing
system monitoring, eg. the proposed KRSI LSM [1]) to retrieve the hash
of a given file from IMA if it's present in the iint cache.
It's true that the existence of the hash means that it's also in the
audit logs or in /sys/kernel/security/ima/ascii_runtime_measurements,
but it can be difficult to pull that information out for every
subsequent exec. This is especially true if a given host has been up
for a long time and the file was first measured a long time ago.
This is based on Peter Moody's patch:
https://sourceforge.net/p/linux-ima/mailman/message/33036180/
[1] https://lkml.org/lkml/2019/9/10/393
Signed-off-by: Florent Revest <revest@google.com>
---
include/linux/ima.h | 6 ++++
security/integrity/ima/ima_main.c | 46 +++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6d904754d858..d621c65ba9a5 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,6 +23,7 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
+extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(const void *buf, int size);
#ifdef CONFIG_IMA_KEXEC
@@ -91,6 +92,11 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
return;
}
+static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
+{
+ return -EOPNOTSUPP;
+}
+
static inline void ima_kexec_cmdline(const void *buf, int size) {}
#endif /* CONFIG_IMA */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..3799b6c6c3b8 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -445,6 +445,52 @@ int ima_file_check(struct file *file, int mask)
}
EXPORT_SYMBOL_GPL(ima_file_check);
+/**
+ * ima_file_hash - return the stored measurement if a file has been hashed.
+ * @file: pointer to the file
+ * @buf: buffer in which to store the hash
+ * @buf_size: length of the buffer
+ *
+ * On success, return the hash algorithm (as defined in the enum hash_algo).
+ * If buf is not NULL, this function also outputs the hash into buf.
+ * If the hash is larger than buf_size, then only buf_size bytes will be copied.
+ * It generally just makes sense to pass a buffer capable of holding the largest
+ * possible hash: IMA_MAX_DIGEST_SIZE
+ *
+ * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
+ * If the parameters are incorrect, return -EINVAL.
+ */
+int ima_file_hash(struct file *file, char *buf, size_t buf_size)
+{
+ struct inode *inode;
+ struct integrity_iint_cache *iint;
+ int hash_algo;
+
+ if (!file)
+ return -EINVAL;
+
+ if (!ima_policy_flag)
+ return -EOPNOTSUPP;
+
+ inode = file_inode(file);
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&iint->mutex);
+ if (buf) {
+ size_t copied_size;
+
+ copied_size = min_t(size_t, iint->ima_hash->length, buf_size);
+ memcpy(buf, iint->ima_hash->digest, copied_size);
+ }
+ hash_algo = iint->ima_hash->algo;
+ mutex_unlock(&iint->mutex);
+
+ return hash_algo;
+}
+EXPORT_SYMBOL_GPL(ima_file_hash);
+
/**
* ima_post_create_tmpfile - mark newly created tmpfile as new
* @file : newly created tmpfile
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related
* Re: [PATCH 24/25] LSM: Add /proc attr entry for full LSM context
From: Stephen Smalley @ 2020-01-06 16:22 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-25-casey@schaufler-ca.com>
On 12/24/19 6:59 PM, Casey Schaufler wrote:
> Add an entry /proc/.../attr/context which displays the full
> process security "context" in compound format:'
> lsm1\0value\0lsm2\0value\0...
> This entry is not writable.
My takeaway from the earlier exchange with Simon on v12 was that we
should either define a new LSM hook for /proc/pid/attr/context that is
guaranteed to return a value in a consistent format with SO_PEERCONTEXT
or fix AppArmor to return consistent results from its existing
getprocattr hook (particularly with respect to the newline character).
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-api@vger.kernel.org
> ---
> fs/proc/base.c | 1 +
> security/security.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 950c200cb9ad..d13c2cf50e4b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2653,6 +2653,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate", 0666),
> ATTR(NULL, "display", 0666),
> + ATTR(NULL, "context", 0666),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack", 0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/security/security.c b/security/security.c
> index 80539dfd0245..e94de64e114c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2099,6 +2099,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> char **value)
> {
> struct security_hook_list *hp;
> + char *final = NULL;
> + char *cp;
> + int rc = 0;
> + int finallen = 0;
> int display = lsm_task_display(current);
> int slot = 0;
>
> @@ -2126,6 +2130,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> return -ENOMEM;
> }
>
> + if (!strcmp(name, "context")) {
> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
> + list) {
> + rc = hp->hook.getprocattr(p, "current", &cp);
> + if (rc == -EINVAL || rc == -ENOPROTOOPT)
> + continue;
> + if (rc < 0) {
> + kfree(final);
> + return rc;
> + }
> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
> + cp, rc);
> + if (rc < 0) {
> + kfree(final);
> + return rc;
> + }
> + }
> + if (final == NULL)
> + return -EINVAL;
> + *value = final;
> + return finallen;
> + }
> +
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
>
^ permalink raw reply
* Re: [PATCH] ima: add the ability to query ima for the hash of a given file.
From: Florent Revest @ 2020-01-06 16:15 UTC (permalink / raw)
To: Mimi Zohar, Lakshmi Ramasubramanian, linux-integrity
Cc: kpsingh, mjg59, linux-kernel, linux-security-module,
Florent Revest
In-Reply-To: <1577122751.5241.144.camel@linux.ibm.com>
On Mon, 2019-12-23 at 12:39 -0500, Mimi Zohar wrote:
> On Fri, 2019-12-20 at 08:48 -0800, Lakshmi Ramasubramanian wrote:
> > On 12/20/2019 8:31 AM, Florent Revest wrote:
> >
> > >
> > > +/**
> > > + * ima_file_hash - return the stored measurement if a file has
> > > been hashed.
> > > + * @file: pointer to the file
> > > + * @buf: buffer in which to store the hash
> > > + * @buf_size: length of the buffer
> > > + *
> > > + * On success, output the hash into buf and return the hash
> > > algorithm (as
> > > + * defined in the enum hash_algo).
> > > + * If the hash is larger than buf, then only size bytes will be
> > > copied. It
> > > + * generally just makes sense to pass a buffer capable of
> > > holding the largest
> > > + * possible hash: IMA_MAX_DIGEST_SIZE
> >
> > If the given buffer is smaller than the hash length, wouldn't it
> > be
> > better to return the required size and a status indicating the
> > buffer is
> > not enough. The caller can then call back with the required buffer.
> >
> > If the hash is truncated the caller may not know if the hash is
> > partial
> > or not.
>
> Based on the hash algorithm, the caller would know if the buffer
> provided was too small and was truncated.
>
> > > + *
> > > + * If IMA is disabled or if no measurement is available, return
> > > -EOPNOTSUPP.
> > > + * If the parameters are incorrect, return -EINVAL.
> > > + */
> > > +int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> > > +{
> > > + struct inode *inode;
> > > + struct integrity_iint_cache *iint;
> > > + size_t copied_size;
> > > +
> > > + if (!file || !buf)
> > > + return -EINVAL;
> > > +
>
> Other kernel functions provide a means of determining the needed
> buffer size by passing a NULL field. Instead of failing here, if buf
> is NULL, how about returning the hash algorithm?
I wasn't aware of that. This is nice to have indeed! I'll send a v2.
Thanks!
^ permalink raw reply
* Re: [PATCH v13 15/25] LSM: Use lsmcontext in security_secid_to_secctx
From: Stephen Smalley @ 2020-01-06 16:15 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-16-casey@schaufler-ca.com>
On 12/24/19 6:59 PM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
> ---
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3305c4af43a8..224c7b4a1bc0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1178,9 +1178,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_buffer *ab;
> u16 msg_type = nlh->nlmsg_type;
> struct audit_sig_info *sig_data;
> - char *ctx = NULL;
> u32 len;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context = { };
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1418,25 +1417,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_SIGNAL_INFO:
> len = 0;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> - &len);
> + err = security_secid_to_secctx(&audit_sig_lsm,
> + &context);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
As I mentioned in my 2nd reply on the v12 version of this patch, you
forgot to update this kmalloc() to use context.len, so you'll end up
allocating too small a buffer and then writing out of bounds upon the
memcpy below. KASAN would have detected this for you if you enabled it
in your kernel config.
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm)) {
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmblob_is_set(&audit_sig_lsm))
> + security_release_secctx(&context);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - memcpy(sig_data->ctx, ctx, len);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + memcpy(sig_data->ctx, context.context, context.len);
> + security_release_secctx(&context);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
^ permalink raw reply
* Re: [PATCH] ima: add the ability to query ima for the hash of a given file.
From: Florent Revest @ 2020-01-06 16:10 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, linux-integrity
Cc: kpsingh, mjg59, zohar, linux-kernel, linux-security-module,
Florent Revest
In-Reply-To: <8f4d9c4e-735d-8ba9-b84a-4f341030e0cf@linux.microsoft.com>
On Fri, 2019-12-20 at 08:48 -0800, Lakshmi Ramasubramanian wrote:
> On 12/20/2019 8:31 AM, Florent Revest wrote:
>
> >
> > +/**
> > + * ima_file_hash - return the stored measurement if a file has
> > been hashed.
> > + * @file: pointer to the file
> > + * @buf: buffer in which to store the hash
> > + * @buf_size: length of the buffer
> > + *
> > + * On success, output the hash into buf and return the hash
> > algorithm (as
> > + * defined in the enum hash_algo).
> > + * If the hash is larger than buf, then only size bytes will be
> > copied. It
> > + * generally just makes sense to pass a buffer capable of holding
> > the largest
> > + * possible hash: IMA_MAX_DIGEST_SIZE
>
> If the given buffer is smaller than the hash length, wouldn't it be
> better to return the required size and a status indicating the buffer
> is not enough. The caller can then call back with the required
> buffer.
>
> If the hash is truncated the caller may not know if the hash is
> partial or not.
I agree with Mimi's answer that the caller would know based on the
returned hash algorithm.
> > + *
> > + * If IMA is disabled or if no measurement is available, return
> > -EOPNOTSUPP.
> > + * If the parameters are incorrect, return -EINVAL.
> > + */
> > +int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> > +{
> > + struct inode *inode;
> > + struct integrity_iint_cache *iint;
> > + size_t copied_size;
> > +
> > + if (!file || !buf)
> > + return -EINVAL;
> > +
> > + if (!ima_policy_flag)
> > + return -EOPNOTSUPP;
> > +
> > + inode = file_inode(file);
> > + iint = integrity_iint_find(inode);
> > + if (!iint)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&iint->mutex);
> > + copied_size = min_t(size_t, iint->ima_hash->length, buf_size);
> > + memcpy(buf, iint->ima_hash->digest, copied_size);
> > + mutex_unlock(&iint->mutex);
> > +
> > + return iint->ima_hash->algo;
>
> Should the hash algorithm be copied from iinit->ima_hash to a local
> variable while holding the mutex and that one returned?
>
> I assume iinit->mutex is taken to ensure iinit->ima_hash is not
> removed while this function is accessing it.
Ah! Good catch, thank you :)
^ permalink raw reply
* Re: [PATCH v6 00/10] proc: modernize proc to support multiple private instances
From: Alexey Dobriyan @ 2020-01-06 15:15 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20191225125151.1950142-1-gladkov.alexey@gmail.com>
> hidepid= Set /proc/<pid>/ access mode.
> gid= Set the group authorized to learn processes information.
> + pidonly= Show only task related subset of procfs.
I'd rather have
mount -t proc -o set=pid
so that is can be naturally extended to
mount -t proc -o set=pid,sysctl,misc
> +static int proc_dir_open(struct inode *inode, struct file *file)
> +{
> + struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
> +
> + if (proc_fs_pidonly(fs_info) == PROC_PIDONLY_ON)
> + return -ENOENT;
> +
> + return 0;
> +}
> +
> /*
> * These are the generic /proc directory operations. They
> * use the in-memory "struct proc_dir_entry" tree to parse
> @@ -338,6 +357,7 @@ static const struct file_operations proc_dir_operations = {
> .llseek = generic_file_llseek,
> .read = generic_read_dir,
> .iterate_shared = proc_readdir,
> + .open = proc_dir_open,
This should not be necessary: if lookup and readdir filters work
then ->open can't happen.
> static int proc_reg_open(struct inode *inode, struct file *file)
> {
> + struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
> struct proc_dir_entry *pde = PDE(inode);
> int rv = 0;
> typeof_member(struct file_operations, open) open;
> typeof_member(struct file_operations, release) release;
> struct pde_opener *pdeo;
>
> + if (proc_fs_pidonly(fs_info) == PROC_PIDONLY_ON)
> + return -ENOENT;
Ditto. Can't open what can't be looked up.
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> +/* definitions for hide_pid field */
> +enum {
> + HIDEPID_OFF = 0,
> + HIDEPID_NO_ACCESS = 1,
> + HIDEPID_INVISIBLE = 2,
> + HIDEPID_NOT_PTRACABLE = 3, /* Limit pids to only ptracable pids */
> +};
These should live in uapi/ as they _are_ user interface to mount().
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Peter Zijlstra @ 2020-01-06 8:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: KP Singh, Rick Edgecombe, linux-kernel, bpf, x86,
linux-security-module, Kees Cook, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, Thomas Garnier, Florent Revest, Brendan Jackman,
Jann Horn, Matthew Garrett, Michael Halcrow
In-Reply-To: <F25C9071-A7A7-4221-BC49-A769E1677EE1@amacapital.net>
On Sat, Jan 04, 2020 at 09:49:10AM +0900, Andy Lutomirski wrote:
> > - 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?
We only care about STRICT_MODULE_RMW (iow, arm, arm64, s390, x86). Of
those only s390 seems to be 'off'.
^ permalink raw reply
* [PATCH] efi: Allow some cert-related UEFI variables to not be present
From: ignat.loskutov @ 2020-01-05 17:34 UTC (permalink / raw)
To: Mimi Zohar, James Morris, Serge E. Hallyn
Cc: linux-security-module, Ignat Loskutov
From: Ignat Loskutov <ignat.loskutov@gmail.com>
get_cert_list() prints an error message if no UEFI variable exists with
the given name. However, the calling code doesn't always consider this
an error. Fix by returning silently in this case.
Signed-off-by: Ignat Loskutov <ignat.loskutov@gmail.com>
---
security/integrity/platform_certs/load_uefi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 111898aad56e..163ede8d2abc 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -43,6 +43,8 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
void *db;
status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+ if (status == EFI_NOT_FOUND)
+ return NULL;
if (status != EFI_BUFFER_TOO_SMALL) {
pr_err("Couldn't get size: 0x%lx\n", status);
return NULL;
--
2.20.1
^ permalink raw reply related
* Re: [GIT PULL] apparmor bug fixes for v5.5-rc5
From: pr-tracker-bot @ 2020-01-05 3:35 UTC (permalink / raw)
To: John Johansen; +Cc: Linus Torvalds, LKLM, open list:SECURITY SUBSYSTEM
In-Reply-To: <cbec7335-39b8-6b7d-402b-a6dd467b492b@canonical.com>
The pull request you sent on Sat, 4 Jan 2020 17:36:48 -0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2020-01-04
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a125bcda2d0aee1d98b51c167aca60fb312572aa
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* [GIT PULL] apparmor bug fixes for v5.5-rc5
From: John Johansen @ 2020-01-05 1:36 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LKLM, open list:SECURITY SUBSYSTEM
Hi Linus,
Can you please pull the following bug fixes for apparmor
Thanks!
- John
The following changes since commit fd6988496e79a6a4bdb514a4655d2920209eb85d:
Linux 5.5-rc4 (2019-12-29 15:29:16 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2020-01-04
for you to fetch changes up to 8c62ed27a12c00e3db1c9f04bc0f272bdbb06734:
apparmor: fix aa_xattrs_match() may sleep while holding a RCU lock (2020-01-04 15:56:44 -0800)
----------------------------------------------------------------
+ Bug fixes
- performance regression: only get a label reference if the fast
path check fails
- fix aa_xattrs_match() may sleep while holding a RCU lock
- fix bind mounts aborting with -ENOMEM
----------------------------------------------------------------
John Johansen (2):
apparmor: only get a label reference if the fast path check fails
apparmor: fix aa_xattrs_match() may sleep while holding a RCU lock
Patrick Steinhardt (1):
apparmor: fix bind mounts aborting with -ENOMEM
security/apparmor/apparmorfs.c | 2 +-
security/apparmor/domain.c | 82 ++++++++++++++++++++++--------------------
security/apparmor/file.c | 12 ++++---
security/apparmor/mount.c | 2 +-
security/apparmor/policy.c | 4 +--
5 files changed, 55 insertions(+), 47 deletions(-)
^ permalink raw reply
* [PATCH v2] ima: Add a space after printing LSM rules for readability
From: Clay Chang @ 2020-01-05 1:18 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge
Cc: linux-kernel, linux-security-module, linux-integrity, Clay Chang
When reading ima_policy from securityfs, there is a missing
space between output string of LSM rules and the remaining
rules.
Signed-off-by: Clay Chang <clayc@hpe.com>
---
security/integrity/ima/ima_policy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ef8dfd47c7e3..1a266e4f99bc 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1496,6 +1496,7 @@ int ima_policy_show(struct seq_file *m, void *v)
(char *)entry->lsm[i].args_p);
break;
}
+ seq_puts(m, " ");
}
}
if (entry->template)
--
2.16.6
^ permalink raw reply related
* Re: [PATCH bpf-next] bpf: Make trampolines W^X
From: Justin Capella @ 2020-01-05 1:19 UTC (permalink / raw)
To: KP Singh
Cc: Andy Lutomirski, Rick Edgecombe, LKML, bpf, x86,
linux-security-module, Kees Cook, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, Thomas Garnier, Florent Revest, Brendan Jackman,
Jann Horn, Matthew Garrett, Michael Halcrow
In-Reply-To: <F25C9071-A7A7-4221-BC49-A769E1677EE1@amacapital.net>
I'm guessing 2 pages are used to allow for different protections? Does
only the first page's protections need to be changed? Is that
"old_image"?
+ set_memory_nx((unsigned long)image, 1);
+ set_memory_rw((unsigned long)image, 1);
+ set_memory_ro((unsigned long)new_image, 1);
+ set_memory_x((unsigned long)new_image, 1);
Because
+ void *old_image = tr->image + ((tr->selector + 1) & 1) * PAGE_SIZE;
+ void *new_image = tr->image + (tr->selector & 1) * PAGE_SIZE
> > - 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.
If there were checks between these steps to verify the trampoline
wasn't tampered with while the page was writable it would make sense
to do so before enabling execution.
Could some of these int's be unsigned to be extra cautious?
One last thought, if the extra checks are implemented, maybe comparing
against the old image prior to setting rw would be worthwhile?
^ permalink raw reply
* Re: [PATCH] ima: Add a space after printing a LSM rule for readability
From: Clay Chang @ 2020-01-05 1:12 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-kernel, linux-security-module, linux-integrity,
dmitry.kasatkin, jmorris, serge
In-Reply-To: <1578071487.5152.13.camel@linux.ibm.com>
On Fri, Jan 03, 2020 at 12:11:27PM -0500, Mimi Zohar wrote:
> On Fri, 2020-01-03 at 15:51 +0800, clayc@hpe.com wrote:
> > From: Clay Chang <clayc@hpe.com>
>
> Normally this "From" line is only seen when the sender isn't the patch
> author. Any ideas what happened?
>
Hi Mimi,
Apparently I should not use "--from" in git-send-email command.
> >
> > When reading ima_policy from securityfs, there is a missing
> > space between output string of LSM rules.
> >
> > Signed-off-by: Clay Chang <clayc@hpe.com>
>
> Good catch! IMA policy rules based on LSM labels are used to
> constrain which files are in policy. Normally a single LSM label is
> enough (e.g. dont_measure obj_type=auditd_log_t). Could you include
> in this patch description a use case where multiple LSM labels are
> needed?
>
Apology for not expressed my intention clearly. The intention of this
patch is to add a space after printing LSM rules (if any) and the
remaining rules.
Currently, if I have a policy, for example:
appraise func=BPRM_CHECK obj_type=shell_exec_t appraise_type=imasig
The read back result is:
appraise func=BPRM_CHECK obj_type=shell_exec_tappraise_type=imasig
which is not correct.
I do not have a case for multiple LSM labels, but if there is one
such case, this patch will also apply.
I will post a v2 patch with tuned description.
Thanks,
Clay
^ permalink raw reply
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