* Re: [RFC PATCH v14 01/10] landlock: Add object and rule management
From: Mickaël Salaün @ 2020-02-26 15:31 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez1FN0B05r35c-EDuQNoW=5ZTy1iBzksbkt+toqs+_tdqg@mail.gmail.com>
On 25/02/2020 21:49, Jann Horn wrote:
> On Mon, Feb 24, 2020 at 5:05 PM Mickaël Salaün <mic@digikod.net> wrote:
>> A Landlock object enables to identify a kernel object (e.g. an inode).
>> A Landlock rule is a set of access rights allowed on an object. Rules
>> are grouped in rulesets that may be tied to a set of processes (i.e.
>> subjects) to enforce a scoped access-control (i.e. a domain).
>>
>> Because Landlock's goal is to empower any process (especially
>> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
>> object identification such as file extended attributes. Indeed, we need
>> innocuous, composable and modular access-controls.
>>
>> The main challenge with this constraints is to identify kernel objects
>> while this identification is useful (i.e. when a security policy makes
>> use of this object). But this identification data should be freed once
>> no policy is using it. This ephemeral tagging should not and may not be
>> written in the filesystem. We then need to manage the lifetime of a
>> rule according to the lifetime of its object. To avoid a global lock,
>> this implementation make use of RCU and counters to safely reference
>> objects.
>>
>> A following commit uses this generic object management for inodes.
> [...]
>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>> new file mode 100644
>> index 000000000000..4a321d5b3f67
>> --- /dev/null
>> +++ b/security/landlock/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config SECURITY_LANDLOCK
>> + bool "Landlock support"
>> + depends on SECURITY
>> + default n
>
> (I think "default n" is implicit?)
It seems that most (all?) Kconfig are written like this.
>
>> + help
>> + This selects Landlock, a safe sandboxing mechanism. It enables to
>> + restrict processes on the fly (i.e. enforce an access control policy),
>> + which can complement seccomp-bpf. The security policy is a set of access
>> + rights tied to an object, which could be a file, a socket or a process.
>> +
>> + See Documentation/security/landlock/ for further information.
>> +
>> + If you are unsure how to answer this question, answer N.
> [...]
>> diff --git a/security/landlock/object.c b/security/landlock/object.c
>> new file mode 100644
>> index 000000000000..38fbbb108120
>> --- /dev/null
>> +++ b/security/landlock/object.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Object and rule management
>> + *
>> + * Copyright © 2016-2020 Mickaël Salaün <mic@digikod.net>
>> + * Copyright © 2018-2020 ANSSI
>> + *
>> + * Principles and constraints of the object and rule management:
>> + * - Do not leak memory.
>> + * - Try as much as possible to free a memory allocation as soon as it is
>> + * unused.
>> + * - Do not use global lock.
>> + * - Do not charge processes other than the one requesting a Landlock
>> + * operation.
>> + */
>> +
>> +#include <linux/bug.h>
>> +#include <linux/compiler.h>
>> +#include <linux/compiler_types.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/fs.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/rbtree.h>
>> +#include <linux/rcupdate.h>
>> +#include <linux/refcount.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "object.h"
>> +
>> +struct landlock_object *landlock_create_object(
>> + const enum landlock_object_type type, void *underlying_object)
>> +{
>> + struct landlock_object *object;
>> +
>> + if (WARN_ON_ONCE(!underlying_object))
>> + return NULL;
>> + object = kzalloc(sizeof(*object), GFP_KERNEL);
>> + if (!object)
>> + return NULL;
>> + refcount_set(&object->usage, 1);
>> + refcount_set(&object->cleaners, 1);
>> + spin_lock_init(&object->lock);
>> + INIT_LIST_HEAD(&object->rules);
>> + object->type = type;
>> + WRITE_ONCE(object->underlying_object, underlying_object);
>
> `object` is not globally visible at this point, so WRITE_ONCE() is unnecessary.
Right. It was written like this to have a uniform use of this pointer,
but I'll remove it.
>
>> + return object;
>> +}
>> +
>> +struct landlock_object *landlock_get_object(struct landlock_object *object)
>> + __acquires(object->usage)
>> +{
>> + __acquire(object->usage);
>> + /*
>> + * If @object->usage equal 0, then it will be ignored by writers, and
>> + * underlying_object->object may be replaced, but this is not an issue
>> + * for release_object().
>> + */
>> + if (object && refcount_inc_not_zero(&object->usage)) {
>> + /*
>> + * It should not be possible to get a reference to an object if
>> + * its underlying object is being terminated (e.g. with
>> + * landlock_release_object()), because an object is only
>> + * modifiable through such underlying object. This is not the
>> + * case with landlock_get_object_cleaner().
>> + */
>> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
>> + return object;
>> + }
>> + return NULL;
>> +}
>> +
>> +static struct landlock_object *get_object_cleaner(
>> + struct landlock_object *object)
>> + __acquires(object->cleaners)
>> +{
>> + __acquire(object->cleaners);
>> + if (object && refcount_inc_not_zero(&object->cleaners))
>> + return object;
>> + return NULL;
>> +}
>
> I don't get this whole "cleaners" thing. Can you give a quick
> description of why this is necessary, and what benefits it has over a
> standard refcounting+RCU scheme? I don't immediately see anything that
> requires this.
This indeed needs more documentation here. Here is a comment I'll add to
get_object_cleaner():
This enables to safely get a reference to an object to potentially free
it if it is not already being freed by a concurrent thread. Indeed, the
object's address may still be read and dereferenced while a concurrent
thread is attempting to clean the object. Cf. &struct
landlock_object->usage and &struct landlock_object->cleaners.
See below the explanation about "usage" and "cleaners".
>
>> +/*
>> + * There is two cases when an object should be free and the reference to the
>> + * underlying object should be put:
>> + * - when the last rule tied to this object is removed, which is handled by
>> + * landlock_put_rule() and then release_object();
>> + * - when the object is being terminated (e.g. no more reference to an inode),
>> + * which is handled by landlock_put_object().
>> + */
>> +static void put_object_free(struct landlock_object *object)
>> + __releases(object->cleaners)
>> +{
>> + __release(object->cleaners);
>> + if (!refcount_dec_and_test(&object->cleaners))
>> + return;
>> + WARN_ON_ONCE(refcount_read(&object->usage));
>> + /*
>> + * Ensures a safe use of @object in the RCU block from
>> + * landlock_put_rule().
>> + */
>> + kfree_rcu(object, rcu_free);
>> +}
>> +
>> +/*
>> + * Destroys a newly created and useless object.
>> + */
>> +void landlock_drop_object(struct landlock_object *object)
>> +{
>> + if (WARN_ON_ONCE(!refcount_dec_and_test(&object->usage)))
>> + return;
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> +}
>> +
>> +/*
>> + * Puts the underlying object (e.g. inode) if it is the first request to
>> + * release @object, without calling landlock_put_object().
>> + *
>> + * Return true if this call effectively marks @object as released, false
>> + * otherwise.
>> + */
>> +static bool release_object(struct landlock_object *object)
>> + __releases(&object->lock)
>> +{
>> + void *underlying_object;
>> +
>> + lockdep_assert_held(&object->lock);
>> +
>> + underlying_object = xchg(&object->underlying_object, NULL);
>> + spin_unlock(&object->lock);
>> + might_sleep();
>> + if (!underlying_object)
>> + return false;
>> +
>> + switch (object->type) {
>> + case LANDLOCK_OBJECT_INODE:
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + }
>> + return true;
>> +}
>> +
>> +static void put_object_cleaner(struct landlock_object *object)
>> + __releases(object->cleaners)
>> +{
>> + /* Let's try an early lockless check. */
>> + if (list_empty(&object->rules) &&
>> + READ_ONCE(object->underlying_object)) {
>> + /*
>> + * Puts @object if there is no rule tied to it and the
>> + * remaining user is the underlying object. This check is
>> + * atomic because @object->rules and @object->underlying_object
>> + * are protected by @object->lock.
>> + */
>> + spin_lock(&object->lock);
>> + if (list_empty(&object->rules) &&
>> + READ_ONCE(object->underlying_object) &&
>> + refcount_dec_if_one(&object->usage)) {
>> + /*
>> + * Releases @object, in place of
>> + * landlock_release_object().
>> + *
>> + * @object is already empty, implying that all its
>> + * previous rules are already disabled.
>> + *
>> + * Unbalance the @object->cleaners counter to reflect
>> + * the underlying object release.
>> + */
>> + if (!WARN_ON_ONCE(!release_object(object))) {
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> + }
>> + } else {
>> + spin_unlock(&object->lock);
>> + }
>> + }
>> + put_object_free(object);
>> +}
>> +
>> +/*
>> + * Putting an object is easy when the object is being terminated, but it is
>> + * much more tricky when the reason is that there is no more rule tied to this
>> + * object. Indeed, new rules could be added at the same time.
>> + */
>> +void landlock_put_object(struct landlock_object *object)
>> + __releases(object->usage)
>> +{
>> + struct landlock_object *object_cleaner;
>> +
>> + __release(object->usage);
>> + might_sleep();
>> + if (!object)
>> + return;
>> + /*
>> + * Guards against concurrent termination to be able to terminate
>> + * @object if it is empty and not referenced by another rule-appender
>> + * other than the underlying object.
>> + */
>> + object_cleaner = get_object_cleaner(object);
>> + if (WARN_ON_ONCE(!object_cleaner)) {
>> + __release(object->cleaners);
>> + return;
>> + }
>> + /*
>> + * Decrements @object->usage and if it reach zero, also decrement
>> + * @object->cleaners. If both reach zero, then release and free
>> + * @object.
>> + */
>> + if (refcount_dec_and_test(&object->usage)) {
>> + struct landlock_rule *rule_walker, *rule_walker2;
>> +
>> + spin_lock(&object->lock);
>> + /*
>> + * Disables all the rules tied to @object when it is forbidden
>> + * to add new rule but still allowed to remove them with
>> + * landlock_put_rule(). This is crucial to be able to safely
>> + * free a rule according to landlock_rule_is_disabled().
>> + */
>> + list_for_each_entry_safe(rule_walker, rule_walker2,
>> + &object->rules, list)
>> + list_del_rcu(&rule_walker->list);
>> +
>> + /*
>> + * Releases @object if it is not already released (e.g. with
>> + * landlock_release_object()).
>> + */
>> + release_object(object);
>> + /*
>> + * Unbalances the @object->cleaners counter to reflect the
>> + * underlying object release.
>> + */
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> + }
>> + put_object_cleaner(object_cleaner);
>> +}
>> +
>> +void landlock_put_rule(struct landlock_object *object,
>> + struct landlock_rule *rule)
>> +{
>> + if (!rule)
>> + return;
>> + WARN_ON_ONCE(!object);
>> + /*
>> + * Guards against a concurrent @object self-destruction with
>> + * landlock_put_object() or put_object_cleaner().
>> + */
>> + rcu_read_lock();
>> + if (landlock_rule_is_disabled(rule)) {
>> + rcu_read_unlock();
>> + if (refcount_dec_and_test(&rule->usage))
>> + kfree_rcu(rule, rcu_free);
>> + return;
>> + }
>> + if (refcount_dec_and_test(&rule->usage)) {
>> + struct landlock_object *safe_object;
>> +
>> + /*
>> + * Now, @rule may still be enabled, or in the process of being
>> + * untied to @object by put_object_cleaner(). However, we know
>> + * that @object will not be freed until rcu_read_unlock() and
>> + * until @object->cleaners reach zero. Furthermore, we may not
>> + * be the only one willing to free a @rule linked with @object.
>> + * If we succeed to hold @object with get_object_cleaner(), we
>> + * know that until put_object_cleaner(), we can safely use
>> + * @object to remove @rule.
>> + */
>> + safe_object = get_object_cleaner(object);
>> + rcu_read_unlock();
>> + if (!safe_object) {
>> + __release(safe_object->cleaners);
>> + /*
>> + * We can safely free @rule because it is already
>> + * removed from @object's list.
>> + */
>> + WARN_ON_ONCE(!landlock_rule_is_disabled(rule));
>> + kfree_rcu(rule, rcu_free);
>> + } else {
>> + spin_lock(&safe_object->lock);
>> + if (!landlock_rule_is_disabled(rule))
>> + list_del(&rule->list);
>> + spin_unlock(&safe_object->lock);
>> + kfree_rcu(rule, rcu_free);
>> + put_object_cleaner(safe_object);
>> + }
>> + } else {
>> + rcu_read_unlock();
>> + }
>> + /*
>> + * put_object_cleaner() might sleep, but it is only reachable if
>> + * !landlock_rule_is_disabled(). Therefore, clean_ref() can not sleep.
>> + */
>> + might_sleep();
>> +}
>> +
>> +void landlock_release_object(struct landlock_object __rcu *rcu_object)
>> +{
>> + struct landlock_object *object;
>> +
>> + if (!rcu_object)
>> + return;
>> + rcu_read_lock();
>> + object = get_object_cleaner(rcu_dereference(rcu_object));
>
> This is not how RCU works. You need the rcu annotation on the access
> to the data structure member (or global variable) that's actually
> being accessed. A "struct foo __rcu *foo" argument is essentially
> always wrong.
Absolutely! I fixed this with the following patch:
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 7f3bd4fd04bb..01a48c75f210 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -98,7 +98,9 @@ void landlock_release_inodes(struct super_block *sb)
if (iput_inode)
iput(iput_inode);
- landlock_release_object(inode_landlock(inode)->object);
+ rcu_read_lock();
+ landlock_release_object(rcu_dereference(
+ inode_landlock(inode)->object));
iput_inode = inode;
spin_lock(&sb->s_inode_list_lock);
diff --git a/security/landlock/object.c b/security/landlock/object.c
index 2d373f224989..a0e65a78068d 100644
--- a/security/landlock/object.c
+++ b/security/landlock/object.c
@@ -300,14 +300,16 @@ void landlock_put_rule(struct landlock_object *object,
might_sleep();
}
-void landlock_release_object(struct landlock_object __rcu *rcu_object)
+void landlock_release_object(struct landlock_object *rcu_object)
+ __releases(RCU)
{
struct landlock_object *object;
- if (!rcu_object)
+ if (!rcu_object) {
+ rcu_read_unlock();
return;
- rcu_read_lock();
- object = get_object_cleaner(rcu_dereference(rcu_object));
+ }
+ object = get_object_cleaner(rcu_object);
rcu_read_unlock();
if (unlikely(!object)) {
__release(object->cleaners);
diff --git a/security/landlock/object.h b/security/landlock/object.h
index 15dfc9a75a82..78bfb25d4bcc 100644
--- a/security/landlock/object.h
+++ b/security/landlock/object.h
@@ -12,9 +12,9 @@
#include <linux/compiler_types.h>
#include <linux/list.h>
#include <linux/poison.h>
-#include <linux/rcupdate.h>
#include <linux/refcount.h>
#include <linux/spinlock.h>
+#include <linux/types.h>
struct landlock_access {
/*
@@ -105,7 +105,8 @@ struct landlock_object {
void landlock_put_rule(struct landlock_object *object,
struct landlock_rule *rule);
-void landlock_release_object(struct landlock_object __rcu *rcu_object);
+void landlock_release_object(struct landlock_object *object)
+ __releases(RCU);
struct landlock_object *landlock_create_object(
const enum landlock_object_type type, void *underlying_object);
>
>> +struct landlock_rule {
>> + struct landlock_access access;
>> + /*
>> + * @list: Linked list with other rules tied to the same object, which
>> + * enable to manage their lifetimes. This is also used to identify if
>> + * a rule is still valid, thanks to landlock_rule_is_disabled(), which
>> + * is important in the matching process because the original object
>> + * address might have been recycled.
>> + */
>> + struct list_head list;
>> + union {
>> + /*
>> + * @usage: Number of rulesets pointing to this rule. This
>> + * field is never used by RCU readers.
>> + */
>> + refcount_t usage;
>> + struct rcu_head rcu_free;
>> + };
>> +};
>
> An object that is subject to RCU but whose refcount must not be
> accessed from RCU context? That seems a weird.
The fields "access" and "list" are read (in a RCU-read block) by
ruleset.c:landlock_find_access() (cf. patch 2). The use of the "usage"
counter is in landlock_insert_ruleset_rule() and landlock_put_rule(),
but in these cases the rule is always owned/held by the caller. I should
say something like "This field must only be used when already holding
the rule."
>
>> +enum landlock_object_type {
>> + LANDLOCK_OBJECT_INODE = 1,
>> +};
>> +
>> +struct landlock_object {
>> + /*
>> + * @usage: Main usage counter, used to tie an object to it's underlying
>> + * object (i.e. create a lifetime) and potentially add new rules.
>
> I can't really follow this by reading this patch on its own. As one
> suggestion to make things at least a bit better, how about documenting
> here that `usage` always reaches zero before `cleaners` does?
What about this?
This counter is used to tie an object to its underlying object (e.g. an
inode) and to modify it (e.g. add or remove a rule). If this counter
reaches zero, the object must not be modified, but it may still be used
from within an RCU-read block. When adding a new rule to an object with
a usage counter of zero, the underlying object must be locked and its
object pointer can then be replaced with a new empty object (while
ignoring the disabled object which is being handled by another thread).
This counter always reaches zero before @cleaners does.
>
>> + */
>> + refcount_t usage;
>> + /*
>> + * @cleaners: Usage counter used to free a rule from @rules (thanks to
>> + * put_rule()). Enables to get a reference to this object until it
>> + * really become freed. Cf. put_object().
>
> Maybe add: @usage being non-zero counts as one reference to @cleaners.
> Once @cleaners has become zero, the object is freed after an RCU grace
> period.
What about this?
This counter can only reach zero if the @usage counter already reached
zero. Indeed, @usage being non-zero counts as one reference to
@cleaners. Once @cleaners has become zero, the object is freed after an
RCU grace period. This enables concurrent threads to safely get an
object reference to terminate it if there is no more concurrent cleaners
for this object. This mechanism is required to enable concurrent threads
to safely dereference an object from potentially different pointers
(e.g. the underlying object, or a rule tied to this object), to
potentially terminate and free it (i.e. if there is no more rules tied
to it, or if the underlying object is being terminated).
>
>> + */
>> + refcount_t cleaners;
>> + union {
>> + /*
>> + * The use of this struct is controlled by @usage and
>> + * @cleaners, which makes it safe to union it with @rcu_free.
>> + */
> [...]
>> + struct rcu_head rcu_free;
>> + };
>> +};
> [...]
>> +static inline bool landlock_rule_is_disabled(
>> + struct landlock_rule *rule)
>> +{
>> + /*
>> + * Disabling (i.e. unlinking) a landlock_rule is a one-way operation.
>> + * It is not possible to re-enable such a rule, then there is no need
>> + * for smp_load_acquire().
>> + *
>> + * LIST_POISON2 is set by list_del() and list_del_rcu().
>> + */
>> + return !rule || READ_ONCE(rule->list.prev) == LIST_POISON2;
>
> You're not allowed to do this, the comment above list_del() states:
>
> * Note: list_empty() on entry does not return true after this, the entry is
> * in an undefined state.
list_del() checks READ_ONCE(head->next) == head, but
landlock_rule_is_disabled() checks READ_ONCE(rule->list.prev) ==
LIST_POISON2.
The comment about LIST_POISON2 is right but may be misleading. There is
no use of list_empty() with a landlock_rule->list, only
landlock_object->rules. The only list_del() is in landlock_put_rule()
when there is a guarantee that there is no other reference to it, hence
no possible use of landlock_rule_is_disabled() with this rule. I could
replace it with a call to list_del_rcu() to make it more consistent.
>
> If you want to be able to test whether the element is on a list
> afterwards, use stuff like list_del_init().
There is no need to re-initialize the list but using list_del_init() and
list_empty() could work too. However, there is no list_del_init_rcu()
helper. Moreover, resetting the list's pointer with LIST_POISON2 might
help to detect bugs.
Thanks for this review!
^ permalink raw reply related
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Mickaël Salaün @ 2020-02-26 15:34 UTC (permalink / raw)
To: J Freyensee, linux-kernel
Cc: Al Viro, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Greg Kroah-Hartman, James Morris, Jann Horn, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün,
Serge E . Hallyn, Shuah Khan, Vincent Dagonneau, kernel-hardening,
linux-api, linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <6df3e6b1-ffd1-dacf-2f2d-7df8e5aca668@gmail.com>
On 25/02/2020 19:49, J Freyensee wrote:
>
>
> On 2/24/20 8:02 AM, Mickaël Salaün wrote:
>
>> ## Syscall
>>
>> Because it is only tested on x86_64, the syscall is only wired up for
>> this architecture. The whole x86 family (and probably all the others)
>> will be supported in the next patch series.
> General question for u. What is it meant "whole x86 family will be
> supported". 32-bit x86 will be supported?
Yes, I was referring to x86_32, x86_64 and x32, but all architectures
should be supported.
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Casey Schaufler @ 2020-02-26 15:35 UTC (permalink / raw)
To: KP Singh
Cc: Alexei Starovoitov, Kees Cook, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev, Casey Schaufler
In-Reply-To: <20200226051535.GA17117@chromium.org>
On 2/25/2020 9:15 PM, KP Singh wrote:
> On 25-Feb 16:30, Casey Schaufler wrote:
>> On 2/24/2020 9:41 PM, Alexei Starovoitov wrote:
>>> On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
>>>> But the LSM subsystem doesn't want special cases (Casey has worked very
>>>> hard to generalize everything there for stacking). It is really hard to
>>>> accept adding a new special case when there are still special cases yet
>>>> to be worked out even in the LSM code itself[2].
>>>> [2] Casey's work to generalize the LSM interfaces continues and it quite
>>>> complex:
>>>> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
>>> I think the key mistake we made is that we classified KRSI as LSM.
>>> LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
>>> I don't see anything in LSM infra that KRSI can reuse.
>>> The only thing BPF needs is a function to attach to.
>>> It can be a nop function or any other.
>>> security_*() functions are interesting from that angle only.
>>> Hence I propose to reconsider what I was suggesting earlier.
>>> No changes to secruity/ directory.
>>> Attach to security_*() funcs via bpf trampoline.
>>> The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
>>> I think "security" is also loaded word that should be avoided.
>> No argument there.
>>
>>> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
>>>
>>>> So, unless James is going to take this over Casey's objections, the path
>>>> forward I see here is:
>>>>
>>>> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
>>>> - optimize calling for all LSMs
>>> I'm very much surprised how 'slow' KRSI is an option at all.
>>> 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
>>> functions for every place in the kernel that calls security_*().
>>> This is not an acceptable overhead. Even w/o retpoline
>>> this is not something datacenter servers can use.
>> In the universe I live in data centers will disable hyper-threading,
>> reducing performance substantially, in the face of hypothetical security
>> exploits. That's a massively greater performance impact than the handful
>> of instructions required to do indirect calls. Not to mention the impact
> Indirect calls have worse performance implications than just a few
> instructions and are especially not suitable for hotpaths.
>
> There have been multiple efforts to reduce their usage e.g.:
>
> - https://lwn.net/Articles/774743/
> - https://lwn.net/Articles/773985/
>
>> of the BPF programs that have been included. Have you ever looked at what
> BPF programs are JIT'ed and optimized to native code.
Doesn't mean people won't write slow code.
>> happens to system performance when polkitd is enabled?
> However, let's discuss all this separately when we follow-up with
> performance improvements after submitting the initial patch-set.
Think performance up front. Don't ignore issues.
>>> Another option is to do this:
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 64b19f050343..7887ce636fb1 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>>> return kernel_load_data_str[id];
>>> }
>>>
>>> -#ifdef CONFIG_SECURITY
>>> +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
>>>
>>> Single line change to security.h and new file kernel/bpf/override_security.c
>>> that will look like:
>>> int security_binder_set_context_mgr(struct task_struct *mgr)
>>> {
>>> return 0;
>>> }
>>>
>>> int security_binder_transaction(struct task_struct *from,
>>> struct task_struct *to)
>>> {
>>> return 0;
>>> }
>>> Essentially it will provide BPF side with a set of nop functions.
>>> CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
>>> on kernel users. Either they build the kernel with CONFIG_SECURITY and their
>>> choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
>>> BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
>>> it's a pro not a con.
>> Err, no. All distros use an LSM or two. Unless you can re-implement SELinux
> The users mentioned here in this context are (I would assume) the more
> performance sensitive users who would, potentially, disable
> CONFIG_SECURITY because of the current performance characteristics.
You assume that the most performance sensitive people would allow
a mechanism to arbitrarily add overhead that is out of their control?
How does that make sense?
> We can also discuss this separately and only if we find that we need
> it for the BPF_OVERRIDE_RET type attachment.
>
> - KP
>
>> in BPF (good luck with state transitions) you've built a warp drive without
>> ever having mined dilithium crystals.
>>
>>
^ permalink raw reply
* Re: [PATCH] module support: during lockdown, log name of unsigned module
From: Jessica Yu @ 2020-02-26 17:58 UTC (permalink / raw)
To: Martin Haass; +Cc: linux-kernel, linux-security-module, linux-modules
In-Reply-To: <CAH3oDPzeu_bzYa3fOUpcjQk4HJ5K2Rx+Qf+qbqxSrmTdrWHm5g@mail.gmail.com>
+++ Martin Haass [19/02/20 10:02 +0100]:
>during lockdown loading of unsigned modules is restricted to signed
>modules only. The old error message does not show which module misses
>the signature, making it very difficult for a user to determine which
>module is at fault.
>This patch adds a line to the logs which additionally contains the
>module name that caused the error message. The old message cannot
>be replaced as it is generated by lockdown_is_locked_down
>---
> kernel/module.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 33569a01d6e..6dcb28139a0 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2807,7 +2807,8 @@ static int module_sig_check(struct load_info *info,
>int flags)
> const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> const char *reason;
> const void *mod = info->hdr;
>-
>+ int is_locked = -EPERM;
>+
> /*
> * Require flags == 0, as a module with version information
> * removed is no longer the module that was signed
>@@ -2843,7 +2844,12 @@ static int module_sig_check(struct load_info *info,
>int flags)
> return -EKEYREJECTED;
> }
>
>- return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
>+ is_locked = security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
>+ if (is_locked == -EPERM) {
>+ pr_notice("Lockdown: %s: rejected module '%s' cause: %s",
>+ current->comm, info->name, reason);
>+ }
>+ return is_locked;
Hi!
Actually, I think we can just reuse the pr_notice() from the previous if
(is_module_sig_enforced()) block. It already logs the module name as well as
the reason. And we'd better leave the lockdown-specific messages to the LSM.
Something like this perhaps?
diff --git a/kernel/module.c b/kernel/module.c
index b88ec9cd2a7f..2c881e3b9d92 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2838,12 +2838,13 @@ static int module_sig_check(struct load_info *info, int flags)
case -ENOKEY:
reason = "Loading of module with unavailable key";
decide:
- if (is_module_sig_enforced()) {
+ err = is_module_sig_enforced() ? \
+ -EKEYREJECTED : security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
+
+ if (err)
pr_notice("%s: %s is rejected\n", info->name, reason);
- return -EKEYREJECTED;
- }
- return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
+ return err;
/* All other errors are fatal, including nomem, unparseable
* signatures and signature check failures - even if signatures
^ permalink raw reply related
* Re: suspicious RCU usage from smack code
From: Casey Schaufler @ 2020-02-26 20:06 UTC (permalink / raw)
To: John Garry, jmorris@namei.org, serge@hallyn.com,
linux-security-module
Cc: Anders Roxell, Casey Schaufler
In-Reply-To: <9d97e54f-a7d3-30fa-de4c-ae8d70dee087@huawei.com>
On 2/25/2020 9:59 AM, John Garry wrote:
> Hi guys,
>
> JFYI, When I enable CONFIG_PROVE_RCU=y, I get these:
>
> [ 0.369697] WARNING: suspicious RCU usage
> [ 0.374179] 5.6.0-rc3-00002-g619882231229-dirty #1753 Not tainted
> [ 0.380974] -----------------------------
> [ 0.385455] security/smack/smack_lsm.c:354 RCU-list traversed in non-reader section!!
> [ 0.394183]
> [ 0.394183] other info that might help us debug this:
> [ 0.394183]
> [ 0.403107]
> [ 0.403107] rcu_scheduler_active = 1, debug_locks = 1
> [ 0.410389] no locks held by kthreadd/2.
> [ 0.414770]
> [ 0.414770] stack backtrace:
> [ 0.419636] CPU: 0 PID: 2 Comm: kthreadd Not tainted 5.6.0-rc3-00002-g619882231229-dirty #1753
> [ 0.429204] Call trace:
> [ 0.431924] dump_backtrace+0x0/0x298
> [ 0.435990] show_stack+0x14/0x20
> [ 0.439674] dump_stack+0x118/0x190
> [ 0.443548] lockdep_rcu_suspicious+0xe0/0x120
> [ 0.448487] smack_cred_prepare+0x2f8/0x310
> [ 0.453134] security_prepare_creds+0x64/0xe0
> [ 0.457979] prepare_creds+0x25c/0x368
> [ 0.462141] copy_creds+0x40/0x620
> [ 0.465918] copy_process+0x62c/0x25e0
> [ 0.470084] _do_fork+0xc0/0x998
> [ 0.473667] kernel_thread+0xa0/0xc8
> [ 0.477640] kthreadd+0x2b0/0x408
> [ 0.481325] ret_from_fork+0x10/0x18
>
> [ 18.804382] =============================
> [ 18.808872] WARNING: suspicious RCU usage
> [ 18.813348] 5.6.0-rc3-00002-g619882231229-dirty #1753 Not tainted
> [ 18.820145] -----------------------------
> [ 18.824621] security/smack/smack_access.c:87 RCU-list traversed in non-reader section!!
> [ 18.833544]
> [ 18.833544] other info that might help us debug this:
> [ 18.833544]
> [ 18.842465]
> [ 18.842465] rcu_scheduler_active = 1, debug_locks = 1
> [ 18.849741] no locks held by kdevtmpfs/781.
> [ 18.854410]
> [ 18.854410] stack backtrace:
> [ 18.859277] CPU: 1 PID: 781 Comm: kdevtmpfs Not tainted 5.6.0-rc3-00002-g619882231229-dirty #1753
> [ 18.869138] Call trace:
> [ 18.871860] dump_backtrace+0x0/0x298
> [ 18.875929] show_stack+0x14/0x20
> [ 18.879612] dump_stack+0x118/0x190
> [ 18.883489] lockdep_rcu_suspicious+0xe0/0x120
> [ 18.888428] smk_access_entry+0x110/0x128
> [ 18.892885] smk_tskacc+0x70/0xe8
> [ 18.896568] smk_curacc+0x64/0x78
> [ 18.900249] smack_inode_permission+0x110/0x1c8
> [ 18.905284] security_inode_permission+0x50/0x98
> [ 18.910412] inode_permission+0x70/0x1d0
> [ 18.914768] link_path_walk.part.38+0x4a8/0x778
> [ 18.919802] path_lookupat+0xd0/0x1a8
> [ 18.923871] filename_lookup+0xf0/0x1f8
> [ 18.928136] user_path_at_empty+0x48/0x58
> [ 18.932590] ksys_chdir+0x8c/0x138
> [ 18.936366] devtmpfsd+0x148/0x448
> [ 18.940146] kthread+0x1c8/0x1d0
> [ 18.943732] ret_from_fork+0x10/0x18
>
> I haven't had a chance to check whether they are bogus or not.
This is the case where a process with local rules is forked,
requiring that a copy of its rules be made for the child. The
local rules are not used in any production environment that I
know of. It was done for MeeGo. The local rule list really ought
to get moved from the cred to the task blob, now that we have one.
If this turns out to be real it's unlikely to cause any harm as
no one is using local rules. The real fix would take take it out
of the cred blob, and that's a significant change. The RCU details
should be reviewed at that time.
Unless this is worse than it looks.
>
> Thanks,
> John
^ permalink raw reply
* Re: [RFC PATCH v14 01/10] landlock: Add object and rule management
From: Jann Horn @ 2020-02-26 20:24 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <67465638-e22c-5d1a-df37-862b31d999a1@digikod.net>
On Wed, Feb 26, 2020 at 4:32 PM Mickaël Salaün <mic@digikod.net> wrote:
> On 25/02/2020 21:49, Jann Horn wrote:
> > On Mon, Feb 24, 2020 at 5:05 PM Mickaël Salaün <mic@digikod.net> wrote:
> >> A Landlock object enables to identify a kernel object (e.g. an inode).
> >> A Landlock rule is a set of access rights allowed on an object. Rules
> >> are grouped in rulesets that may be tied to a set of processes (i.e.
> >> subjects) to enforce a scoped access-control (i.e. a domain).
> >>
> >> Because Landlock's goal is to empower any process (especially
> >> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
> >> object identification such as file extended attributes. Indeed, we need
> >> innocuous, composable and modular access-controls.
> >>
> >> The main challenge with this constraints is to identify kernel objects
> >> while this identification is useful (i.e. when a security policy makes
> >> use of this object). But this identification data should be freed once
> >> no policy is using it. This ephemeral tagging should not and may not be
> >> written in the filesystem. We then need to manage the lifetime of a
> >> rule according to the lifetime of its object. To avoid a global lock,
> >> this implementation make use of RCU and counters to safely reference
> >> objects.
> >>
> >> A following commit uses this generic object management for inodes.
[...]
> >> +config SECURITY_LANDLOCK
> >> + bool "Landlock support"
> >> + depends on SECURITY
> >> + default n
> >
> > (I think "default n" is implicit?)
>
> It seems that most (all?) Kconfig are written like this.
See e.g. <https://lore.kernel.org/lkml/c187bb77-e804-93bd-64db-9418be58f191@infradead.org/>.
[...]
> >> + return object;
> >> +}
> >> +
> >> +struct landlock_object *landlock_get_object(struct landlock_object *object)
> >> + __acquires(object->usage)
> >> +{
> >> + __acquire(object->usage);
> >> + /*
> >> + * If @object->usage equal 0, then it will be ignored by writers, and
> >> + * underlying_object->object may be replaced, but this is not an issue
> >> + * for release_object().
> >> + */
> >> + if (object && refcount_inc_not_zero(&object->usage)) {
> >> + /*
> >> + * It should not be possible to get a reference to an object if
> >> + * its underlying object is being terminated (e.g. with
> >> + * landlock_release_object()), because an object is only
> >> + * modifiable through such underlying object. This is not the
> >> + * case with landlock_get_object_cleaner().
> >> + */
> >> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
> >> + return object;
> >> + }
> >> + return NULL;
> >> +}
> >> +
> >> +static struct landlock_object *get_object_cleaner(
> >> + struct landlock_object *object)
> >> + __acquires(object->cleaners)
> >> +{
> >> + __acquire(object->cleaners);
> >> + if (object && refcount_inc_not_zero(&object->cleaners))
> >> + return object;
> >> + return NULL;
> >> +}
> >
> > I don't get this whole "cleaners" thing. Can you give a quick
> > description of why this is necessary, and what benefits it has over a
> > standard refcounting+RCU scheme? I don't immediately see anything that
> > requires this.
>
> This indeed needs more documentation here. Here is a comment I'll add to
> get_object_cleaner():
>
> This enables to safely get a reference to an object to potentially free
> it if it is not already being freed by a concurrent thread.
"get a reference to an object to potentially free it" just sounds all
wrong to me. You free an object when you're *dropping* a reference to
it. Your refcounting scheme doesn't fit my mental models of how normal
refcounting works at all...
[...]
> >> +/*
> >> + * Putting an object is easy when the object is being terminated, but it is
> >> + * much more tricky when the reason is that there is no more rule tied to this
> >> + * object. Indeed, new rules could be added at the same time.
> >> + */
> >> +void landlock_put_object(struct landlock_object *object)
> >> + __releases(object->usage)
> >> +{
> >> + struct landlock_object *object_cleaner;
> >> +
> >> + __release(object->usage);
> >> + might_sleep();
> >> + if (!object)
> >> + return;
> >> + /*
> >> + * Guards against concurrent termination to be able to terminate
> >> + * @object if it is empty and not referenced by another rule-appender
> >> + * other than the underlying object.
> >> + */
> >> + object_cleaner = get_object_cleaner(object);
[...]
> >> + /*
> >> + * Decrements @object->usage and if it reach zero, also decrement
> >> + * @object->cleaners. If both reach zero, then release and free
> >> + * @object.
> >> + */
> >> + if (refcount_dec_and_test(&object->usage)) {
> >> + struct landlock_rule *rule_walker, *rule_walker2;
> >> +
> >> + spin_lock(&object->lock);
> >> + /*
> >> + * Disables all the rules tied to @object when it is forbidden
> >> + * to add new rule but still allowed to remove them with
> >> + * landlock_put_rule(). This is crucial to be able to safely
> >> + * free a rule according to landlock_rule_is_disabled().
> >> + */
> >> + list_for_each_entry_safe(rule_walker, rule_walker2,
> >> + &object->rules, list)
> >> + list_del_rcu(&rule_walker->list);
So... rules don't take references on the landlock_objects they use?
Instead, the landlock_object knows which rules use it, and when the
landlock_object goes away, it nukes all the rules associated with
itself?
That seems terrible to me - AFAICS it means that if some random
process decides to install a landlock rule that uses inode X, and then
that process dies together with all its landlock rules, the inode
still stays pinned in kernel memory as long as the superblock is
mounted. In other words, it's a resource leak. (And if I'm not missing
something in patch 5, that applies even if the inode has been
unlinked?)
Can you please refactor your refcounting as follows?
- A rule takes a reference on each landlock_object it uses.
- A landlock_object takes a reference on the underlying object (just like now).
- The underlying object *DOES NOT* take a reference on the
landlock_object (unlike now); the reference from the underlying object
to the landlock_object has weak pointer semantics.
- When a landlock_object's refcount drops to zero (iow no rules use
it anymore), it is freed.
That might also help get rid of the awkward ->cleaners thing?
> >> + /*
> >> + * Releases @object if it is not already released (e.g. with
> >> + * landlock_release_object()).
> >> + */
> >> + release_object(object);
> >> + /*
> >> + * Unbalances the @object->cleaners counter to reflect the
> >> + * underlying object release.
> >> + */
> >> + __acquire(object->cleaners);
> >> + put_object_free(object);
> >> + }
> >> + put_object_cleaner(object_cleaner);
> >> +}
[...]
> >> +static inline bool landlock_rule_is_disabled(
> >> + struct landlock_rule *rule)
> >> +{
> >> + /*
> >> + * Disabling (i.e. unlinking) a landlock_rule is a one-way operation.
> >> + * It is not possible to re-enable such a rule, then there is no need
> >> + * for smp_load_acquire().
> >> + *
> >> + * LIST_POISON2 is set by list_del() and list_del_rcu().
> >> + */
> >> + return !rule || READ_ONCE(rule->list.prev) == LIST_POISON2;
> >
> > You're not allowed to do this, the comment above list_del() states:
> >
> > * Note: list_empty() on entry does not return true after this, the entry is
> > * in an undefined state.
>
> list_del() checks READ_ONCE(head->next) == head, but
> landlock_rule_is_disabled() checks READ_ONCE(rule->list.prev) ==
> LIST_POISON2.
> The comment about LIST_POISON2 is right but may be misleading. There is
> no use of list_empty() with a landlock_rule->list, only
> landlock_object->rules. The only list_del() is in landlock_put_rule()
> when there is a guarantee that there is no other reference to it, hence
> no possible use of landlock_rule_is_disabled() with this rule. I could
> replace it with a call to list_del_rcu() to make it more consistent.
>
> >
> > If you want to be able to test whether the element is on a list
> > afterwards, use stuff like list_del_init().
>
> There is no need to re-initialize the list but using list_del_init() and
> list_empty() could work too. However, there is no list_del_init_rcu()
> helper. Moreover, resetting the list's pointer with LIST_POISON2 might
> help to detect bugs.
Either way, you are currently using the list_head API in a way that
goes against what the header documents. If you want to rely on
list_del() bringing the object into a specific state, then you can't
leave the comment above list_del() as-is that says that it puts the
object in an undefined state; and this kind of check should probably
be done in a helper in list.h instead of open-coding the check for
LIST_POISON2.
^ permalink raw reply
* Re: [RFC PATCH v14 05/10] fs,landlock: Support filesystem access-control
From: Jann Horn @ 2020-02-26 20:29 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <20200224160215.4136-6-mic@digikod.net>
On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
> +static inline u32 get_mem_access(unsigned long prot, bool private)
> +{
> + u32 access = LANDLOCK_ACCESS_FS_MAP;
> +
> + /* Private mapping do not write to files. */
> + if (!private && (prot & PROT_WRITE))
> + access |= LANDLOCK_ACCESS_FS_WRITE;
> + if (prot & PROT_READ)
> + access |= LANDLOCK_ACCESS_FS_READ;
> + if (prot & PROT_EXEC)
> + access |= LANDLOCK_ACCESS_FS_EXECUTE;
> + return access;
> +}
When I do the following, is landlock going to detect that the mmap()
is a read access, or is it incorrectly going to think that it's
neither read nor write?
$ cat write-only.c
#include <fcntl.h>
#include <sys/mman.h>
#include <stdio.h>
int main(void) {
int fd = open("/etc/passwd", O_RDONLY);
char *ptr = mmap(NULL, 0x1000, PROT_WRITE, MAP_PRIVATE, fd, 0);
printf("'%.*s'\n", 4, ptr);
}
$ gcc -o write-only write-only.c -Wall
$ ./write-only
'root'
$
^ permalink raw reply
* Re: [PATCH] apparmor: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-02-27 12:53 UTC (permalink / raw)
To: John Johansen, James Morris, Serge E. Hallyn
Cc: linux-security-module, linux-kernel
In-Reply-To: <20200211210427.GA28952@embeddedor>
Hi all,
Friendly ping: Who can take this?
Thanks
--
Gustavo
On 2/11/20 15:04, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertenly introduced[3] to the codebase from now on.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> security/apparmor/apparmorfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index fefee040bf79..cc81080efb63 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -804,7 +804,7 @@ static ssize_t query_label(char *buf, size_t buf_len,
> struct multi_transaction {
> struct kref count;
> ssize_t size;
> - char data[0];
> + char data[];
> };
>
> #define MULTI_TRANSACTION_LIMIT (PAGE_SIZE - sizeof(struct multi_transaction))
>
^ permalink raw reply
* [PATCH] security: integrity: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-02-27 12:58 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: linux-integrity, linux-security-module, linux-kernel,
Gustavo A. R. Silva
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
security/integrity/ima/ima.h | 2 +-
security/integrity/integrity.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 64317d95363e..da4246ee7e35 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -95,7 +95,7 @@ struct ima_template_entry {
u8 digest[TPM_DIGEST_SIZE]; /* sha1 or md5 measurement hash */
struct ima_template_desc *template_desc; /* template descriptor */
u32 template_data_len;
- struct ima_field_data template_data[0]; /* template related data */
+ struct ima_field_data template_data[]; /* template related data */
};
struct ima_queue_entry {
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 543d277c7e48..3c7e8b902256 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -103,7 +103,7 @@ struct ima_digest_data {
} ng;
u8 data[2];
} xattr;
- u8 digest[0];
+ u8 digest[];
} __packed;
/*
@@ -115,7 +115,7 @@ struct signature_v2_hdr {
uint8_t hash_algo; /* Digest algorithm [enum hash_algo] */
__be32 keyid; /* IMA key identifier - not X509/PGP specific */
__be16 sig_size; /* signature size */
- uint8_t sig[0]; /* signature payload */
+ uint8_t sig[]; /* signature payload */
} __packed;
/* integrity data associated with an inode */
--
2.25.0
^ permalink raw reply related
* Re: [RFC PATCH v14 05/10] fs,landlock: Support filesystem access-control
From: Mickaël Salaün @ 2020-02-27 16:50 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez36SMrPPgsj0omcVukRLwOzBzqWOQjuGCmmmrmsGiNukw@mail.gmail.com>
On 26/02/2020 21:29, Jann Horn wrote:
> On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
>> +static inline u32 get_mem_access(unsigned long prot, bool private)
>> +{
>> + u32 access = LANDLOCK_ACCESS_FS_MAP;
>> +
>> + /* Private mapping do not write to files. */
>> + if (!private && (prot & PROT_WRITE))
>> + access |= LANDLOCK_ACCESS_FS_WRITE;
>> + if (prot & PROT_READ)
>> + access |= LANDLOCK_ACCESS_FS_READ;
>> + if (prot & PROT_EXEC)
>> + access |= LANDLOCK_ACCESS_FS_EXECUTE;
>> + return access;
>> +}
>
> When I do the following, is landlock going to detect that the mmap()
> is a read access, or is it incorrectly going to think that it's
> neither read nor write?
>
> $ cat write-only.c
> #include <fcntl.h>
> #include <sys/mman.h>
> #include <stdio.h>
> int main(void) {
> int fd = open("/etc/passwd", O_RDONLY);
> char *ptr = mmap(NULL, 0x1000, PROT_WRITE, MAP_PRIVATE, fd, 0);
> printf("'%.*s'\n", 4, ptr);
> }
> $ gcc -o write-only write-only.c -Wall
> $ ./write-only
> 'root'
> $
>
Thanks to the "if (!private && (prot & PROT_WRITE))", Landlock allows
this private mmap (as intended) even if there is no write access to this
file, but not with a shared mmap (and a file opened with O_RDWR). I just
added a test for this to be sure.
However, I'm not sure this hook is useful for now. Indeed, the process
still need to have a file descriptor open with the right accesses.
^ permalink raw reply
* Re: [RFC PATCH v14 05/10] fs,landlock: Support filesystem access-control
From: Jann Horn @ 2020-02-27 16:51 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <34319b76-44bd-8915-fd7c-5147f901615e@digikod.net>
On Thu, Feb 27, 2020 at 5:50 PM Mickaël Salaün <mic@digikod.net> wrote:
> On 26/02/2020 21:29, Jann Horn wrote:
> > On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
> >> +static inline u32 get_mem_access(unsigned long prot, bool private)
> >> +{
> >> + u32 access = LANDLOCK_ACCESS_FS_MAP;
> >> +
> >> + /* Private mapping do not write to files. */
> >> + if (!private && (prot & PROT_WRITE))
> >> + access |= LANDLOCK_ACCESS_FS_WRITE;
> >> + if (prot & PROT_READ)
> >> + access |= LANDLOCK_ACCESS_FS_READ;
> >> + if (prot & PROT_EXEC)
> >> + access |= LANDLOCK_ACCESS_FS_EXECUTE;
> >> + return access;
> >> +}
[...]
> However, I'm not sure this hook is useful for now. Indeed, the process
> still need to have a file descriptor open with the right accesses.
Yeah, agreed.
^ permalink raw reply
* Re: [RFC PATCH v14 01/10] landlock: Add object and rule management
From: Mickaël Salaün @ 2020-02-27 16:46 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez33WjzAee9h_Nfxi6vbnjognsKziv=whi_7ocT36DCXcg@mail.gmail.com>
On 26/02/2020 21:24, Jann Horn wrote:
> On Wed, Feb 26, 2020 at 4:32 PM Mickaël Salaün <mic@digikod.net> wrote:
>> On 25/02/2020 21:49, Jann Horn wrote:
>>> On Mon, Feb 24, 2020 at 5:05 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>> A Landlock object enables to identify a kernel object (e.g. an inode).
>>>> A Landlock rule is a set of access rights allowed on an object. Rules
>>>> are grouped in rulesets that may be tied to a set of processes (i.e.
>>>> subjects) to enforce a scoped access-control (i.e. a domain).
>>>>
>>>> Because Landlock's goal is to empower any process (especially
>>>> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
>>>> object identification such as file extended attributes. Indeed, we need
>>>> innocuous, composable and modular access-controls.
>>>>
>>>> The main challenge with this constraints is to identify kernel objects
>>>> while this identification is useful (i.e. when a security policy makes
>>>> use of this object). But this identification data should be freed once
>>>> no policy is using it. This ephemeral tagging should not and may not be
>>>> written in the filesystem. We then need to manage the lifetime of a
>>>> rule according to the lifetime of its object. To avoid a global lock,
>>>> this implementation make use of RCU and counters to safely reference
>>>> objects.
>>>>
>>>> A following commit uses this generic object management for inodes.
> [...]
>>>> +config SECURITY_LANDLOCK
>>>> + bool "Landlock support"
>>>> + depends on SECURITY
>>>> + default n
>>>
>>> (I think "default n" is implicit?)
>>
>> It seems that most (all?) Kconfig are written like this.
>
> See e.g. <https://lore.kernel.org/lkml/c187bb77-e804-93bd-64db-9418be58f191@infradead.org/>.
Ok, done.
>
> [...]
>>>> + return object;
>>>> +}
>>>> +
>>>> +struct landlock_object *landlock_get_object(struct landlock_object *object)
>>>> + __acquires(object->usage)
>>>> +{
>>>> + __acquire(object->usage);
>>>> + /*
>>>> + * If @object->usage equal 0, then it will be ignored by writers, and
>>>> + * underlying_object->object may be replaced, but this is not an issue
>>>> + * for release_object().
>>>> + */
>>>> + if (object && refcount_inc_not_zero(&object->usage)) {
>>>> + /*
>>>> + * It should not be possible to get a reference to an object if
>>>> + * its underlying object is being terminated (e.g. with
>>>> + * landlock_release_object()), because an object is only
>>>> + * modifiable through such underlying object. This is not the
>>>> + * case with landlock_get_object_cleaner().
>>>> + */
>>>> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
>>>> + return object;
>>>> + }
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static struct landlock_object *get_object_cleaner(
>>>> + struct landlock_object *object)
>>>> + __acquires(object->cleaners)
>>>> +{
>>>> + __acquire(object->cleaners);
>>>> + if (object && refcount_inc_not_zero(&object->cleaners))
>>>> + return object;
>>>> + return NULL;
>>>> +}
>>>
>>> I don't get this whole "cleaners" thing. Can you give a quick
>>> description of why this is necessary, and what benefits it has over a
>>> standard refcounting+RCU scheme? I don't immediately see anything that
>>> requires this.
>>
>> This indeed needs more documentation here. Here is a comment I'll add to
>> get_object_cleaner():
>>
>> This enables to safely get a reference to an object to potentially free
>> it if it is not already being freed by a concurrent thread.
>
> "get a reference to an object to potentially free it" just sounds all
> wrong to me. You free an object when you're *dropping* a reference to
> it. Your refcounting scheme doesn't fit my mental models of how normal
> refcounting works at all...
Unfortunately, as I explain below, it is a bit tricky.
>
> [...]
>>>> +/*
>>>> + * Putting an object is easy when the object is being terminated, but it is
>>>> + * much more tricky when the reason is that there is no more rule tied to this
>>>> + * object. Indeed, new rules could be added at the same time.
>>>> + */
>>>> +void landlock_put_object(struct landlock_object *object)
>>>> + __releases(object->usage)
>>>> +{
>>>> + struct landlock_object *object_cleaner;
>>>> +
>>>> + __release(object->usage);
>>>> + might_sleep();
>>>> + if (!object)
>>>> + return;
>>>> + /*
>>>> + * Guards against concurrent termination to be able to terminate
>>>> + * @object if it is empty and not referenced by another rule-appender
>>>> + * other than the underlying object.
>>>> + */
>>>> + object_cleaner = get_object_cleaner(object);
> [...]
>>>> + /*
>>>> + * Decrements @object->usage and if it reach zero, also decrement
>>>> + * @object->cleaners. If both reach zero, then release and free
>>>> + * @object.
>>>> + */
>>>> + if (refcount_dec_and_test(&object->usage)) {
>>>> + struct landlock_rule *rule_walker, *rule_walker2;
>>>> +
>>>> + spin_lock(&object->lock);
>>>> + /*
>>>> + * Disables all the rules tied to @object when it is forbidden
>>>> + * to add new rule but still allowed to remove them with
>>>> + * landlock_put_rule(). This is crucial to be able to safely
>>>> + * free a rule according to landlock_rule_is_disabled().
>>>> + */
>>>> + list_for_each_entry_safe(rule_walker, rule_walker2,
>>>> + &object->rules, list)
>>>> + list_del_rcu(&rule_walker->list);
>
> So... rules don't take references on the landlock_objects they use?
> Instead, the landlock_object knows which rules use it, and when the
> landlock_object goes away, it nukes all the rules associated with
> itself?
Right.
>
> That seems terrible to me - AFAICS it means that if some random
> process decides to install a landlock rule that uses inode X, and then
> that process dies together with all its landlock rules, the inode
> still stays pinned in kernel memory as long as the superblock is
> mounted. In other words, it's a resource leak.
That is not correct. When there is no more process enforced by a
domain/ruleset, this domain is terminated, which means that every rules
linked to this domain are put away. When the usage counter of a rule
reaches zero, then the rule is terminated with landlock_put_rule() which
unlink the rule from its object and clean this object. The cleaning
involves to free the object if there is no rule tied to this object,
thanks to put_object_cleaner().
When the underlying object is terminated, landlock_release_object() also
decrement the usage counter. However, if there is a concurrent thread
adding a new rule, the usage counter still stay greater than zero while
the new rule is being added, but the counter then drops to zero at the
end of this addition, which can then unbalance the "cleaners" counter,
which will finally leads to the object freeing. This design enables to
add rules without locking (if the object already exists). While this
property is interesting for a performance point of view, the main reason
is to avoid unnecessary lock between processes (especially from
different domains).
> (And if I'm not missing
> something in patch 5, that applies even if the inode has been
> unlinked?)
That is true for now, but only because I didn't find yet the right spot
to call landlock_release_inode(). Indeed, unlinking a file may not
terminate an inode because it can still be open by a process, and
freeing an object when the underlying object is unlinked could be a way
to bypass a check on that object/inode.
Do you know where is the best spot to identify the last userspace
reference (through the filesystem or a file descriptor) to an inode?
Fnotify doesn't seem to check for that.
>
> Can you please refactor your refcounting as follows?
>
> - A rule takes a reference on each landlock_object it uses.
> - A landlock_object takes a reference on the underlying object (just like now).
> - The underlying object *DOES NOT* take a reference on the
> landlock_object (unlike now); the reference from the underlying object
> to the landlock_object has weak pointer semantics.
We need to increment the reference counter of the underlying objects
(i.e. inodes) not to lose the link with their Landlock object and then
the related access-control. For instance, if a struct inode (e.g. a
directory) is first tied to a Landlock object/access-control, then
because the inode is not open nor used by any process and the kernel
decides to free it, when a process tries to access a file beneath this
directory, there will not have any Landlock object tied to it and the
requested access might then be forbidden (whereas the initial policy
allowed it).
> - When a landlock_object's refcount drops to zero (iow no rules use
> it anymore), it is freed.
Before the current design, I used a similar pattern, but this is not
necessary because of the management of the underlying object lifetime.
The list_empty() check is enough, and because we need to handle
concurrent termination, the object's usage counter for the rules seems
unnecessary.
>
> That might also help get rid of the awkward ->cleaners thing?
>
>>>> + /*
>>>> + * Releases @object if it is not already released (e.g. with
>>>> + * landlock_release_object()).
>>>> + */
>>>> + release_object(object);
>>>> + /*
>>>> + * Unbalances the @object->cleaners counter to reflect the
>>>> + * underlying object release.
>>>> + */
>>>> + __acquire(object->cleaners);
>>>> + put_object_free(object);
>>>> + }
>>>> + put_object_cleaner(object_cleaner);
>>>> +}
> [...]
>>>> +static inline bool landlock_rule_is_disabled(
>>>> + struct landlock_rule *rule)
>>>> +{
>>>> + /*
>>>> + * Disabling (i.e. unlinking) a landlock_rule is a one-way operation.
>>>> + * It is not possible to re-enable such a rule, then there is no need
>>>> + * for smp_load_acquire().
>>>> + *
>>>> + * LIST_POISON2 is set by list_del() and list_del_rcu().
>>>> + */
>>>> + return !rule || READ_ONCE(rule->list.prev) == LIST_POISON2;
>>>
>>> You're not allowed to do this, the comment above list_del() states:
>>>
>>> * Note: list_empty() on entry does not return true after this, the entry is
>>> * in an undefined state.
>>
>> list_del() checks READ_ONCE(head->next) == head, but
>> landlock_rule_is_disabled() checks READ_ONCE(rule->list.prev) ==
>> LIST_POISON2.
>> The comment about LIST_POISON2 is right but may be misleading. There is
>> no use of list_empty() with a landlock_rule->list, only
>> landlock_object->rules. The only list_del() is in landlock_put_rule()
>> when there is a guarantee that there is no other reference to it, hence
>> no possible use of landlock_rule_is_disabled() with this rule. I could
>> replace it with a call to list_del_rcu() to make it more consistent.
>>
>>>
>>> If you want to be able to test whether the element is on a list
>>> afterwards, use stuff like list_del_init().
>>
>> There is no need to re-initialize the list but using list_del_init() and
>> list_empty() could work too. However, there is no list_del_init_rcu()
>> helper. Moreover, resetting the list's pointer with LIST_POISON2 might
>> help to detect bugs.
>
> Either way, you are currently using the list_head API in a way that
> goes against what the header documents. If you want to rely on
> list_del() bringing the object into a specific state, then you can't
> leave the comment above list_del() as-is that says that it puts the
> object in an undefined state; and this kind of check should probably
> be done in a helper in list.h instead of open-coding the check for
> LIST_POISON2.
In the case of Landlock, it is illegal to use or recycle a rule which
was untied from its (initial) object. There is no use of
list_empty(&landlock_rule->list), only
landlock_rule_is_disabled(landlock_rule). The LIST_POISON2 might help to
identify such misuse.
^ permalink raw reply
* Re: [RFC PATCH v14 01/10] landlock: Add object and rule management
From: Mickaël Salaün @ 2020-02-27 17:01 UTC (permalink / raw)
To: Hillf Danton
Cc: linux-kernel, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E. Hallyn, Shuah Khan,
Vincent Dagonneau, kernel-hardening, linux-api, linux-arch,
linux-doc, linux-fsdevel, linux-kselftest, linux-security-module,
x86
In-Reply-To: <20200227042002.3032-1-hdanton@sina.com>
On 27/02/2020 05:20, Hillf Danton wrote:
>
> On Mon, 24 Feb 2020 17:02:06 +0100 Mickaël Salaün
>> A Landlock object enables to identify a kernel object (e.g. an inode).
>> A Landlock rule is a set of access rights allowed on an object. Rules
>> are grouped in rulesets that may be tied to a set of processes (i.e.
>> subjects) to enforce a scoped access-control (i.e. a domain).
>>
>> Because Landlock's goal is to empower any process (especially
>> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
>> object identification such as file extended attributes. Indeed, we need
>> innocuous, composable and modular access-controls.
>>
>> The main challenge with this constraints is to identify kernel objects
>> while this identification is useful (i.e. when a security policy makes
>> use of this object). But this identification data should be freed once
>> no policy is using it. This ephemeral tagging should not and may not be
>> written in the filesystem. We then need to manage the lifetime of a
>> rule according to the lifetime of its object. To avoid a global lock,
>> this implementation make use of RCU and counters to safely reference
>> objects.
>>
>> A following commit uses this generic object management for inodes.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> ---
>>
>> Changes since v13:
>> * New dedicated implementation, removing the need for eBPF.
>>
>> Previous version:
>> https://lore.kernel.org/lkml/20190721213116.23476-6-mic@digikod.net/
>> ---
>> MAINTAINERS | 10 ++
>> security/Kconfig | 1 +
>> security/Makefile | 2 +
>> security/landlock/Kconfig | 15 ++
>> security/landlock/Makefile | 3 +
>> security/landlock/object.c | 339 +++++++++++++++++++++++++++++++++++++
>> security/landlock/object.h | 134 +++++++++++++++
>> 7 files changed, 504 insertions(+)
>> create mode 100644 security/landlock/Kconfig
>> create mode 100644 security/landlock/Makefile
>> create mode 100644 security/landlock/object.c
>> create mode 100644 security/landlock/object.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fcd79fc38928..206f85768cd9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9360,6 +9360,16 @@ F: net/core/skmsg.c
>> F: net/core/sock_map.c
>> F: net/ipv4/tcp_bpf.c
>>
>> +LANDLOCK SECURITY MODULE
>> +M: Mickaël Salaün <mic@digikod.net>
>> +L: linux-security-module@vger.kernel.org
>> +W: https://landlock.io
>> +T: git https://github.com/landlock-lsm/linux.git
>> +S: Supported
>> +F: security/landlock/
>> +K: landlock
>> +K: LANDLOCK
>> +
>> LANTIQ / INTEL Ethernet drivers
>> M: Hauke Mehrtens <hauke@hauke-m.de>
>> L: netdev@vger.kernel.org
>> diff --git a/security/Kconfig b/security/Kconfig
>> index 2a1a2d396228..9d9981394fb0 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -238,6 +238,7 @@ source "security/loadpin/Kconfig"
>> source "security/yama/Kconfig"
>> source "security/safesetid/Kconfig"
>> source "security/lockdown/Kconfig"
>> +source "security/landlock/Kconfig"
>>
>> source "security/integrity/Kconfig"
>>
>> diff --git a/security/Makefile b/security/Makefile
>> index 746438499029..2472ef96d40a 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA) += yama
>> subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin
>> subdir-$(CONFIG_SECURITY_SAFESETID) += safesetid
>> subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown
>> +subdir-$(CONFIG_SECURITY_LANDLOCK) += landlock
>>
>> # always enable default capabilities
>> obj-y += commoncap.o
>> @@ -29,6 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>> obj-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown/
>> +obj-$(CONFIG_SECURITY_LANDLOCK) += landlock/
>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>
>> # Object integrity file lists
>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>> new file mode 100644
>> index 000000000000..4a321d5b3f67
>> --- /dev/null
>> +++ b/security/landlock/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config SECURITY_LANDLOCK
>> + bool "Landlock support"
>> + depends on SECURITY
>> + default n
>> + help
>> + This selects Landlock, a safe sandboxing mechanism. It enables to
>> + restrict processes on the fly (i.e. enforce an access control policy),
>> + which can complement seccomp-bpf. The security policy is a set of access
>> + rights tied to an object, which could be a file, a socket or a process.
>> +
>> + See Documentation/security/landlock/ for further information.
>> +
>> + If you are unsure how to answer this question, answer N.
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> new file mode 100644
>> index 000000000000..cb6deefbf4c0
>> --- /dev/null
>> +++ b/security/landlock/Makefile
>> @@ -0,0 +1,3 @@
>> +obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>> +
>> +landlock-y := object.o
>> diff --git a/security/landlock/object.c b/security/landlock/object.c
>> new file mode 100644
>> index 000000000000..38fbbb108120
>> --- /dev/null
>> +++ b/security/landlock/object.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Object and rule management
>> + *
>> + * Copyright © 2016-2020 Mickaël Salaün <mic@digikod.net>
>> + * Copyright © 2018-2020 ANSSI
>> + *
>> + * Principles and constraints of the object and rule management:
>> + * - Do not leak memory.
>> + * - Try as much as possible to free a memory allocation as soon as it is
>> + * unused.
>> + * - Do not use global lock.
>> + * - Do not charge processes other than the one requesting a Landlock
>> + * operation.
>> + */
>> +
>> +#include <linux/bug.h>
>> +#include <linux/compiler.h>
>> +#include <linux/compiler_types.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/fs.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/rbtree.h>
>> +#include <linux/rcupdate.h>
>> +#include <linux/refcount.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "object.h"
>> +
>> +struct landlock_object *landlock_create_object(
>> + const enum landlock_object_type type, void *underlying_object)
>> +{
>> + struct landlock_object *object;
>> +
>> + if (WARN_ON_ONCE(!underlying_object))
>> + return NULL;
>> + object = kzalloc(sizeof(*object), GFP_KERNEL);
>> + if (!object)
>> + return NULL;
>> + refcount_set(&object->usage, 1);
>> + refcount_set(&object->cleaners, 1);
>> + spin_lock_init(&object->lock);
>> + INIT_LIST_HEAD(&object->rules);
>> + object->type = type;
>> + WRITE_ONCE(object->underlying_object, underlying_object);
>> + return object;
>> +}
>> +
>> +struct landlock_object *landlock_get_object(struct landlock_object *object)
>> + __acquires(object->usage)
>> +{
>> + __acquire(object->usage);
>> + /*
>> + * If @object->usage equal 0, then it will be ignored by writers, and
>> + * underlying_object->object may be replaced, but this is not an issue
>> + * for release_object().
>> + */
>> + if (object && refcount_inc_not_zero(&object->usage)) {
>> + /*
>> + * It should not be possible to get a reference to an object if
>> + * its underlying object is being terminated (e.g. with
>> + * landlock_release_object()), because an object is only
>> + * modifiable through such underlying object. This is not the
>> + * case with landlock_get_object_cleaner().
>> + */
>> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
>> + return object;
>> + }
>> + return NULL;
>> +}
>> +
>> +static struct landlock_object *get_object_cleaner(
>> + struct landlock_object *object)
>> + __acquires(object->cleaners)
>> +{
>> + __acquire(object->cleaners);
>> + if (object && refcount_inc_not_zero(&object->cleaners))
>> + return object;
>> + return NULL;
>> +}
>> +
>> +/*
>> + * There is two cases when an object should be free and the reference to the
>> + * underlying object should be put:
>> + * - when the last rule tied to this object is removed, which is handled by
>> + * landlock_put_rule() and then release_object();
>> + * - when the object is being terminated (e.g. no more reference to an inode),
>> + * which is handled by landlock_put_object().
>> + */
>> +static void put_object_free(struct landlock_object *object)
>> + __releases(object->cleaners)
>> +{
>> + __release(object->cleaners);
>> + if (!refcount_dec_and_test(&object->cleaners))
>> + return;
>> + WARN_ON_ONCE(refcount_read(&object->usage));
>> + /*
>> + * Ensures a safe use of @object in the RCU block from
>> + * landlock_put_rule().
>> + */
>> + kfree_rcu(object, rcu_free);
>> +}
>> +
>> +/*
>> + * Destroys a newly created and useless object.
>> + */
>> +void landlock_drop_object(struct landlock_object *object)
>> +{
>> + if (WARN_ON_ONCE(!refcount_dec_and_test(&object->usage)))
>> + return;
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> +}
>> +
>> +/*
>> + * Puts the underlying object (e.g. inode) if it is the first request to
>> + * release @object, without calling landlock_put_object().
>> + *
>> + * Return true if this call effectively marks @object as released, false
>> + * otherwise.
>> + */
>> +static bool release_object(struct landlock_object *object)
>> + __releases(&object->lock)
>> +{
>> + void *underlying_object;
>> +
>> + lockdep_assert_held(&object->lock);
>> +
>> + underlying_object = xchg(&object->underlying_object, NULL);
>
> A one-line comment looks needed for xchg.
Ok. This is to have a guarantee that the underlying_object (e.g. the
inode pointer) is only used once. I'll add a comment.
>
>> + spin_unlock(&object->lock);
>> + might_sleep();
>
> Have trouble working out what might_sleep is put for.
Patch 5 adds a call to landlock_release_inode(underlying_object, object)
(LANDLOCK_OBJECT_INODE case), which can sleep e.g., with a call to iput().
>
>> + if (!underlying_object)
>> + return false;
>> +
>> + switch (object->type) {
>> + case LANDLOCK_OBJECT_INODE:
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + }
>> + return true;
>> +}
>> +
>> +static void put_object_cleaner(struct landlock_object *object)
>> + __releases(object->cleaners)
>> +{
>> + /* Let's try an early lockless check. */
>> + if (list_empty(&object->rules) &&
>> + READ_ONCE(object->underlying_object)) {
>> + /*
>> + * Puts @object if there is no rule tied to it and the
>> + * remaining user is the underlying object. This check is
>> + * atomic because @object->rules and @object->underlying_object
>> + * are protected by @object->lock.
>> + */
>> + spin_lock(&object->lock);
>> + if (list_empty(&object->rules) &&
>> + READ_ONCE(object->underlying_object) &&
>> + refcount_dec_if_one(&object->usage)) {
>> + /*
>> + * Releases @object, in place of
>> + * landlock_release_object().
>> + *
>> + * @object is already empty, implying that all its
>> + * previous rules are already disabled.
>> + *
>> + * Unbalance the @object->cleaners counter to reflect
>> + * the underlying object release.
>> + */
>> + if (!WARN_ON_ONCE(!release_object(object))) {
>
> Two ! hurt more than help.
Well, it may not look nice but don't you think it is better than a
WARN_ON_ONCE(1) in the if block?
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>
> Why put object more than once?
I just replied to Jann about this subject. This is to "unbalance" the
counter to potentially free it (if there is no more user). I explain it
here:
https://lore.kernel.org/lkml/67465638-e22c-5d1a-df37-862b31d999a1@digikod.net/
>
>> + }
>> + } else {
>> + spin_unlock(&object->lock);
>> + }
>> + }
>> + put_object_free(object);
>> +}
>> +
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Dr. Greg @ 2020-02-27 18:40 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, 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, Florent Revest,
Brendan Jackman, Martin KaFai Lau, Song Liu, Yonghong Song,
Serge E. Hallyn, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <20200220175250.10795-1-kpsingh@chromium.org>
On Thu, Feb 20, 2020 at 06:52:42PM +0100, KP Singh wrote:
Good morning, I hope the week is going well for everyone.
Apologies for being somewhat late with these comments, I've been
recovering from travel.
> # Motivation
>
> Google does analysis of rich runtime security data to detect and thwart
> threats in real-time. Currently, this is done in custom kernel modules
> but we would like to replace this with something that's upstream and
> useful to others.
>
> The current kernel infrastructure for providing telemetry (Audit, Perf
> etc.) is disjoint from access enforcement (i.e. LSMs). Augmenting the
> information provided by audit requires kernel changes to audit, its
> policy language and user-space components. Furthermore, building a MAC
> policy based on the newly added telemetry data requires changes to
> various LSMs and their respective policy languages.
>
> This patchset allows BPF programs to be attached to LSM hooks This
> facilitates a unified and dynamic (not requiring re-compilation of the
> kernel) audit and MAC policy.
>
> # Why an LSM?
>
> Linux Security Modules target security behaviours rather than the
> kernel's API. For example, it's easy to miss out a newly added system
> call for executing processes (eg. execve, execveat etc.) but the LSM
> framework ensures that all process executions trigger the relevant hooks
> irrespective of how the process was executed.
>
> Allowing users to implement LSM hooks at runtime also benefits the LSM
> eco-system by enabling a quick feedback loop from the security community
> about the kind of behaviours that the LSM Framework should be targeting.
On the remote possibility that our practical experiences are relevant
to this, I thought I would pitch these comments in, since I see that
LWN is covering the issues and sensitivities surrounding BPF based
'intelligent' LSM hooks, if I can take the liberty of referring to
them as that.
We namespaced a modified version of the Linux IMA implementation in
order to provide a mechanism for deterministic system modeling, in
order to support autonomously self defensive platforms for
IOT/INED/SCADA type applications. Big picture, the objective was to
provide 'dynamic intelligence' for LSM decisions, presumably an
objective similar to the KRSI initiative.
Our IMA implementation, if you can still call it that, pushes
actor/subject interaction identities up into an SGX enclave that runs
a modeling engine that makes decisions on whether or not a process is
engaging in activity inconsistent with a behavioral map defined by the
platform or container developer. If the behavior is extra-dimensional
(untrusted), the enclave, via an OCALL, sets the value of a 'bad
actor' variable in the task control structure that is used to indicate
that the context of execution has questionable trust status.
We paired this with a very simple LSM that has each hook check a bit
position in the bad actor variable/bitfield to determine whether or
not the hook should operate on the requested action. Separate LSM
infrastructure is provided that specifies whether or not the behavior
should be EPERM'ed or logged. An LSM using this infrastructure also
has the ability, if triggered by the trust status of the context of
execution, to make further assessments based on what information is
supplied via the hook itself.
Our field experience and testing has suggested that this architecture
has considerable utility.
In this model, numerous and disparate sections of the kernel can have
input into the trust status of a context of execution. This
methodology would seem to be consistent with having multiple eBPF tap
points in the kernel that can make decisions on what they perceive to
be security relevant issues and if and how the behavior should be
acted upon by the LSM.
At the LSM level the costs are minimal, essentially a conditional
check for non-zero status. Performance costs will be with the eBPF
code installed at introspection points. At the end of the
day. security costs money, if no one is willing to pay the bill we
simply won't have secure systems, the fundamental tenant of the
inherent economic barrier to security.
Food for thought if anyone is interested.
Best wishes for a productive remainder of the week.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC SGX secured infrastructure and
4206 N. 19th Ave. autonomously self-defensive platforms.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"We have to grow some roots before we can even think about having
any blossoms."
-- Terrance George Wieland
Resurrection.
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Josef Bacik @ 2020-02-27 19:33 UTC (permalink / raw)
To: Christian Brauner, Stéphane Graber, Eric W. Biederman,
Aleksa Sarai, Jann Horn
Cc: smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api, mpawlowski
In-Reply-To: <20200218143411.2389182-1-christian.brauner@ubuntu.com>
On 2/18/20 9:33 AM, Christian Brauner wrote:
> Hey everyone,
>
> This is v3 after (off- and online) discussions with Jann the following
> changes were made:
> - To handle nested user namespaces cleanly, efficiently, and with full
> backwards compatibility for non fsid-mapping aware workloads we only
> allow writing fsid mappings as long as the corresponding id mapping
> type has not been written.
> - Split the patch which adds the internal ability in
> kernel/user_namespace to verify and write fsid mappings into tree
> patches:
> 1. [PATCH v3 04/25] fsuidgid: add fsid mapping helpers
> patch to implement core helpers for fsid translations (i.e.
> make_kfs*id(), from_kfs*id{_munged}(), kfs*id_to_k*id(),
> k*id_to_kfs*id()
> 2. [PATCH v3 05/25] user_namespace: refactor map_write()
> patch to refactor map_write() in order to prepare for actual fsid
> mappings changes in the following patch. (This should make it
> easier to review.)
> 3. [PATCH v3 06/25] user_namespace: make map_write() support fsid mappings
> patch to implement actual fsid mappings support in mape_write()
> - Let the keyctl infrastructure only operate on kfsid which are always
> mapped/looked up in the id mappings similar to what we do for
> filesystems that have the same superblock visible in multiple user
> namespaces.
>
> This version also comes with minimal tests which I intend to expand in
> the future.
>
> From pings and off-list questions and discussions at Google Container
> Security Summit there seems to be quite a lot of interest in this
> patchset with use-cases ranging from layer sharing for app containers
> and k8s, as well as data sharing between containers with different id
> mappings. I haven't Cced all people because I don't have all the email
> adresses at hand but I've at least added Phil now. :)
>
I put this into a kernel for our container guys to mess with in order to
validate it would actually be useful for real world uses. I've cc'ed the guy
who did all of the work in case you have specific questions.
Good news is the interface is acceptable, albeit apparently the whole user ns
interface sucks in general. But you haven't made it worse, so success!
But in testing it there appears to be a problem with tmpfs? Our applications
will use shared memory segments for certain things and it apparently breaks this
in interesting ways, it appears to not shift the UID appropriately on tmpfs.
This seems to be relatively straightforward to reproduce, but if you have
trouble let me know and I'll come up with a shell script that reproduces the
problem.
We are happy to continue testing these patches to make sure they're working in
our container setup, if you want to CC me on future submissions I can build them
for our internal testing and validate them as well. Thanks,
Josef
^ permalink raw reply
* Please revert SELinux/keys patches from the keys linux-next branch
From: Paul Moore @ 2020-02-28 0:19 UTC (permalink / raw)
To: dhowells; +Cc: selinux, linux-security-module
Hi David,
For some reason we haven't been able to get your attention on the
related SELinux mailing list threads, but we need you to revert commit
f981a85690dc ("security/selinux: Add support for new key permissions")
from your linux-next branch. Can you please do that?
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: David Hildenbrand @ 2020-02-28 9:19 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: linux-efi, Hans de Goede, Eric Richter, James Morris,
Michael Ellerman, Mimi Zohar, Nayna Jain, Serge E. Hallyn,
YueHaibing, linux-security-module
In-Reply-To: <20200217113947.2070436-1-javierm@redhat.com>
On 17.02.20 12:39, Javier Martinez Canillas wrote:
> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>
> But it just assumes that the variables will be present and prints an error
> if the certs can't be loaded, even when is possible that the variables may
> not exist. For example the MokListRT variable will only be present if shim
> is used.
>
> So only print an error message about failing to get the certs list from an
> EFI variable if this is found. Otherwise these printed errors just pollute
> the kernel log ring buffer with confusing messages like the following:
>
> [ 5.427251] Couldn't get size: 0x800000000000000e
> [ 5.427261] MODSIGN: Couldn't get UEFI db list
> [ 5.428012] Couldn't get size: 0x800000000000000e
> [ 5.428023] Couldn't get UEFI MokListRT
>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
This patch seems to break a very basic x86-64 QEMU setup (booting
upstream kernel with a F31 initrd - are you running basic boot tests?).
Luckily, it only took me 5 minutes to identify this patch. Reverting
this patch from linux-next fixes it for me.
[ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
[ 1.044314] zswap: loaded using pool lzo/zbud
[ 1.045663] Key type ._fscrypt registered
[ 1.046154] Key type .fscrypt registered
[ 1.046524] Key type fscrypt-provisioning registered
[ 1.051178] Key type big_key registered
[ 1.055108] Key type encrypted registered
[ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 1.056172] #PF: supervisor instruction fetch in kernel mode
[ 1.056706] #PF: error_code(0x0010) - not-present page
[ 1.057367] PGD 0 P4D 0
[ 1.057729] Oops: 0010 [#1] SMP NOPTI
[ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
[ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
[ 1.060230] RIP: 0010:0x0
[ 1.060478] Code: Bad RIP value.
[ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
[ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
[ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
[ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
[ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
[ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
[ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
[ 1.066562] Call Trace:
[ 1.066803] load_uefi_certs+0xc8/0x2bb
[ 1.067171] ? get_cert_list+0xfb/0xfb
[ 1.067523] do_one_initcall+0x5d/0x2f0
[ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
[ 1.068337] kernel_init_freeable+0x243/0x2c2
[ 1.068751] ? rest_init+0x23a/0x23a
[ 1.069095] kernel_init+0xa/0x106
[ 1.069416] ret_from_fork+0x27/0x50
[ 1.069759] Modules linked in:
[ 1.070050] CR2: 0000000000000000
[ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: David Hildenbrand @ 2020-02-28 9:31 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: linux-efi, Hans de Goede, Eric Richter, James Morris,
Michael Ellerman, Mimi Zohar, Nayna Jain, Serge E. Hallyn,
YueHaibing, linux-security-module, Ard Biesheuvel, Serge Hallyn
In-Reply-To: <0fd1b499-3a5e-c78e-0279-186a4c424217@redhat.com>
On 28.02.20 10:19, David Hildenbrand wrote:
> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>
>> But it just assumes that the variables will be present and prints an error
>> if the certs can't be loaded, even when is possible that the variables may
>> not exist. For example the MokListRT variable will only be present if shim
>> is used.
>>
>> So only print an error message about failing to get the certs list from an
>> EFI variable if this is found. Otherwise these printed errors just pollute
>> the kernel log ring buffer with confusing messages like the following:
>>
>> [ 5.427251] Couldn't get size: 0x800000000000000e
>> [ 5.427261] MODSIGN: Couldn't get UEFI db list
>> [ 5.428012] Couldn't get size: 0x800000000000000e
>> [ 5.428023] Couldn't get UEFI MokListRT
>>
>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>
> This patch seems to break a very basic x86-64 QEMU setup (booting
> upstream kernel with a F31 initrd - are you running basic boot tests?).
> Luckily, it only took me 5 minutes to identify this patch. Reverting
> this patch from linux-next fixes it for me.
>
>
> [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> [ 1.044314] zswap: loaded using pool lzo/zbud
> [ 1.045663] Key type ._fscrypt registered
> [ 1.046154] Key type .fscrypt registered
> [ 1.046524] Key type fscrypt-provisioning registered
> [ 1.051178] Key type big_key registered
> [ 1.055108] Key type encrypted registered
> [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [ 1.056172] #PF: supervisor instruction fetch in kernel mode
> [ 1.056706] #PF: error_code(0x0010) - not-present page
> [ 1.057367] PGD 0 P4D 0
> [ 1.057729] Oops: 0010 [#1] SMP NOPTI
> [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> [ 1.060230] RIP: 0010:0x0
> [ 1.060478] Code: Bad RIP value.
> [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> [ 1.066562] Call Trace:
> [ 1.066803] load_uefi_certs+0xc8/0x2bb
> [ 1.067171] ? get_cert_list+0xfb/0xfb
> [ 1.067523] do_one_initcall+0x5d/0x2f0
> [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
> [ 1.068337] kernel_init_freeable+0x243/0x2c2
> [ 1.068751] ? rest_init+0x23a/0x23a
> [ 1.069095] kernel_init+0xa/0x106
> [ 1.069416] ret_from_fork+0x27/0x50
> [ 1.069759] Modules linked in:
> [ 1.070050] CR2: 0000000000000000
> [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>
>
Sorry, wrong mail identified, the patch is actually
commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
Author: Ard Biesheuvel <ardb@kernel.org>
Date: Sun Feb 16 19:46:25 2020 +0100
integrity: Check properly whether EFI GetVariable() is available
Testing the value of the efi.get_variable function pointer is not
which made it work. (not even able to find that patch on lkml ...)
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: Ard Biesheuvel @ 2020-02-28 9:35 UTC (permalink / raw)
To: David Hildenbrand
Cc: Javier Martinez Canillas, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Eric Richter, James Morris, Michael Ellerman,
Mimi Zohar, Nayna Jain, Serge E. Hallyn, YueHaibing,
linux-security-module
In-Reply-To: <5c60e016-fb30-b33d-39c6-ea30a4f777cb@redhat.com>
On Fri, 28 Feb 2020 at 10:31, David Hildenbrand <david@redhat.com> wrote:
>
> On 28.02.20 10:19, David Hildenbrand wrote:
> > On 17.02.20 12:39, Javier Martinez Canillas wrote:
> >> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> >> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> >>
> >> But it just assumes that the variables will be present and prints an error
> >> if the certs can't be loaded, even when is possible that the variables may
> >> not exist. For example the MokListRT variable will only be present if shim
> >> is used.
> >>
> >> So only print an error message about failing to get the certs list from an
> >> EFI variable if this is found. Otherwise these printed errors just pollute
> >> the kernel log ring buffer with confusing messages like the following:
> >>
> >> [ 5.427251] Couldn't get size: 0x800000000000000e
> >> [ 5.427261] MODSIGN: Couldn't get UEFI db list
> >> [ 5.428012] Couldn't get size: 0x800000000000000e
> >> [ 5.428023] Couldn't get UEFI MokListRT
> >>
> >> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >
> > This patch seems to break a very basic x86-64 QEMU setup (booting
> > upstream kernel with a F31 initrd - are you running basic boot tests?).
> > Luckily, it only took me 5 minutes to identify this patch. Reverting
> > this patch from linux-next fixes it for me.
> >
> >
> > [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> > [ 1.044314] zswap: loaded using pool lzo/zbud
> > [ 1.045663] Key type ._fscrypt registered
> > [ 1.046154] Key type .fscrypt registered
> > [ 1.046524] Key type fscrypt-provisioning registered
> > [ 1.051178] Key type big_key registered
> > [ 1.055108] Key type encrypted registered
> > [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [ 1.056172] #PF: supervisor instruction fetch in kernel mode
> > [ 1.056706] #PF: error_code(0x0010) - not-present page
> > [ 1.057367] PGD 0 P4D 0
> > [ 1.057729] Oops: 0010 [#1] SMP NOPTI
> > [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> > [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> > [ 1.060230] RIP: 0010:0x0
> > [ 1.060478] Code: Bad RIP value.
> > [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> > [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> > [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> > [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> > [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> > [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> > [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> > [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> > [ 1.066562] Call Trace:
> > [ 1.066803] load_uefi_certs+0xc8/0x2bb
> > [ 1.067171] ? get_cert_list+0xfb/0xfb
> > [ 1.067523] do_one_initcall+0x5d/0x2f0
> > [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
> > [ 1.068337] kernel_init_freeable+0x243/0x2c2
> > [ 1.068751] ? rest_init+0x23a/0x23a
> > [ 1.069095] kernel_init+0xa/0x106
> > [ 1.069416] ret_from_fork+0x27/0x50
> > [ 1.069759] Modules linked in:
> > [ 1.070050] CR2: 0000000000000000
> > [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> >
> >
>
> Sorry, wrong mail identified, the patch is actually
>
> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> Author: Ard Biesheuvel <ardb@kernel.org>
> Date: Sun Feb 16 19:46:25 2020 +0100
>
> integrity: Check properly whether EFI GetVariable() is available
>
> Testing the value of the efi.get_variable function pointer is not
>
> which made it work. (not even able to find that patch on lkml ...)
>
https://lore.kernel.org/linux-efi/20200219171907.11894-10-ardb@kernel.org/T/#u
Interesting. So reverting that patch makes things work again?
Could you share your QEMU command line? I assume the bug is caused by
the fact that efi.get_variable == NULL
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: David Hildenbrand @ 2020-02-28 9:35 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: linux-efi, Hans de Goede, Eric Richter, James Morris,
Michael Ellerman, Mimi Zohar, Nayna Jain, Serge E. Hallyn,
YueHaibing, linux-security-module, Ard Biesheuvel
In-Reply-To: <5c60e016-fb30-b33d-39c6-ea30a4f777cb@redhat.com>
On 28.02.20 10:31, David Hildenbrand wrote:
> On 28.02.20 10:19, David Hildenbrand wrote:
>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>>
>>> But it just assumes that the variables will be present and prints an error
>>> if the certs can't be loaded, even when is possible that the variables may
>>> not exist. For example the MokListRT variable will only be present if shim
>>> is used.
>>>
>>> So only print an error message about failing to get the certs list from an
>>> EFI variable if this is found. Otherwise these printed errors just pollute
>>> the kernel log ring buffer with confusing messages like the following:
>>>
>>> [ 5.427251] Couldn't get size: 0x800000000000000e
>>> [ 5.427261] MODSIGN: Couldn't get UEFI db list
>>> [ 5.428012] Couldn't get size: 0x800000000000000e
>>> [ 5.428023] Couldn't get UEFI MokListRT
>>>
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> This patch seems to break a very basic x86-64 QEMU setup (booting
>> upstream kernel with a F31 initrd - are you running basic boot tests?).
>> Luckily, it only took me 5 minutes to identify this patch. Reverting
>> this patch from linux-next fixes it for me.
>>
>>
>> [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
>> [ 1.044314] zswap: loaded using pool lzo/zbud
>> [ 1.045663] Key type ._fscrypt registered
>> [ 1.046154] Key type .fscrypt registered
>> [ 1.046524] Key type fscrypt-provisioning registered
>> [ 1.051178] Key type big_key registered
>> [ 1.055108] Key type encrypted registered
>> [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> [ 1.056172] #PF: supervisor instruction fetch in kernel mode
>> [ 1.056706] #PF: error_code(0x0010) - not-present page
>> [ 1.057367] PGD 0 P4D 0
>> [ 1.057729] Oops: 0010 [#1] SMP NOPTI
>> [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
>> [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
>> [ 1.060230] RIP: 0010:0x0
>> [ 1.060478] Code: Bad RIP value.
>> [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
>> [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
>> [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
>> [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
>> [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
>> [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
>> [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
>> [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
>> [ 1.066562] Call Trace:
>> [ 1.066803] load_uefi_certs+0xc8/0x2bb
>> [ 1.067171] ? get_cert_list+0xfb/0xfb
>> [ 1.067523] do_one_initcall+0x5d/0x2f0
>> [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
>> [ 1.068337] kernel_init_freeable+0x243/0x2c2
>> [ 1.068751] ? rest_init+0x23a/0x23a
>> [ 1.069095] kernel_init+0xa/0x106
>> [ 1.069416] ret_from_fork+0x27/0x50
>> [ 1.069759] Modules linked in:
>> [ 1.070050] CR2: 0000000000000000
>> [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>>
>>
>
> Sorry, wrong mail identified, the patch is actually
>
> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> Author: Ard Biesheuvel <ardb@kernel.org>
> Date: Sun Feb 16 19:46:25 2020 +0100
>
> integrity: Check properly whether EFI GetVariable() is available
>
> Testing the value of the efi.get_variable function pointer is not
>
> which made it work. (not even able to find that patch on lkml ...)
To clarify for Ard, your patch breaks a basic QEMU setup (see above,
NULL pointer dereference). Reverting your patch from linux-next makes it
work again.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: Ard Biesheuvel @ 2020-02-28 9:39 UTC (permalink / raw)
To: David Hildenbrand
Cc: Javier Martinez Canillas, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Eric Richter, James Morris, Michael Ellerman,
Mimi Zohar, Nayna Jain, Serge E. Hallyn, YueHaibing,
linux-security-module
In-Reply-To: <30819cad-1a00-1ea7-13cf-d1d15c0fa96c@redhat.com>
On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
>
> On 28.02.20 10:31, David Hildenbrand wrote:
> > On 28.02.20 10:19, David Hildenbrand wrote:
> >> On 17.02.20 12:39, Javier Martinez Canillas wrote:
> >>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> >>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> >>>
> >>> But it just assumes that the variables will be present and prints an error
> >>> if the certs can't be loaded, even when is possible that the variables may
> >>> not exist. For example the MokListRT variable will only be present if shim
> >>> is used.
> >>>
> >>> So only print an error message about failing to get the certs list from an
> >>> EFI variable if this is found. Otherwise these printed errors just pollute
> >>> the kernel log ring buffer with confusing messages like the following:
> >>>
> >>> [ 5.427251] Couldn't get size: 0x800000000000000e
> >>> [ 5.427261] MODSIGN: Couldn't get UEFI db list
> >>> [ 5.428012] Couldn't get size: 0x800000000000000e
> >>> [ 5.428023] Couldn't get UEFI MokListRT
> >>>
> >>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >>
> >> This patch seems to break a very basic x86-64 QEMU setup (booting
> >> upstream kernel with a F31 initrd - are you running basic boot tests?).
> >> Luckily, it only took me 5 minutes to identify this patch. Reverting
> >> this patch from linux-next fixes it for me.
> >>
> >>
> >> [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> >> [ 1.044314] zswap: loaded using pool lzo/zbud
> >> [ 1.045663] Key type ._fscrypt registered
> >> [ 1.046154] Key type .fscrypt registered
> >> [ 1.046524] Key type fscrypt-provisioning registered
> >> [ 1.051178] Key type big_key registered
> >> [ 1.055108] Key type encrypted registered
> >> [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> >> [ 1.056172] #PF: supervisor instruction fetch in kernel mode
> >> [ 1.056706] #PF: error_code(0x0010) - not-present page
> >> [ 1.057367] PGD 0 P4D 0
> >> [ 1.057729] Oops: 0010 [#1] SMP NOPTI
> >> [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> >> [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> >> [ 1.060230] RIP: 0010:0x0
> >> [ 1.060478] Code: Bad RIP value.
> >> [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> >> [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> >> [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> >> [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> >> [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> >> [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> >> [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> >> [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> >> [ 1.066562] Call Trace:
> >> [ 1.066803] load_uefi_certs+0xc8/0x2bb
> >> [ 1.067171] ? get_cert_list+0xfb/0xfb
> >> [ 1.067523] do_one_initcall+0x5d/0x2f0
> >> [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
> >> [ 1.068337] kernel_init_freeable+0x243/0x2c2
> >> [ 1.068751] ? rest_init+0x23a/0x23a
> >> [ 1.069095] kernel_init+0xa/0x106
> >> [ 1.069416] ret_from_fork+0x27/0x50
> >> [ 1.069759] Modules linked in:
> >> [ 1.070050] CR2: 0000000000000000
> >> [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> >>
> >>
> >
> > Sorry, wrong mail identified, the patch is actually
> >
> > commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> > Author: Ard Biesheuvel <ardb@kernel.org>
> > Date: Sun Feb 16 19:46:25 2020 +0100
> >
> > integrity: Check properly whether EFI GetVariable() is available
> >
> > Testing the value of the efi.get_variable function pointer is not
> >
> > which made it work. (not even able to find that patch on lkml ...)
>
> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
> NULL pointer dereference). Reverting your patch from linux-next makes it
> work again.
>
Does this fix it?
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 41269a95ff85..d1746a579c99 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
{
int error;
- if (!efi_enabled(EFI_BOOT))
- return 0;
-
if (!efi_enabled(EFI_RUNTIME_SERVICES))
efi.runtime_supported_mask = 0;
+ if (!efi_enabled(EFI_BOOT))
+ return 0;
+
if (efi.runtime_supported_mask) {
/*
* Since we process only one efi_runtime_service() at a time, an
^ permalink raw reply related
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: David Hildenbrand @ 2020-02-28 9:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Javier Martinez Canillas, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Eric Richter, James Morris, Michael Ellerman,
Mimi Zohar, Nayna Jain, Serge E. Hallyn, YueHaibing,
linux-security-module
In-Reply-To: <CAKv+Gu869JovSC9AdCC1yvcKF_Qpa0U3dr_kF2_e3zZMaDDs+Q@mail.gmail.com>
On 28.02.20 10:39, Ard Biesheuvel wrote:
> On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 28.02.20 10:31, David Hildenbrand wrote:
>>> On 28.02.20 10:19, David Hildenbrand wrote:
>>>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>>>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>>>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>>>>
>>>>> But it just assumes that the variables will be present and prints an error
>>>>> if the certs can't be loaded, even when is possible that the variables may
>>>>> not exist. For example the MokListRT variable will only be present if shim
>>>>> is used.
>>>>>
>>>>> So only print an error message about failing to get the certs list from an
>>>>> EFI variable if this is found. Otherwise these printed errors just pollute
>>>>> the kernel log ring buffer with confusing messages like the following:
>>>>>
>>>>> [ 5.427251] Couldn't get size: 0x800000000000000e
>>>>> [ 5.427261] MODSIGN: Couldn't get UEFI db list
>>>>> [ 5.428012] Couldn't get size: 0x800000000000000e
>>>>> [ 5.428023] Couldn't get UEFI MokListRT
>>>>>
>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> This patch seems to break a very basic x86-64 QEMU setup (booting
>>>> upstream kernel with a F31 initrd - are you running basic boot tests?).
>>>> Luckily, it only took me 5 minutes to identify this patch. Reverting
>>>> this patch from linux-next fixes it for me.
>>>>
>>>>
>>>> [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
>>>> [ 1.044314] zswap: loaded using pool lzo/zbud
>>>> [ 1.045663] Key type ._fscrypt registered
>>>> [ 1.046154] Key type .fscrypt registered
>>>> [ 1.046524] Key type fscrypt-provisioning registered
>>>> [ 1.051178] Key type big_key registered
>>>> [ 1.055108] Key type encrypted registered
>>>> [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>> [ 1.056172] #PF: supervisor instruction fetch in kernel mode
>>>> [ 1.056706] #PF: error_code(0x0010) - not-present page
>>>> [ 1.057367] PGD 0 P4D 0
>>>> [ 1.057729] Oops: 0010 [#1] SMP NOPTI
>>>> [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
>>>> [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
>>>> [ 1.060230] RIP: 0010:0x0
>>>> [ 1.060478] Code: Bad RIP value.
>>>> [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
>>>> [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
>>>> [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
>>>> [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
>>>> [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
>>>> [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
>>>> [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
>>>> [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
>>>> [ 1.066562] Call Trace:
>>>> [ 1.066803] load_uefi_certs+0xc8/0x2bb
>>>> [ 1.067171] ? get_cert_list+0xfb/0xfb
>>>> [ 1.067523] do_one_initcall+0x5d/0x2f0
>>>> [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
>>>> [ 1.068337] kernel_init_freeable+0x243/0x2c2
>>>> [ 1.068751] ? rest_init+0x23a/0x23a
>>>> [ 1.069095] kernel_init+0xa/0x106
>>>> [ 1.069416] ret_from_fork+0x27/0x50
>>>> [ 1.069759] Modules linked in:
>>>> [ 1.070050] CR2: 0000000000000000
>>>> [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>>>>
>>>>
>>>
>>> Sorry, wrong mail identified, the patch is actually
>>>
>>> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
>>> Author: Ard Biesheuvel <ardb@kernel.org>
>>> Date: Sun Feb 16 19:46:25 2020 +0100
>>>
>>> integrity: Check properly whether EFI GetVariable() is available
>>>
>>> Testing the value of the efi.get_variable function pointer is not
>>>
>>> which made it work. (not even able to find that patch on lkml ...)
>>
>> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
>> NULL pointer dereference). Reverting your patch from linux-next makes it
>> work again.
>>
>
> Does this fix it?
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 41269a95ff85..d1746a579c99 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
> {
> int error;
>
> - if (!efi_enabled(EFI_BOOT))
> - return 0;
> -
> if (!efi_enabled(EFI_RUNTIME_SERVICES))
> efi.runtime_supported_mask = 0;
>
> + if (!efi_enabled(EFI_BOOT))
> + return 0;
> +
> if (efi.runtime_supported_mask) {
> /*
> * Since we process only one efi_runtime_service() at a time, an
>
Yes, does the trick!
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: Ard Biesheuvel @ 2020-02-28 9:44 UTC (permalink / raw)
To: David Hildenbrand
Cc: Javier Martinez Canillas, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Eric Richter, James Morris, Michael Ellerman,
Mimi Zohar, Nayna Jain, Serge E. Hallyn, YueHaibing,
linux-security-module
In-Reply-To: <0d8e7525-545d-23d5-dace-6804833447d2@redhat.com>
On Fri, 28 Feb 2020 at 10:43, David Hildenbrand <david@redhat.com> wrote:
>
> On 28.02.20 10:39, Ard Biesheuvel wrote:
> > On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 28.02.20 10:31, David Hildenbrand wrote:
> >>> On 28.02.20 10:19, David Hildenbrand wrote:
> >>>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
> >>>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> >>>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> >>>>>
> >>>>> But it just assumes that the variables will be present and prints an error
> >>>>> if the certs can't be loaded, even when is possible that the variables may
> >>>>> not exist. For example the MokListRT variable will only be present if shim
> >>>>> is used.
> >>>>>
> >>>>> So only print an error message about failing to get the certs list from an
> >>>>> EFI variable if this is found. Otherwise these printed errors just pollute
> >>>>> the kernel log ring buffer with confusing messages like the following:
> >>>>>
> >>>>> [ 5.427251] Couldn't get size: 0x800000000000000e
> >>>>> [ 5.427261] MODSIGN: Couldn't get UEFI db list
> >>>>> [ 5.428012] Couldn't get size: 0x800000000000000e
> >>>>> [ 5.428023] Couldn't get UEFI MokListRT
> >>>>>
> >>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >>>>
> >>>> This patch seems to break a very basic x86-64 QEMU setup (booting
> >>>> upstream kernel with a F31 initrd - are you running basic boot tests?).
> >>>> Luckily, it only took me 5 minutes to identify this patch. Reverting
> >>>> this patch from linux-next fixes it for me.
> >>>>
> >>>>
> >>>> [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> >>>> [ 1.044314] zswap: loaded using pool lzo/zbud
> >>>> [ 1.045663] Key type ._fscrypt registered
> >>>> [ 1.046154] Key type .fscrypt registered
> >>>> [ 1.046524] Key type fscrypt-provisioning registered
> >>>> [ 1.051178] Key type big_key registered
> >>>> [ 1.055108] Key type encrypted registered
> >>>> [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> >>>> [ 1.056172] #PF: supervisor instruction fetch in kernel mode
> >>>> [ 1.056706] #PF: error_code(0x0010) - not-present page
> >>>> [ 1.057367] PGD 0 P4D 0
> >>>> [ 1.057729] Oops: 0010 [#1] SMP NOPTI
> >>>> [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> >>>> [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> >>>> [ 1.060230] RIP: 0010:0x0
> >>>> [ 1.060478] Code: Bad RIP value.
> >>>> [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> >>>> [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> >>>> [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> >>>> [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> >>>> [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> >>>> [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> >>>> [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> >>>> [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>> [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> >>>> [ 1.066562] Call Trace:
> >>>> [ 1.066803] load_uefi_certs+0xc8/0x2bb
> >>>> [ 1.067171] ? get_cert_list+0xfb/0xfb
> >>>> [ 1.067523] do_one_initcall+0x5d/0x2f0
> >>>> [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
> >>>> [ 1.068337] kernel_init_freeable+0x243/0x2c2
> >>>> [ 1.068751] ? rest_init+0x23a/0x23a
> >>>> [ 1.069095] kernel_init+0xa/0x106
> >>>> [ 1.069416] ret_from_fork+0x27/0x50
> >>>> [ 1.069759] Modules linked in:
> >>>> [ 1.070050] CR2: 0000000000000000
> >>>> [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> >>>>
> >>>>
> >>>
> >>> Sorry, wrong mail identified, the patch is actually
> >>>
> >>> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> >>> Author: Ard Biesheuvel <ardb@kernel.org>
> >>> Date: Sun Feb 16 19:46:25 2020 +0100
> >>>
> >>> integrity: Check properly whether EFI GetVariable() is available
> >>>
> >>> Testing the value of the efi.get_variable function pointer is not
> >>>
> >>> which made it work. (not even able to find that patch on lkml ...)
> >>
> >> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
> >> NULL pointer dereference). Reverting your patch from linux-next makes it
> >> work again.
> >>
> >
> > Does this fix it?
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 41269a95ff85..d1746a579c99 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
> > {
> > int error;
> >
> > - if (!efi_enabled(EFI_BOOT))
> > - return 0;
> > -
> > if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > efi.runtime_supported_mask = 0;
> >
> > + if (!efi_enabled(EFI_BOOT))
> > + return 0;
> > +
> > if (efi.runtime_supported_mask) {
> > /*
> > * Since we process only one efi_runtime_service() at a time, an
> >
>
> Yes, does the trick!
>
Thanks, David. I'll get this out and into -next asap.
^ permalink raw reply
* Re: [RESEND PATCH v2] efi: Only print errors about failing to get certs if EFI vars are found
From: David Hildenbrand @ 2020-02-28 9:58 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Javier Martinez Canillas, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Eric Richter, James Morris, Michael Ellerman,
Mimi Zohar, Nayna Jain, Serge E. Hallyn, YueHaibing,
linux-security-module
In-Reply-To: <CAKv+Gu-OO_YiDWbF2pRBQTGe3FxC5CmjuxCnCwxJmnzRHhEz_g@mail.gmail.com>
On 28.02.20 10:44, Ard Biesheuvel wrote:
> On Fri, 28 Feb 2020 at 10:43, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 28.02.20 10:39, Ard Biesheuvel wrote:
>>> On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 28.02.20 10:31, David Hildenbrand wrote:
>>>>> On 28.02.20 10:19, David Hildenbrand wrote:
>>>>>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>>>>>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>>>>>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>>>>>>
>>>>>>> But it just assumes that the variables will be present and prints an error
>>>>>>> if the certs can't be loaded, even when is possible that the variables may
>>>>>>> not exist. For example the MokListRT variable will only be present if shim
>>>>>>> is used.
>>>>>>>
>>>>>>> So only print an error message about failing to get the certs list from an
>>>>>>> EFI variable if this is found. Otherwise these printed errors just pollute
>>>>>>> the kernel log ring buffer with confusing messages like the following:
>>>>>>>
>>>>>>> [ 5.427251] Couldn't get size: 0x800000000000000e
>>>>>>> [ 5.427261] MODSIGN: Couldn't get UEFI db list
>>>>>>> [ 5.428012] Couldn't get size: 0x800000000000000e
>>>>>>> [ 5.428023] Couldn't get UEFI MokListRT
>>>>>>>
>>>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>
>>>>>> This patch seems to break a very basic x86-64 QEMU setup (booting
>>>>>> upstream kernel with a F31 initrd - are you running basic boot tests?).
>>>>>> Luckily, it only took me 5 minutes to identify this patch. Reverting
>>>>>> this patch from linux-next fixes it for me.
>>>>>>
>>>>>>
>>>>>> [ 1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
>>>>>> [ 1.044314] zswap: loaded using pool lzo/zbud
>>>>>> [ 1.045663] Key type ._fscrypt registered
>>>>>> [ 1.046154] Key type .fscrypt registered
>>>>>> [ 1.046524] Key type fscrypt-provisioning registered
>>>>>> [ 1.051178] Key type big_key registered
>>>>>> [ 1.055108] Key type encrypted registered
>>>>>> [ 1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>>>> [ 1.056172] #PF: supervisor instruction fetch in kernel mode
>>>>>> [ 1.056706] #PF: error_code(0x0010) - not-present page
>>>>>> [ 1.057367] PGD 0 P4D 0
>>>>>> [ 1.057729] Oops: 0010 [#1] SMP NOPTI
>>>>>> [ 1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
>>>>>> [ 1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
>>>>>> [ 1.060230] RIP: 0010:0x0
>>>>>> [ 1.060478] Code: Bad RIP value.
>>>>>> [ 1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
>>>>>> [ 1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
>>>>>> [ 1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
>>>>>> [ 1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
>>>>>> [ 1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
>>>>>> [ 1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
>>>>>> [ 1.064609] FS: 0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
>>>>>> [ 1.065360] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> [ 1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
>>>>>> [ 1.066562] Call Trace:
>>>>>> [ 1.066803] load_uefi_certs+0xc8/0x2bb
>>>>>> [ 1.067171] ? get_cert_list+0xfb/0xfb
>>>>>> [ 1.067523] do_one_initcall+0x5d/0x2f0
>>>>>> [ 1.067894] ? rcu_read_lock_sched_held+0x52/0x80
>>>>>> [ 1.068337] kernel_init_freeable+0x243/0x2c2
>>>>>> [ 1.068751] ? rest_init+0x23a/0x23a
>>>>>> [ 1.069095] kernel_init+0xa/0x106
>>>>>> [ 1.069416] ret_from_fork+0x27/0x50
>>>>>> [ 1.069759] Modules linked in:
>>>>>> [ 1.070050] CR2: 0000000000000000
>>>>>> [ 1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>>>>>>
>>>>>>
>>>>>
>>>>> Sorry, wrong mail identified, the patch is actually
>>>>>
>>>>> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
>>>>> Author: Ard Biesheuvel <ardb@kernel.org>
>>>>> Date: Sun Feb 16 19:46:25 2020 +0100
>>>>>
>>>>> integrity: Check properly whether EFI GetVariable() is available
>>>>>
>>>>> Testing the value of the efi.get_variable function pointer is not
>>>>>
>>>>> which made it work. (not even able to find that patch on lkml ...)
>>>>
>>>> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
>>>> NULL pointer dereference). Reverting your patch from linux-next makes it
>>>> work again.
>>>>
>>>
>>> Does this fix it?
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index 41269a95ff85..d1746a579c99 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
>>> {
>>> int error;
>>>
>>> - if (!efi_enabled(EFI_BOOT))
>>> - return 0;
>>> -
>>> if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>> efi.runtime_supported_mask = 0;
>>>
>>> + if (!efi_enabled(EFI_BOOT))
>>> + return 0;
>>> +
>>> if (efi.runtime_supported_mask) {
>>> /*
>>> * Since we process only one efi_runtime_service() at a time, an
>>>
>>
>> Yes, does the trick!
>>
>
> Thanks, David. I'll get this out and into -next asap.
>
Thanks for the very fast fix :)
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: Please revert SELinux/keys patches from the keys linux-next branch
From: David Howells @ 2020-02-28 15:37 UTC (permalink / raw)
To: Paul Moore; +Cc: dhowells, selinux, linux-security-module
In-Reply-To: <CAHC9VhQ=W4R2LGCxaKzVEx4J31m4-F7mDo2BOMTqso2JdScHzw@mail.gmail.com>
Paul Moore <paul@paul-moore.com> wrote:
> For some reason we haven't been able to get your attention on the
> related SELinux mailing list threads, but we need you to revert commit
> f981a85690dc ("security/selinux: Add support for new key permissions")
> from your linux-next branch. Can you please do that?
Sorry, I had to squeeze out a new version of notifications and fsinfo before
disappearing off to Vault - and then I disappeared off to Vault. However, I
can do that now.
I can drop keyring ACL patches for the moment and pick them back up after the
next merge window.
David
^ 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