Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support

From: "Mickaël Salaün" <mic@digikod.net>
To: "Günther Noack" <gnoack@google.com>,
	"Alejandro Colomar" <alx@kernel.org>
Cc: Paul Moore <paul@paul-moore.com>,
	 "Serge E . Hallyn" <serge@hallyn.com>,
	Konstantin Meskhidze <konstantin.meskhidze@huawei.com>,
	 Shervin Oloumi <enlightened@chromium.org>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	 linux-security-module@vger.kernel.org,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support
Date: Thu, 7 Mar 2024 11:21:57 +0100	[thread overview]
Message-ID: <20240307.oxQuab5tho0u@digikod.net> (raw)
In-Reply-To: <Zd4OlL1G3t1D3TgC@google.com>

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!