Thanks for your patch, but this is not a review of my patch.
On 24/09/2022 15:29, Bagas Sanjaya wrote:
On Fri, Sep 23, 2022 at 05:42:06PM +0200, Mickaël Salaün wrote:
Now that we have more than one ABI version, make limitation explanation
more consistent by replacing "ABI 1" with "ABI < 2". This also
indicates which ABIs support such past limitation.
Hi,
I think grammar and reference link on userspace documentation can be
improved, like:
---- >8 ----
diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
index 2cc502a75ef640..c49454900edb12 100644
--- a/Documentation/admin-guide/cgroup-v1/memory.rst
+++ b/Documentation/admin-guide/cgroup-v1/memory.rst
@@ -1,3 +1,5 @@
+.. _cgroup-v1-memory:
+
==========================
Memory Resource Controller
==========================
diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 4c76fda076454c..dbeddb6851e6c9 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -3,6 +3,8 @@
Written by: Neil Brown
Please see MAINTAINERS file for where to send questions.
+.. _overlayfs:
+
Overlay Filesystem
==================
diff --git a/Documentation/filesystems/sharedsubtree.rst b/Documentation/filesystems/sharedsubtree.rst
index d83395354250d9..cbc20658bd0c07 100644
--- a/Documentation/filesystems/sharedsubtree.rst
+++ b/Documentation/filesystems/sharedsubtree.rst
@@ -1,5 +1,7 @@
.. SPDX-License-Identifier: GPL-2.0
+.. _shared-subtrees:
+
These modifications are unrelated to Landlock.
===============
Shared Subtrees
===============
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 5dbd577b5f58b7..13a8db59b0c0e5 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -14,16 +14,18 @@ The goal of Landlock is to enable to restrict ambient rights (e.g. global
filesystem access) for a set of processes. Because Landlock is a stackable
LSM, it makes possible to create safe security sandboxes as new security layers
in addition to the existing system-wide access-controls. This kind of sandbox
-is expected to help mitigate the security impact of bugs or
-unexpected/malicious behaviors in user space applications. Landlock empowers
-any process, including unprivileged ones, to securely restrict themselves.
+can help mitigating the security impact of bugs or unexpected/malicious
+behaviors in user space applications. Landlock empowers any process, including
+unprivileged ones, to securely restrict themselves.
-We can quickly make sure that Landlock is enabled in the running system by
-looking for ``landlock: Up and running`` in kernel logs (as root): ``dmesg |
-grep landlock || journalctl -kg landlock`` . Developers can also easily check
-for Landlock support with a :ref:`related system call <landlock_abi_versions>`.
-If Landlock is not currently supported, we need to :ref:`configure the kernel
-appropriately <kernel_support>`.
+To check whether Landlock has been successfully enabled, look for
+``landlock: Up and running`` in kernel logs, which can be seen with (as root)::
+
+ dmesg | grep landlock || journalctl -kg landlock
+
+Developers can also easily check for Landlock support with a
+:ref:`related system call <landlock_abi_versions>`. If Landlock is not
+currently supported, :ref:`the kernel needs to be configured <kernel_support>`.
This looks good, but it is difficult to see your changes because you
re-wrapped the lines.
Landlock rules
==============
@@ -36,12 +38,12 @@ the thread enforcing it, and its future children.
Defining and enforcing a security policy
----------------------------------------
-We first need to define the ruleset that will contain our rules. For this
-example, the ruleset will contain rules that only allow read actions, but write
-actions will be denied. The ruleset then needs to handle both of these kind of
+First, the ruleset that will contain rules needs to be defined. For this
+example, the ruleset will contain rules that only allow read actions (write
+actions will be denied). The ruleset then needs to handle both of these kind of
Why such changes?
actions. This is required for backward and forward compatibility (i.e. the
kernel and user space may not know each other's supported restrictions), hence
-the need to be explicit about the denied-by-default access rights.
+it is required to be explicit about the denied-by-default access rights.
.. code-block:: c
@@ -63,14 +65,14 @@ the need to be explicit about the denied-by-default access rights.
LANDLOCK_ACCESS_FS_REFER,
};
-Because we may not know on which kernel version an application will be
-executed, it is safer to follow a best-effort security approach. Indeed, we
-should try to protect users as much as possible whatever the kernel they are
-using. To avoid binary enforcement (i.e. either all security features or
-none), we can leverage a dedicated Landlock command to get the current version
-of the Landlock ABI and adapt the handled accesses. Let's check if we should
-remove the ``LANDLOCK_ACCESS_FS_REFER`` access right which is only supported
-starting with the second version of the ABI.
+Because it is unknown which kernel version an application will be executed,
+it is safer to follow a best-effort security approach. Indeed, users need to
+be protected as much as possible whatever the kernel they are
+using. To avoid atomic enforcement situations (i.e. either all security
+features or none), a dedicated Landlock command to get the current version
+of the Landlock ABI can be used and adapt the handled accesses. Let's check if
+``LANDLOCK_ACCESS_FS_REFER`` access right (which is available since second
+ABI version) can be removed:
Again, difficult to review with all this line changes.
.. code-block:: c
@@ -81,7 +83,7 @@ starting with the second version of the ABI.
ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
}
-This enables to create an inclusive ruleset that will contain our rules.
+This enables to create an inclusive ruleset that will contain the rules.
Can you explain your motivation?
.. code-block:: c
@@ -93,12 +95,11 @@ This enables to create an inclusive ruleset that will contain our rules.
return 1;
}
-We can now add a new rule to this ruleset thanks to the returned file
+Now a new rule to this ruleset can be added using the returned file
descriptor referring to this ruleset. The rule will only allow reading the
-file hierarchy ``/usr``. Without another rule, write actions would then be
-denied by the ruleset. To add ``/usr`` to the ruleset, we open it with the
-``O_PATH`` flag and fill the &struct landlock_path_beneath_attr with this file
-descriptor.
+file hierarchy ``/usr``. To add ``/usr`` to the ruleset, open the ruleset
+with the ``O_PATH`` flag and fill the struct landlock_path_beneath_attr with
+the descriptor:
.. code-block:: c
@@ -125,15 +126,15 @@ descriptor.
return 1;
}
-It may also be required to create rules following the same logic as explained
-for the ruleset creation, by filtering access rights according to the Landlock
-ABI version. In this example, this is not required because
-``LANDLOCK_ACCESS_FS_REFER`` is not allowed by any rule.
+It may also be required to add rules following the same logic as explained
+above, to filter access rights according to the Landlock ABI version.
+In this example, this is not required because ``LANDLOCK_ACCESS_FS_REFER``
+is not allowed by any rule.
-We now have a ruleset with one rule allowing read access to ``/usr`` while
+Now there is a ruleset with one rule allowing read access to ``/usr`` while
denying all other handled accesses for the filesystem. The next step is to
-restrict the current thread from gaining more privileges (e.g. thanks to a SUID
-binary).
+restrict the current thread from gaining more privileges (for example due
+to be run by setuid binary):
.. code-block:: c
@@ -158,7 +159,7 @@ If the ``landlock_restrict_self`` system call succeeds, the current thread is
now restricted and this policy will be enforced on all its subsequently created
children as well. Once a thread is landlocked, there is no way to remove its
security policy; only adding more restrictions is allowed. These threads are
-now in a new Landlock domain, merge of their parent one (if any) with the new
+now in a new Landlock domain, merging their parent one (if any) with the new
"merging their parent one"?
ruleset.
Full working code can be found in `samples/landlock/sandboxer.c`_.
@@ -166,24 +167,24 @@ Full working code can be found in `samples/landlock/sandboxer.c`_.
Good practices
--------------
-It is recommended setting access rights to file hierarchy leaves as much as
-possible. For instance, it is better to be able to have ``~/doc/`` as a
-read-only hierarchy and ``~/tmp/`` as a read-write hierarchy, compared to
-``~/`` as a read-only hierarchy and ``~/tmp/`` as a read-write hierarchy.
-Following this good practice leads to self-sufficient hierarchies that do not
-depend on their location (i.e. parent directories). This is particularly
-relevant when we want to allow linking or renaming. Indeed, having consistent
-access rights per directory enables to change the location of such directory
-without relying on the destination directory access rights (except those that
-are required for this operation, see ``LANDLOCK_ACCESS_FS_REFER``
-documentation).
+It is recommended to set access rights to file hierarchy leaves as much as
+possible. For instance, it is better to have a read-only ``~/doc/`` and
+read-write ``~/tmp/`` hierarchies, compared to read-only ``~/`` and
+read-write ``~/tmp/`` hierarchies. Following it leads to self-sufficient
+hierarchies that do not depend on their location (i.e. parent directories).
+This is particularly relevant when allowing linking or renaming is needed.
+Indeed, having consistent access rights per directory enables to change the
+location of such directory without relying on the destination directory access
+rights (except those that are required for this operation, see
+``LANDLOCK_ACCESS_FS_REFER`` documentation for example).
+
Having self-sufficient hierarchies also helps to tighten the required access
rights to the minimal set of data. This also helps avoid sinkhole directories,
-i.e. directories where data can be linked to but not linked from. However,
-this depends on data organization, which might not be controlled by developers.
-In this case, granting read-write access to ``~/tmp/``, instead of write-only
-access, would potentially allow to move ``~/tmp/`` to a non-readable directory
-and still keep the ability to list the content of ``~/tmp/``.
+i.e. directories where data can be linked to but not from. However,
+this depends on data organization and layout, which may be outside developer
+control. In this case, granting read-write access instead of write-only to
+``~/tmp/`` would potentially allow to move ``~/tmp/`` to a non-readable
+directory and still keep the ability to list the content of ``~/tmp/``.
Layers of file path access rights
---------------------------------
@@ -194,7 +195,7 @@ the potentially other rulesets already restricting this thread. A sandboxed
thread can then safely add more constraints to itself with a new enforced
ruleset.
-One policy layer grants access to a file path if at least one of its rules
+A policy layer grants access to a file path if at least one of its rules
encountered on the path grants the access. A sandboxed thread can only access
a file path if all its enforced policy layers grant the access as well as all
the other system access controls (e.g. filesystem DAC, other LSM policies,
@@ -203,42 +204,40 @@ etc.).
Bind mounts and OverlayFS
-------------------------
-Landlock enables to restrict access to file hierarchies, which means that these
-access rights can be propagated with bind mounts (cf.
-Documentation/filesystems/sharedsubtree.rst) but not with
-Documentation/filesystems/overlayfs.rst.
+Landlock can restrict access to file hierarchies, which means that these
+access rights can be propagated with bind mounts (see
+:ref:`shared-subtrees`) but not with :ref:`OverlayFS <overlayfs>`.
A bind mount mirrors a source file hierarchy to a destination. The destination
hierarchy is then composed of the exact same files, on which Landlock rules can
-be tied, either via the source or the destination path. These rules restrict
-access when they are encountered on a path, which means that they can restrict
-access to multiple file hierarchies at the same time, whether these hierarchies
+be applied, either via the source or the destination path. These rules
+restrict access when they are encountered on a path, which means that access
+to multiple file hierarchies can be restricted, whether these hierarchies
are the result of bind mounts or not.
-An OverlayFS mount point consists of upper and lower layers. These layers are
-combined in a merge directory, result of the mount point. This merge hierarchy
-may include files from the upper and lower layers, but modifications performed
-on the merge hierarchy only reflects on the upper layer. From a Landlock
-policy point of view, each OverlayFS layers and merge hierarchies are
-standalone and contains their own set of files and directories, which is
-different from bind mounts. A policy restricting an OverlayFS layer will not
-restrict the resulted merged hierarchy, and vice versa. Landlock users should
-then only think about file hierarchies they want to allow access to, regardless
-of the underlying filesystem.
+On the other hand, an OverlayFS mount point consists of upper and lower layers.
+These layers are combined in a merge directory; result of the mount point.
+This merge hierarchy may include files from the upper and lower layers, but
+modifications performed on the merge hierarchy only reflects on the upper
+layer. From a Landlock policy point of view, each OverlayFS layers and merge
+hierarchies are standalone and contains their own set of files and directories,
+which are different from bind mounts. A policy restricting an OverlayFS layer
+will not restrict the resulted merged hierarchy, and vice versa. Landlock users
+should then only think about file hierarchies they want to allow access to,
+regardless of the underlying filesystem.
Inheritance
-----------
Every new thread resulting from a :manpage:`clone(2)` inherits Landlock domain
-restrictions from its parent. This is similar to the seccomp inheritance (cf.
-Documentation/userspace-api/seccomp_filter.rst) or any other LSM dealing with
-task's :manpage:`credentials(7)`. For instance, one process's thread may apply
-Landlock rules to itself, but they will not be automatically applied to other
-sibling threads (unlike POSIX thread credential changes, cf.
-:manpage:`nptl(7)`).
+restrictions from its parent. This is similar to the :ref:`seccomp-bpf`) or
+any other LSM dealing with task :manpage:`credentials(7)`. For instance, a
+process thread may apply Landlock rules to itself, but they will not be
+automatically applied to other sibling threads (unlike POSIX thread credential
+changes, see :manpage:`nptl(7)`).
-When a thread sandboxes itself, we have the guarantee that the related security
-policy will stay enforced on all this thread's descendants. This allows
+When a thread sandboxes itself, there is a guarantee that the related security
+policy will stay enforced on all the thread descendants. This allows
creating standalone and modular security policies per application, which will
automatically be composed between themselves according to their runtime parent
policies.
@@ -259,18 +258,19 @@ Backward and forward compatibility
----------------------------------
Landlock is designed to be compatible with past and future versions of the
-kernel. This is achieved thanks to the system call attributes and the
-associated bitflags, particularly the ruleset's ``handled_access_fs``. Making
-handled access right explicit enables the kernel and user space to have a clear
-contract with each other. This is required to make sure sandboxing will not
-get stricter with a system update, which could break applications.
+kernel. This is achieved bysystem call attributes and the associated bitflags,
+particularly the ``handled_access_fs`` of ruleset. Making access rights
+handling explicit enables the kernel and user space to have a clear contract
+with each other. This is required to make sure sandboxing will not get
+stricter with a system update, which could break applications.
Developers can subscribe to the `Landlock mailing list
-<https://subspace.kernel.org/lists.linux.dev.html>`_ to knowingly update and
-test their applications with the latest available features. In the interest of
-users, and because they may use different kernel versions, it is strongly
-encouraged to follow a best-effort security approach by checking the Landlock
-ABI version at runtime and only enforcing the supported features.
+<https://subspace.kernel.org/lists.linux.dev.html>`_ to get to know with
+Landlock updates and test their applications with bleeding-edge features.
+In the interest of users, and because they may use different kernel versions,
+it is strongly encouraged to follow a best-effort security approach by
+checking the Landlock ABI version at runtime and only enforcing the supported
+features.
.. _landlock_abi_versions:
@@ -345,7 +345,7 @@ Filesystem topology modification
As for file renaming and linking, a sandboxed thread cannot modify its
filesystem topology, whether via :manpage:`mount(2)` or
-:manpage:`pivot_root(2)`. However, :manpage:`chroot(2)` calls are not denied.
+:manpage:`pivot_root(2)`. However, :manpage:`chroot(2)` calls are allowed.
Special filesystems
-------------------
@@ -353,13 +353,11 @@ Special filesystems
Access to regular files and directories can be restricted by Landlock,
according to the handled accesses of a ruleset. However, files that do not
come from a user-visible filesystem (e.g. pipe, socket), but can still be
-accessed through ``/proc/<pid>/fd/*``, cannot currently be explicitly
-restricted. Likewise, some special kernel filesystems such as nsfs, which can
-be accessed through ``/proc/<pid>/ns/*``, cannot currently be explicitly
-restricted. However, thanks to the `ptrace restrictions`_, access to such
-sensitive ``/proc`` files are automatically restricted according to domain
-hierarchies. Future Landlock evolutions could still enable to explicitly
-restrict such paths with dedicated ruleset flags.
+accessed through ``/proc/<pid>/fd/*``, cannot currently be restricted,
+and so some special kernel filesystems such as nsfs which can
+be accessed through ``/proc/<pid>/ns/*``. However, access to such
+sensitive ``/proc`` files are automatically restricted due to `ptrace restrictions`_ according to domain hierarchies. Future Landlock evolutions could still
+enable to explicitly restrict such paths with dedicated ruleset flags.
Ruleset layers
--------------
@@ -376,7 +374,7 @@ Memory usage
------------
Kernel memory allocated to create rulesets is accounted and can be restricted
-by the Documentation/admin-guide/cgroup-v1/memory.rst.
+(see :ref:`cgroup-v1-memory`).
Previous limitations
====================
@@ -386,7 +384,7 @@ File renaming and linking (ABI < 2)
Because Landlock targets unprivileged access controls, it needs to properly
handle composition of rules. Such property also implies rules nesting.
-Properly handling multiple layers of rulesets, each one of them able to
+Properly handling multiple layers of rulesets, with each one of them able to
restrict access to files, also implies inheritance of the ruleset restrictions
from a parent to its hierarchy. Because files are identified and restricted by
their hierarchy, moving or linking a file from one directory to another implies
@@ -395,26 +393,24 @@ according to the potentially lost constraints. To protect against privilege
escalations through renaming or linking, and for the sake of simplicity,
Landlock previously limited linking and renaming to the same directory.
Starting with the Landlock ABI version 2, it is now possible to securely
-control renaming and linking thanks to the new ``LANDLOCK_ACCESS_FS_REFER``
-access right.
+control renaming and linking by ``LANDLOCK_ACCESS_FS_REFER``.
.. _kernel_support:
Kernel support
==============
-Landlock was first introduced in Linux 5.13 but it must be configured at build
-time with ``CONFIG_SECURITY_LANDLOCK=y``. Landlock must also be enabled at boot
-time as the other security modules. The list of security modules enabled by
-default is set with ``CONFIG_LSM``. The kernel configuration should then
-contains ``CONFIG_LSM=landlock,[...]`` with ``[...]`` as the list of other
+Landlock was first introduced in Linux 5.13. To enable the support, the kernel
+must be configured at build time with ``CONFIG_SECURITY_LANDLOCK=y``.
+Landlock must also be enabled at boot time like other security modules.
+The list of security modules enabled by default can be seen with
+``CONFIG_LSM``. The kernel configuration should then contains
+``CONFIG_LSM=landlock,[...]`` with ``[...]`` as the list of other
potentially useful security modules for the running system (see the
``CONFIG_LSM`` help).
-If the running kernel does not have ``landlock`` in ``CONFIG_LSM``, then we can
-still enable it by adding ``lsm=landlock,[...]`` to
-Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader
-configuration.
+If the running kernel does not have ``landlock`` in ``CONFIG_LSM``, then it can still be enabled by adding ``lsm=landlock,[...]`` kernel parameter (see
+:ref:`kernelparameters`)
Questions and answers
=====================
@@ -433,7 +429,7 @@ What about namespaces and containers?
Namespaces can help create sandboxes but they are not designed for
access-control and then miss useful features for such use case (e.g. no
fine-grained restrictions). Moreover, their complexity can lead to security
-issues, especially when untrusted processes can manipulate them (cf.
+issues, especially when untrusted processes can manipulate them (see
`Controlling access to user namespaces <https://lwn.net/Articles/673597/>`_).
Additional documentation
This is too difficult to review and there is no explanation justifying
them. Anyway, it is unrelated to my patch. You can send a standalone
patch on top of this series, but I need more explanation.
diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index d1e2b9193f0984..a12782a958d43c 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -1,3 +1,5 @@
+.. _seccomp-bpf:
+
===========================================
Seccomp BPF (SECure COMPuting with filters)
===========================================
Thanks.
Not related to Landlock.
