Message-ID: <20251223.69fdc8e48fce@gnoack.org>
Date: Tue, 23 Dec 2025 23:22:36 +0100
From: Günther Noack <gnoack3000@...il.com>
To: Samasth Norway Ananda <samasth.norway.ananda@...cle.com>
Cc: mic@...ikod.net, gnoack@...gle.com,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] landlock: Add missing ABI 7 case in documentation
 example

Hello!

On Tue, Dec 16, 2025 at 01:02:42PM -0800, Samasth Norway Ananda wrote:
> Add the missing case 6 and case 7 handling in the ABI version
> compatibility example to properly handle LANDLOCK_RESTRICT_SELF_LOG_*
> flags for kernels with ABI < 7.
> 
> This introduces the supported_restrict_flags variable which is
> initialized with LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON, removed
> in case 6 for ABI < 7, and passed to landlock_restrict_self() to
> enable logging on supported kernels.
> 
> Also fix misleading description of the /usr rule which incorrectly
> stated it "only allow[s] reading" when the code actually allows both
> reading and executing (LANDLOCK_ACCESS_FS_EXECUTE is included in
> allowed_access).
> 
> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@...cle.com>

Thank you for sending a patch, much appreciated!

You are right to point this out - the logging aspect is a bit hard to
spot when reading the current documentation starting from the code
example.


> ---
>  Documentation/userspace-api/landlock.rst | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 1d0c2c15c22e..b8caac299056 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -97,6 +97,8 @@ version, and only use the available subset of access rights:
>  .. code-block:: c
>  
>      int abi;
> +    /* Tracks which landlock_restrict_self() flags are supported */
> +    int supported_restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;

This might be confusing, as there are actually more supported flags:
ABI v7 does not only introduce LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON,
but also LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF and
LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF.

I am unconvinced whether it is a good idea to set these flags in the
first example that we have in the documentation, especially if we
don't discuss what these flags do there.  If we suggest the wrong
default flags in the example, people might use them in their
production code without realizing what they do.  The way that the
logging flags are designed, the assumption was that most users should
be able to pass 0 as flags to landlock_restrict_self() and still get
the relevant parts of the audit logging.

But you are right that an implementation that *does pass* logging
flags will want check the ABI and not pass them on older kernels.

To throw in a constructive suggestion: we could also have "backwards
compatibility for restrict flags" section between the existing case
analysis and the landlock_restrict_self() call?  It could then say
something like

    When passing a non-zero `flags` argument to
    landlock_restrict_self(), the following backwards compatibility
    check needs to be taken into account:

      /*
       * Desired restriction flags, see section suchandsuch.
       * This value is only an example and differs by use case.
       */
      int restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;
      if (abi < 7) {
        /* clear the necessary bits */
        restrict_flags &= ~...;
      }

Readers who do not need to pass any flags could then skip over that
section (I assume these are most readers).  The readers who *do* want
to pass flags could merge that logic into the bigger case analysis
themselves, but for the sake of explaining it we would not mix up that
explanation with the access right discussion that much.

WDYT?

>      abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
>      if (abi < 0) {
> @@ -127,6 +129,17 @@ version, and only use the available subset of access rights:
>          /* Removes LANDLOCK_SCOPE_* for ABI < 6 */
>          ruleset_attr.scoped &= ~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET |
>                                   LANDLOCK_SCOPE_SIGNAL);
> +        __attribute__((fallthrough));
> +    case 6:
> +        /*
> +         * Removes LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON for ABI < 7.
> +         * Note: This modifies supported_restrict_flags, not ruleset_attr,
> +         * because logging flags are passed to landlock_restrict_self().
> +         */
> +        supported_restrict_flags &= ~LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;

If this should be a generic example, we would have to clear the other
two flags here as well.

> +        __attribute__((fallthrough));
> +    case 7:
> +        break;
>      }

Thanks,
–Günther

Powered by blists - more mailing lists