Re: [PATCH v24 01/12] landlock: Add object management

2020-11-21 Thread Mickaël Salaün


On 21/11/2020 08:00, Jann Horn wrote:
> On Thu, Nov 12, 2020 at 9:51 PM Mickaël Salaün  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 
>> Cc: Kees Cook 
>> Cc: Serge E. Hallyn 
>> Signed-off-by: Mickaël Salaün 
>> Reviewed-by: Jann Horn 
> 
> Still looks good, except for one comment:
> 
> [...]
>> +   /**
>> +* @lock: Guards against concurrent modifications.  This lock might 
>> be
>> +* held 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;
> 
> Why did you change this to "might be held" (v22 had "must")? Is the
> "might" a typo?
> 

Good catch, a typo indeed.


Re: [PATCH v24 01/12] landlock: Add object management

2020-11-20 Thread Jann Horn
On Thu, Nov 12, 2020 at 9:51 PM Mickaël Salaün  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 
> Cc: Kees Cook 
> Cc: Serge E. Hallyn 
> Signed-off-by: Mickaël Salaün 
> Reviewed-by: Jann Horn 

Still looks good, except for one comment:

[...]
> +   /**
> +* @lock: Guards against concurrent modifications.  This lock might be
> +* held 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;

Why did you change this to "might be held" (v22 had "must")? Is the
"might" a typo?


[PATCH v24 01/12] landlock: Add object management

2020-11-12 Thread Mickaël Salaün
From: 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 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 
Cc: Kees Cook 
Cc: Serge E. Hallyn 
Signed-off-by: Mickaël Salaün 
Reviewed-by: Jann Horn 
---

Changes since v23:
* Update landlock_create_object() to return error codes instead of NULL.
  This help error handling in callers.
* When using make oldconfig with a previous configuration already
  including the CONFIG_LSM variable, no question is asked to update its
  content.  Update the Kconfig help to warn about LSM stacking
  configuration.
* Constify variable (spotted by Vincent Dagonneau).

Changes since v22:
* Fix spelling (spotted by Jann Horn).

Changes since v21:
* Update Kconfig help.
* Clean up comments.

Changes since v18:
* Account objects to kmemcg.

Changes since v14:
* Simplify the object, rule and ruleset management at the expense of a
  less aggressive memory freeing (contributed by Jann Horn, with
  additional modifications):
  - Remove object->list aggregating the rules tied to an object.
  - Remove landlock_get_object(), landlock_drop_object(),
{get,put}_object_cleaner() and landlock_rule_is_disabled().
  - Rewrite landlock_put_object() to use a more simple mechanism
(no tricky RCU).
  - Replace enum landlock_object_type and landlock_release_object() with
landlock_object_underops->release()
  - Adjust unions and Sparse annotations.
  Cf. 
https://lore.kernel.org/lkml/cag48ez21ben0wl1bbmtiiu8j9jp5iewthowz4turuj+ki0y...@mail.gmail.com/
* Merge struct landlock_rule into landlock_ruleset_elem to simplify the
  rule management.
* Constify variables.
* Improve kernel documentation.
* Cosmetic variable renames.
* Remove the "default" in the Kconfig (suggested by Jann Horn).
* Only use refcount_inc() through getter helpers.
* Update Kconfig description.

Changes since v13:
* New dedicated implementation, removing the need for eBPF.

Previous changes:
https://lore.kernel.org/lkml/20190721213116.23476-6-...@digikod.net/
---
 MAINTAINERS| 10 +
 security/Kconfig   |  1 +
 security/Makefile  |  2 +
 security/landlock/Kconfig  | 21 +
 security/landlock/Makefile |  3 ++
 security/landlock/object.c | 67 
 security/landlock/object.h | 91 ++
 7 files changed, 195 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 3da6d8c154e4..33482cbe56e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9826,6 +9826,16 @@ F:   net/core/sock_map.c
 F: net/ipv4/tcp_bpf.c
 F: net/ipv4/udp_bpf.c
 
+LANDLOCK SECURITY MODULE
+M: Mickaël Salaün 
+L: linux-security-mod...@vger.kernel.org
+S: Supported
+W: https://landlock.io
+T: git https://github.com/landlock-lsm/linux.git
+F: security/landlock/
+K: landlock
+K: LANDLOCK
+
 LANTIQ / INTEL Ethernet drivers
 M: Hauke Mehrtens 
 L: net...@vger.kernel.org
diff --git a/security/Kconfig b/security/Kconfig
index 7561f6f99f1d..15a4342b5d01 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 3baf435de541..c688f4907a1b 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -13,6 +13,7 @@ subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin
 subdir-$(CONFIG_SECURITY_SAFESETID)+= safesetid
 subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown
 subdir-$(CONFIG_BPF_LSM)   += bpf
+subdir-$(CONFIG_SECURITY_LANDLOCK) += landlock
 
 # always enable default capabilities
 obj-y