Linux-Hardening mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] landlock: Extend documentation for kernel support
@ 2024-02-27 11:05 Mickaël Salaün
  2024-02-27 11:05 ` [PATCH v2 2/2] landlock: Warn once if a Landlock action is requested while disabled Mickaël Salaün
  2024-02-27 16:32 ` [PATCH v2 1/2] landlock: Extend documentation for kernel support Günther Noack
  0 siblings, 2 replies; 7+ messages in thread
From: Mickaël Salaün @ 2024-02-27 11:05 UTC (permalink / raw
  To: Günther Noack, Paul Moore, Serge E . Hallyn
  Cc: Mickaël Salaün, Konstantin Meskhidze, Shervin Oloumi,
	linux-hardening, linux-kernel, linux-security-module, Kees Cook

Extend the kernel support section with one subsection for build time
configuration and another for boot time configuration.

Extend the boot time subsection with a concrete example.

Update the journalctl command to include the boot option.

Cc: Günther Noack <gnoack@google.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

Changes since v1:
* New patch, suggested by Kees Cook.
---
 Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 2e3822677061..838cc27db232 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -19,11 +19,12 @@ 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>`.
+looking for "landlock: Up and running" in kernel logs (as root):
+``dmesg | grep landlock || journalctl -kb -g 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>`.
 
 Landlock rules
 ==============
@@ -499,6 +500,9 @@ access rights.
 Kernel support
 ==============
 
+Build time configuration
+------------------------
+
 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
@@ -507,11 +511,52 @@ contains ``CONFIG_LSM=landlock,[...]`` with ``[...]``  as the list of other
 potentially useful security modules for the running system (see the
 ``CONFIG_LSM`` help).
 
+Boot time configuration
+-----------------------
+
 If the running kernel does not have ``landlock`` in ``CONFIG_LSM``, then we can
-still enable it by adding ``lsm=landlock,[...]`` to
+enable Landlock by adding ``lsm=landlock,[...]`` to
 Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader
 configuration.
 
+For example, if the current built-in configuration is:
+
+.. code-block:: console
+
+    $ zgrep -h "^CONFIG_LSM=" "/boot/config-$(uname -r)" /proc/config.gz 2>/dev/null
+    CONFIG_LSM="lockdown,yama,integrity,apparmor"
+
+...and if the cmdline doesn't contain ``landlock`` either:
+
+.. code-block:: console
+
+    $ sed -n 's/.*\(\<lsm=\S\+\).*/\1/p' /proc/cmdline
+    lsm=lockdown,yama,integrity,apparmor
+
+...we should configure the bootloader to set a cmdline extending the ``lsm``
+list with the ``landlock,`` prefix::
+
+  lsm=landlock,lockdown,yama,integrity,apparmor
+
+After a reboot, we can check that Landlock is up and running by looking at
+kernel logs:
+
+.. code-block:: console
+
+    # dmesg | grep landlock || journalctl -kb -g landlock
+    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
+    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
+    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
+    [    0.000000] landlock: Up and running.
+
+Note that according to the built time kernel configuration,
+``lockdown,capability,`` may always stay at the beginning of the ``LSM:
+initializing lsm=`` list even if they are not configured with the bootloader,
+which is OK.
+
+Network support
+---------------
+
 To be able to explicitly allow TCP operations (e.g., adding a network rule with
 ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
 (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an

base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] landlock: Warn once if a Landlock action is requested while disabled
  2024-02-27 11:05 [PATCH v2 1/2] landlock: Extend documentation for kernel support Mickaël Salaün
@ 2024-02-27 11:05 ` Mickaël Salaün
  2024-02-27 16:32 ` [PATCH v2 1/2] landlock: Extend documentation for kernel support Günther Noack
  1 sibling, 0 replies; 7+ messages in thread
From: Mickaël Salaün @ 2024-02-27 11:05 UTC (permalink / raw
  To: Günther Noack, Paul Moore, Serge E . Hallyn
  Cc: Mickaël Salaün, Konstantin Meskhidze, Shervin Oloumi,
	linux-hardening, linux-kernel, linux-security-module, stable,
	Kees Cook, Günther Noack

Because sandboxing can be used as an opportunistic security measure,
user space may not log unsupported features.  Let the system
administrator know if an application tries to use Landlock but failed
because it isn't enabled at boot time.  This may be caused by bootloader
configurations with outdated "lsm" kernel's command-line parameter.

Cc: stable@vger.kernel.org
Fixes: 265885daf3e5 ("landlock: Add syscall implementations")
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Günther Noack <gnoack3000@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

Changes since v1:
* Add Kees's and Günther's Reviewed-by.
* Rename is_not_initialized() to not_initialized() and invert the logic,
  as suggested by Günther.  This is a cosmetic change without global
  behavioral changed.
* Update link to point to a new subsection.
---
 security/landlock/syscalls.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 898358f57fa0..6788e73b6681 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -33,6 +33,18 @@
 #include "ruleset.h"
 #include "setup.h"
 
+static bool is_initialized(void)
+{
+	if (likely(landlock_initialized))
+		return true;
+
+	pr_warn_once(
+		"Disabled but requested by user space. "
+		"You should enable Landlock at boot time: "
+		"https://docs.kernel.org/userspace-api/landlock.html#boot-time-configuration\n");
+	return false;
+}
+
 /**
  * copy_min_struct_from_user - Safe future-proof argument copying
  *
@@ -173,7 +185,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	/* Build-time checks. */
 	build_check_abi();
 
-	if (!landlock_initialized)
+	if (!is_initialized())
 		return -EOPNOTSUPP;
 
 	if (flags) {
@@ -398,7 +410,7 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
 	struct landlock_ruleset *ruleset;
 	int err;
 
-	if (!landlock_initialized)
+	if (!is_initialized())
 		return -EOPNOTSUPP;
 
 	/* No flag for now. */
@@ -458,7 +470,7 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
 	struct landlock_cred_security *new_llcred;
 	int err;
 
-	if (!landlock_initialized)
+	if (!is_initialized())
 		return -EOPNOTSUPP;
 
 	/*
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support
  2024-02-27 11:05 [PATCH v2 1/2] landlock: Extend documentation for kernel support Mickaël Salaün
  2024-02-27 11:05 ` [PATCH v2 2/2] landlock: Warn once if a Landlock action is requested while disabled Mickaël Salaün
@ 2024-02-27 16:32 ` Günther Noack
  2024-03-07 10:21   ` Mickaël Salaün
  1 sibling, 1 reply; 7+ messages in thread
From: Günther Noack @ 2024-02-27 16:32 UTC (permalink / raw
  To: Mickaël Salaün
  Cc: Paul Moore, Serge E . Hallyn, Konstantin Meskhidze,
	Shervin Oloumi, linux-hardening, linux-kernel,
	linux-security-module, Kees Cook

On Tue, Feb 27, 2024 at 12:05:49PM +0100, Mickaël Salaün wrote:
> Extend the kernel support section with one subsection for build time
> configuration and another for boot time configuration.
> 
> Extend the boot time subsection with a concrete example.
> 
> Update the journalctl command to include the boot option.
> 
> Cc: Günther Noack <gnoack@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> Changes since v1:
> * New patch, suggested by Kees Cook.
> ---
>  Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 2e3822677061..838cc27db232 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> +Boot time configuration
> +-----------------------
> +
>  If the running kernel does not have ``landlock`` in ``CONFIG_LSM``, then we can
> -still enable it by adding ``lsm=landlock,[...]`` to
> +enable Landlock by adding ``lsm=landlock,[...]`` to
>  Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader
>  configuration.

I would suggest: s/thanks to/in/

> +For example, if the current built-in configuration is:
> +
> +.. code-block:: console
> +
> +    $ zgrep -h "^CONFIG_LSM=" "/boot/config-$(uname -r)" /proc/config.gz 2>/dev/null
> +    CONFIG_LSM="lockdown,yama,integrity,apparmor"
> +
> +...and if the cmdline doesn't contain ``landlock`` either:
> +
> +.. code-block:: console
> +
> +    $ sed -n 's/.*\(\<lsm=\S\+\).*/\1/p' /proc/cmdline
> +    lsm=lockdown,yama,integrity,apparmor
> +
> +...we should configure the bootloader to set a cmdline extending the ``lsm``
> +list with the ``landlock,`` prefix::

Nit: Is the double colon at the end of this line accidental?
(It does not appear before the previous code block.)

> +
> +  lsm=landlock,lockdown,yama,integrity,apparmor
> +
> +After a reboot, we can check that Landlock is up and running by looking at
> +kernel logs:
> +
> +.. code-block:: console
> +
> +    # dmesg | grep landlock || journalctl -kb -g landlock
> +    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> +    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> +    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
> +    [    0.000000] landlock: Up and running.
> +
> +Note that according to the built time kernel configuration,

s/built time/build time/
                 ^

It feels like the phrase "according to" could be slightly more specific here.

To paraphrase Alejandro Colomar, "Note that" is usually redundant.
https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/

I'd suggest:

  The kernel may be configured at build time to always load the ``lockdown`` and
  ``capability`` LSMs.  In that case, these LSMs will appear at the beginning of
  the ``LSM: initializing`` log line as well, even if they are not configured in
  the boot loader.

> +``lockdown,capability,`` may always stay at the beginning of the ``LSM:
> +initializing lsm=`` list even if they are not configured with the bootloader,

Nit: The man pages spell this in two words as "boot loader".


> +which is OK.
> +
> +Network support
> +---------------
> +
>  To be able to explicitly allow TCP operations (e.g., adding a network rule with
>  ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
>  (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an
> 
> base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
> -- 
> 2.44.0
> 

Reviewed-by: Günther Noack <gnoack@google.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support
  2024-02-27 16:32 ` [PATCH v2 1/2] landlock: Extend documentation for kernel support Günther Noack
@ 2024-03-07 10:21   ` Mickaël Salaün
  2024-03-18  9:50     ` Alejandro Colomar
  0 siblings, 1 reply; 7+ messages in thread
From: Mickaël Salaün @ 2024-03-07 10:21 UTC (permalink / raw
  To: Günther Noack, Alejandro Colomar
  Cc: Paul Moore, Serge E . Hallyn, Konstantin Meskhidze,
	Shervin Oloumi, linux-hardening, linux-kernel,
	linux-security-module, Kees Cook

CCing Alejandro

On Tue, Feb 27, 2024 at 05:32:20PM +0100, Günther Noack wrote:
> On Tue, Feb 27, 2024 at 12:05:49PM +0100, Mickaël Salaün wrote:
> > Extend the kernel support section with one subsection for build time
> > configuration and another for boot time configuration.
> > 
> > Extend the boot time subsection with a concrete example.
> > 
> > Update the journalctl command to include the boot option.
> > 
> > Cc: Günther Noack <gnoack@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> > 
> > Changes since v1:
> > * New patch, suggested by Kees Cook.
> > ---
> >  Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
> >  1 file changed, 51 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index 2e3822677061..838cc27db232 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> > +Boot time configuration
> > +-----------------------
> > +
> >  If the running kernel does not have ``landlock`` in ``CONFIG_LSM``, then we can
> > -still enable it by adding ``lsm=landlock,[...]`` to
> > +enable Landlock by adding ``lsm=landlock,[...]`` to
> >  Documentation/admin-guide/kernel-parameters.rst thanks to the bootloader
> >  configuration.
> 
> I would suggest: s/thanks to/in/

OK

> 
> > +For example, if the current built-in configuration is:
> > +
> > +.. code-block:: console
> > +
> > +    $ zgrep -h "^CONFIG_LSM=" "/boot/config-$(uname -r)" /proc/config.gz 2>/dev/null
> > +    CONFIG_LSM="lockdown,yama,integrity,apparmor"
> > +
> > +...and if the cmdline doesn't contain ``landlock`` either:
> > +
> > +.. code-block:: console
> > +
> > +    $ sed -n 's/.*\(\<lsm=\S\+\).*/\1/p' /proc/cmdline
> > +    lsm=lockdown,yama,integrity,apparmor
> > +
> > +...we should configure the bootloader to set a cmdline extending the ``lsm``
> > +list with the ``landlock,`` prefix::
> 
> Nit: Is the double colon at the end of this line accidental?
> (It does not appear before the previous code block.)

The "::" creates an implicit text block with the following text.  For the
other block I used an explicit code-block, which then doesn't require
this "::".

> 
> > +
> > +  lsm=landlock,lockdown,yama,integrity,apparmor
> > +
> > +After a reboot, we can check that Landlock is up and running by looking at
> > +kernel logs:
> > +
> > +.. code-block:: console
> > +
> > +    # dmesg | grep landlock || journalctl -kb -g landlock
> > +    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > +    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > +    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
> > +    [    0.000000] landlock: Up and running.
> > +
> > +Note that according to the built time kernel configuration,
> 
> s/built time/build time/
>                  ^

OK

> 
> It feels like the phrase "according to" could be slightly more specific here.
> 
> To paraphrase Alejandro Colomar, "Note that" is usually redundant.
> https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> 
> I'd suggest:
> 
>   The kernel may be configured at build time to always load the ``lockdown`` and
>   ``capability`` LSMs.  In that case, these LSMs will appear at the beginning of
>   the ``LSM: initializing`` log line as well, even if they are not configured in
>   the boot loader.

OK, I integrated your suggestion.  I guess `capability` is not really
considered an LSM but it would be too confusing and out of scope for an
user documentation to explain that.

> 
> > +``lockdown,capability,`` may always stay at the beginning of the ``LSM:
> > +initializing lsm=`` list even if they are not configured with the bootloader,
> 
> Nit: The man pages spell this in two words as "boot loader".

OK, I'll use "boot loader" too.

> 
> 
> > +which is OK.
> > +
> > +Network support
> > +---------------
> > +
> >  To be able to explicitly allow TCP operations (e.g., adding a network rule with
> >  ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
> >  (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an
> > 
> > base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
> > -- 
> > 2.44.0
> > 
> 
> Reviewed-by: Günther Noack <gnoack@google.com>

Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support
  2024-03-07 10:21   ` Mickaël Salaün
@ 2024-03-18  9:50     ` Alejandro Colomar
  2024-03-19 10:46       ` Mickaël Salaün
  0 siblings, 1 reply; 7+ messages in thread
From: Alejandro Colomar @ 2024-03-18  9:50 UTC (permalink / raw
  To: Mickaël Salaün
  Cc: Günther Noack, Paul Moore, Serge E . Hallyn,
	Konstantin Meskhidze, Shervin Oloumi, linux-hardening,
	linux-kernel, linux-security-module, Kees Cook

[-- Attachment #1: Type: text/plain, Size: 3577 bytes --]

Hi Mickaël, Günther,

Sorry for the delay!

On Thu, Mar 07, 2024 at 11:21:57AM +0100, Mickaël Salaün wrote:
> CCing Alejandro
> 
> On Tue, Feb 27, 2024 at 05:32:20PM +0100, Günther Noack wrote:
> > On Tue, Feb 27, 2024 at 12:05:49PM +0100, Mickaël Salaün wrote:
> > > Extend the kernel support section with one subsection for build time
> > > configuration and another for boot time configuration.
> > > 
> > > Extend the boot time subsection with a concrete example.
> > > 
> > > Update the journalctl command to include the boot option.
> > > 
> > > Cc: Günther Noack <gnoack@google.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > > 
> > > Changes since v1:
> > > * New patch, suggested by Kees Cook.
> > > ---
> > >  Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
> > >  1 file changed, 51 insertions(+), 6 deletions(-)

[...]

> > > +
> > > +  lsm=landlock,lockdown,yama,integrity,apparmor
> > > +
> > > +After a reboot, we can check that Landlock is up and running by looking at
> > > +kernel logs:
> > > +
> > > +.. code-block:: console
> > > +
> > > +    # dmesg | grep landlock || journalctl -kb -g landlock
> > > +    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > +    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > +    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
> > > +    [    0.000000] landlock: Up and running.
> > > +
> > > +Note that according to the built time kernel configuration,
> > 
> > s/built time/build time/
> >                  ^
> 
> OK

Here, this should actually be "build-time" since it works as an
adjective.

> 
> > 
> > It feels like the phrase "according to" could be slightly more specific here.
> > 
> > To paraphrase Alejandro Colomar, "Note that" is usually redundant.
> > https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> > 
> > I'd suggest:
> > 
> >   The kernel may be configured at build time to always load the ``lockdown`` and
> >   ``capability`` LSMs.  In that case, these LSMs will appear at the beginning of
> >   the ``LSM: initializing`` log line as well, even if they are not configured in
> >   the boot loader.

LGTM

> 
> OK, I integrated your suggestion.  I guess `capability` is not really
> considered an LSM but it would be too confusing and out of scope for an
> user documentation to explain that.
> 
> > 
> > > +``lockdown,capability,`` may always stay at the beginning of the ``LSM:
> > > +initializing lsm=`` list even if they are not configured with the bootloader,
> > 
> > Nit: The man pages spell this in two words as "boot loader".
> 
> OK, I'll use "boot loader" too.
> 
> > 
> > 
> > > +which is OK.
> > > +
> > > +Network support
> > > +---------------
> > > +
> > >  To be able to explicitly allow TCP operations (e.g., adding a network rule with
> > >  ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
> > >  (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an
> > > 
> > > base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
> > > -- 
> > > 2.44.0
> > > 
> > 
> > Reviewed-by: Günther Noack <gnoack@google.com>
> 
> Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support
  2024-03-18  9:50     ` Alejandro Colomar
@ 2024-03-19 10:46       ` Mickaël Salaün
  2024-03-19 11:40         ` Alejandro Colomar
  0 siblings, 1 reply; 7+ messages in thread
From: Mickaël Salaün @ 2024-03-19 10:46 UTC (permalink / raw
  To: Alejandro Colomar
  Cc: Günther Noack, Paul Moore, Serge E . Hallyn,
	Konstantin Meskhidze, Shervin Oloumi, linux-hardening,
	linux-kernel, linux-security-module, Kees Cook

On Mon, Mar 18, 2024 at 10:50:42AM +0100, Alejandro Colomar wrote:
> Hi Mickaël, Günther,
> 
> Sorry for the delay!
> 
> On Thu, Mar 07, 2024 at 11:21:57AM +0100, Mickaël Salaün wrote:
> > CCing Alejandro
> > 
> > On Tue, Feb 27, 2024 at 05:32:20PM +0100, Günther Noack wrote:
> > > On Tue, Feb 27, 2024 at 12:05:49PM +0100, Mickaël Salaün wrote:
> > > > Extend the kernel support section with one subsection for build time
> > > > configuration and another for boot time configuration.
> > > > 
> > > > Extend the boot time subsection with a concrete example.
> > > > 
> > > > Update the journalctl command to include the boot option.
> > > > 
> > > > Cc: Günther Noack <gnoack@google.com>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > ---
> > > > 
> > > > Changes since v1:
> > > > * New patch, suggested by Kees Cook.
> > > > ---
> > > >  Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
> > > >  1 file changed, 51 insertions(+), 6 deletions(-)
> 
> [...]
> 
> > > > +
> > > > +  lsm=landlock,lockdown,yama,integrity,apparmor
> > > > +
> > > > +After a reboot, we can check that Landlock is up and running by looking at
> > > > +kernel logs:
> > > > +
> > > > +.. code-block:: console
> > > > +
> > > > +    # dmesg | grep landlock || journalctl -kb -g landlock
> > > > +    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > > +    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > > +    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
> > > > +    [    0.000000] landlock: Up and running.
> > > > +
> > > > +Note that according to the built time kernel configuration,
> > > 
> > > s/built time/build time/
> > >                  ^
> > 
> > OK
> 
> Here, this should actually be "build-time" since it works as an
> adjective.

Thanks Alex but this was already merged:
https://git.kernel.org/torvalds/c/35e886e88c803920644c9d3abb45a9ecb7f1e761

Because I picked Günther's below suggestion, it should be good right?

> 
> > 
> > > 
> > > It feels like the phrase "according to" could be slightly more specific here.
> > > 
> > > To paraphrase Alejandro Colomar, "Note that" is usually redundant.
> > > https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> > > 
> > > I'd suggest:
> > > 
> > >   The kernel may be configured at build time to always load the ``lockdown`` and
> > >   ``capability`` LSMs.  In that case, these LSMs will appear at the beginning of
> > >   the ``LSM: initializing`` log line as well, even if they are not configured in
> > >   the boot loader.
> 
> LGTM
> 
> > 
> > OK, I integrated your suggestion.  I guess `capability` is not really
> > considered an LSM but it would be too confusing and out of scope for an
> > user documentation to explain that.
> > 
> > > 
> > > > +``lockdown,capability,`` may always stay at the beginning of the ``LSM:
> > > > +initializing lsm=`` list even if they are not configured with the bootloader,
> > > 
> > > Nit: The man pages spell this in two words as "boot loader".
> > 
> > OK, I'll use "boot loader" too.
> > 
> > > 
> > > 
> > > > +which is OK.
> > > > +
> > > > +Network support
> > > > +---------------
> > > > +
> > > >  To be able to explicitly allow TCP operations (e.g., adding a network rule with
> > > >  ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
> > > >  (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an
> > > > 
> > > > base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
> > > > -- 
> > > > 2.44.0
> > > > 
> > > 
> > > Reviewed-by: Günther Noack <gnoack@google.com>
> > 
> > Thanks!
> 
> Reviewed-by: Alejandro Colomar <alx@kernel.org>
> 
> Have a lovely day!
> Alex
> 
> -- 
> <https://www.alejandro-colomar.es/>
> Looking for a remote C programming job at the moment.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support
  2024-03-19 10:46       ` Mickaël Salaün
@ 2024-03-19 11:40         ` Alejandro Colomar
  0 siblings, 0 replies; 7+ messages in thread
From: Alejandro Colomar @ 2024-03-19 11:40 UTC (permalink / raw
  To: Mickaël Salaün
  Cc: Günther Noack, Paul Moore, Serge E . Hallyn,
	Konstantin Meskhidze, Shervin Oloumi, linux-hardening,
	linux-kernel, linux-security-module, Kees Cook

[-- Attachment #1: Type: text/plain, Size: 4852 bytes --]

Hi Mickaël!

On Tue, Mar 19, 2024 at 11:46:34AM +0100, Mickaël Salaün wrote:
> On Mon, Mar 18, 2024 at 10:50:42AM +0100, Alejandro Colomar wrote:
> > Hi Mickaël, Günther,
> > 
> > Sorry for the delay!
> > 
> > On Thu, Mar 07, 2024 at 11:21:57AM +0100, Mickaël Salaün wrote:
> > > CCing Alejandro
> > > 
> > > On Tue, Feb 27, 2024 at 05:32:20PM +0100, Günther Noack wrote:
> > > > On Tue, Feb 27, 2024 at 12:05:49PM +0100, Mickaël Salaün wrote:
> > > > > Extend the kernel support section with one subsection for build time
> > > > > configuration and another for boot time configuration.
> > > > > 
> > > > > Extend the boot time subsection with a concrete example.
> > > > > 
> > > > > Update the journalctl command to include the boot option.
> > > > > 
> > > > > Cc: Günther Noack <gnoack@google.com>
> > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > ---
> > > > > 
> > > > > Changes since v1:
> > > > > * New patch, suggested by Kees Cook.
> > > > > ---
> > > > >  Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
> > > > >  1 file changed, 51 insertions(+), 6 deletions(-)
> > 
> > [...]
> > 
> > > > > +
> > > > > +  lsm=landlock,lockdown,yama,integrity,apparmor
> > > > > +
> > > > > +After a reboot, we can check that Landlock is up and running by looking at
> > > > > +kernel logs:
> > > > > +
> > > > > +.. code-block:: console
> > > > > +
> > > > > +    # dmesg | grep landlock || journalctl -kb -g landlock
> > > > > +    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > > > +    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > > > +    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
> > > > > +    [    0.000000] landlock: Up and running.
> > > > > +
> > > > > +Note that according to the built time kernel configuration,
> > > > 
> > > > s/built time/build time/
> > > >                  ^
> > > 
> > > OK
> > 
> > Here, this should actually be "build-time" since it works as an
> > adjective.
> 
> Thanks Alex but this was already merged:
> https://git.kernel.org/torvalds/c/35e886e88c803920644c9d3abb45a9ecb7f1e761
> 
> Because I picked Günther's below suggestion, it should be good right?

Yeah, it's a minor grammar mistake that is widespread elsewhere.  If you
want to patch it, go ahead, if you want to keep it until next time you
revise this text, it's not something that will significantly hurt the
understanding of the text.

See also: <https://www.grammar-monster.com/lessons/hyphens_in_compound_adjectives.htm>

Have a lovely day!
Alex

> 
> > 
> > > 
> > > > 
> > > > It feels like the phrase "according to" could be slightly more specific here.
> > > > 
> > > > To paraphrase Alejandro Colomar, "Note that" is usually redundant.
> > > > https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> > > > 
> > > > I'd suggest:
> > > > 
> > > >   The kernel may be configured at build time to always load the ``lockdown`` and
> > > >   ``capability`` LSMs.  In that case, these LSMs will appear at the beginning of
> > > >   the ``LSM: initializing`` log line as well, even if they are not configured in
> > > >   the boot loader.
> > 
> > LGTM
> > 
> > > 
> > > OK, I integrated your suggestion.  I guess `capability` is not really
> > > considered an LSM but it would be too confusing and out of scope for an
> > > user documentation to explain that.
> > > 
> > > > 
> > > > > +``lockdown,capability,`` may always stay at the beginning of the ``LSM:
> > > > > +initializing lsm=`` list even if they are not configured with the bootloader,
> > > > 
> > > > Nit: The man pages spell this in two words as "boot loader".
> > > 
> > > OK, I'll use "boot loader" too.
> > > 
> > > > 
> > > > 
> > > > > +which is OK.
> > > > > +
> > > > > +Network support
> > > > > +---------------
> > > > > +
> > > > >  To be able to explicitly allow TCP operations (e.g., adding a network rule with
> > > > >  ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
> > > > >  (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an
> > > > > 
> > > > > base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
> > > > > -- 
> > > > > 2.44.0
> > > > > 
> > > > 
> > > > Reviewed-by: Günther Noack <gnoack@google.com>
> > > 
> > > Thanks!
> > 
> > Reviewed-by: Alejandro Colomar <alx@kernel.org>
> > 
> > Have a lovely day!
> > Alex
> > 
> > -- 
> > <https://www.alejandro-colomar.es/>
> > Looking for a remote C programming job at the moment.
> 
> 

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-03-19 11:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-27 11:05 [PATCH v2 1/2] landlock: Extend documentation for kernel support Mickaël Salaün
2024-02-27 11:05 ` [PATCH v2 2/2] landlock: Warn once if a Landlock action is requested while disabled Mickaël Salaün
2024-02-27 16:32 ` [PATCH v2 1/2] landlock: Extend documentation for kernel support Günther Noack
2024-03-07 10:21   ` Mickaël Salaün
2024-03-18  9:50     ` Alejandro Colomar
2024-03-19 10:46       ` Mickaël Salaün
2024-03-19 11:40         ` Alejandro Colomar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).