|
Message-ID: <afa8e978-d22c-f06a-d57b-e0d1a9918062@digikod.net> Date: Thu, 29 Oct 2020 10:30:04 +0100 From: Mickaël Salaün <mic@...ikod.net> To: Jann Horn <jannh@...gle.com> Cc: James Morris <jmorris@...ei.org>, "Serge E . Hallyn" <serge@...lyn.com>, Al Viro <viro@...iv.linux.org.uk>, Andy Lutomirski <luto@...capital.net>, Anton Ivanov <anton.ivanov@...bridgegreys.com>, Arnd Bergmann <arnd@...db.de>, Casey Schaufler <casey@...aufler-ca.com>, Jeff Dike <jdike@...toit.com>, Jonathan Corbet <corbet@....net>, Kees Cook <keescook@...omium.org>, Michael Kerrisk <mtk.manpages@...il.com>, Richard Weinberger <richard@....at>, Shuah Khan <shuah@...nel.org>, Vincent Dagonneau <vincent.dagonneau@....gouv.fr>, Kernel Hardening <kernel-hardening@...ts.openwall.com>, Linux API <linux-api@...r.kernel.org>, linux-arch <linux-arch@...r.kernel.org>, "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>, linux-fsdevel <linux-fsdevel@...r.kernel.org>, kernel list <linux-kernel@...r.kernel.org>, "open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>, linux-security-module <linux-security-module@...r.kernel.org>, the arch/x86 maintainers <x86@...nel.org>, Mickaël Salaün <mic@...ux.microsoft.com> Subject: Re: [PATCH v22 01/12] landlock: Add object management On 29/10/2020 02:05, Jann Horn wrote: > On Tue, Oct 27, 2020 at 9:04 PM Mickaël Salaün <mic@...ikod.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 cannot 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 these 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 objects. 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. >> >> Cc: James Morris <jmorris@...ei.org> >> Cc: Jann Horn <jannh@...gle.com> >> Cc: Kees Cook <keescook@...omium.org> >> Cc: Serge E. Hallyn <serge@...lyn.com> >> Signed-off-by: Mickaël Salaün <mic@...ux.microsoft.com> > > Reviewed-by: Jann Horn <jannh@...gle.com> Thanks for the review. > > except for some minor nits: > > [...] >> diff --git a/security/landlock/object.c b/security/landlock/object.c > [...] >> +void landlock_put_object(struct landlock_object *const object) >> +{ >> + /* >> + * The call to @object->underops->release(object) might sleep e.g., > > s/ e.g.,/, e.g./ I indeed prefer the comma preceding the "e.g.", but it seems that there is a difference between UK english and US english: https://english.stackexchange.com/questions/16172/should-i-always-use-a-comma-after-e-g-or-i-e Looking at the kernel documentation makes it clear: $ git grep -F 'e.g. ' | wc -l 1179 $ git grep -F 'e.g., ' | wc -l 160 I'll apply your fix in the whole patch series. > >> + * because of iput(). >> + */ >> + might_sleep(); >> + if (!object) >> + return; > [...] >> +} >> diff --git a/security/landlock/object.h b/security/landlock/object.h > [...] >> +struct landlock_object { >> + /** >> + * @usage: This counter is used to tie an object to the rules matching >> + * it or to keep it alive while adding a new rule. If this counter >> + * reaches zero, this struct must not be modified, but this counter can >> + * still be read from within an RCU read-side critical section. When >> + * adding a new rule to an object with a usage counter of zero, we must >> + * wait until the pointer to this object is set to NULL (or recycled). >> + */ >> + refcount_t usage; >> + /** >> + * @lock: Guards against concurrent modifications. This lock must be > > s/must be/must be held/ ? Right. > >> + * from the time @usage drops to zero until any weak references from >> + * @underobj to this object have been cleaned up. >> + * >> + * Lock ordering: inode->i_lock nests inside this. >> + */ >> + spinlock_t lock; > [...] >> +}; >> + >> +struct landlock_object *landlock_create_object( >> + const struct landlock_object_underops *const underops, >> + void *const underojb); > > nit: "underobj" > Good catch!
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.