[PATCH] landlock: Warn once if a Landlock action is requested while disabled

[PATCH] landlock: Warn once if a Landlock action is requested while disabled

[Mickaël Salaün]

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: Günther Noack <gnoack@google.com>
Cc: stable@vger.kernel.org
Fixes: 265885daf3e5 ("landlock: Add syscall implementations")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 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 f0bc50003b46..b5b424819dee 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -33,6 +33,18 @@
 #include "ruleset.h"
 #include "setup.h"
 
+static bool is_not_initialized(void)
+{
+	if (likely(landlock_initialized))
+		return false;
+
+	pr_warn_once(
+		"Disabled but requested by user space. "
+		"You should enable Landlock at boot time: "
+		"https://docs.kernel.org/userspace-api/landlock.html#kernel-support\n");
+	return true;
+}
+
 /**
  * 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_not_initialized())
 		return -EOPNOTSUPP;
 
 	if (flags) {
@@ -407,7 +419,7 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
 	struct landlock_ruleset *ruleset;
 	int err;
 
-	if (!landlock_initialized)
+	if (is_not_initialized())
 		return -EOPNOTSUPP;
 
 	/* No flag for now. */
@@ -467,7 +479,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_not_initialized())
 		return -EOPNOTSUPP;
 
 	/*
-- 
2.43.0

Re: [PATCH] landlock: Warn once if a Landlock action is requested while disabled

[Kees Cook]

On Mon, Feb 19, 2024 at 08:18:04PM +0100, Mickaël Salaün wrote:
> 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: Günther Noack <gnoack@google.com>
> Cc: stable@vger.kernel.org
> Fixes: 265885daf3e5 ("landlock: Add syscall implementations")
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  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 f0bc50003b46..b5b424819dee 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -33,6 +33,18 @@
>  #include "ruleset.h"
>  #include "setup.h"
>  
> +static bool is_not_initialized(void)
> +{
> +	if (likely(landlock_initialized))
> +		return false;
> +
> +	pr_warn_once(
> +		"Disabled but requested by user space. "
> +		"You should enable Landlock at boot time: "
> +		"https://docs.kernel.org/userspace-api/landlock.html#kernel-support\n");

Perhaps update this docs to be really explicit with a example, maybe...

If `landlock` is not present in `CONFIG_LSM`, you can add it. For
example, if this was the current config::

  $ zgrep -h ^CONFIG_LSM= /boot/config-$(uname -r) /proc/config.gz 2>/dev/null
  CONFIG_LSM="lockdown,yama,integrity,apparmor"

You can boot with::

  lsm=landlock,lockdown,yama,integrity,apparmor


I *still* wish we had the "+" operator for lsm=. It would be SO much
easier to say "boot with lsm=+landlock". *shrug*


Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] landlock: Warn once if a Landlock action is requested while disabled

[Mickaël Salaün]

On Mon, Feb 19, 2024 at 01:07:48PM -0800, Kees Cook wrote:
> On Mon, Feb 19, 2024 at 08:18:04PM +0100, Mickaël Salaün wrote:
> > 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: Günther Noack <gnoack@google.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 265885daf3e5 ("landlock: Add syscall implementations")
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  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 f0bc50003b46..b5b424819dee 100644
> > --- a/security/landlock/syscalls.c
> > +++ b/security/landlock/syscalls.c
> > @@ -33,6 +33,18 @@
> >  #include "ruleset.h"
> >  #include "setup.h"
> >  
> > +static bool is_not_initialized(void)
> > +{
> > +	if (likely(landlock_initialized))
> > +		return false;
> > +
> > +	pr_warn_once(
> > +		"Disabled but requested by user space. "
> > +		"You should enable Landlock at boot time: "
> > +		"https://docs.kernel.org/userspace-api/landlock.html#kernel-support\n");
> 
> Perhaps update this docs to be really explicit with a example, maybe...
> 
> If `landlock` is not present in `CONFIG_LSM`, you can add it. For
> example, if this was the current config::
> 
>   $ zgrep -h ^CONFIG_LSM= /boot/config-$(uname -r) /proc/config.gz 2>/dev/null
>   CONFIG_LSM="lockdown,yama,integrity,apparmor"
> 
> You can boot with::
> 
>   lsm=landlock,lockdown,yama,integrity,apparmor
> 

Indeed, I'll send a dedicated patch and update the link accordingly.

> 
> I *still* wish we had the "+" operator for lsm=. It would be SO much
> easier to say "boot with lsm=+landlock". *shrug*

I guess it's still possible and it would need to be backported to be
more useful.

> 
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> -- 
> Kees Cook
> 

Re: [PATCH] landlock: Warn once if a Landlock action is requested while disabled

[Günther Noack]

Hello!

I think this is a good idea.
Some minor implementation remarks below.

On Mon, Feb 19, 2024 at 08:18:04PM +0100, Mickaël Salaün wrote:
> 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: Günther Noack <gnoack@google.com>
> Cc: stable@vger.kernel.org
> Fixes: 265885daf3e5 ("landlock: Add syscall implementations")
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  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 f0bc50003b46..b5b424819dee 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -33,6 +33,18 @@
>  #include "ruleset.h"
>  #include "setup.h"
>  
> +static bool is_not_initialized(void)
> +{
> +	if (likely(landlock_initialized))
> +		return false;

Optional stylistic remark; I try to avoid predicate functions which
have a "negated" meaning, because double negations are slightly more
error prone.  (We return false here, so Landlock is not not
initialized.)

> +
> +	pr_warn_once(
> +		"Disabled but requested by user space. "
> +		"You should enable Landlock at boot time: "
> +		"https://docs.kernel.org/userspace-api/landlock.html#kernel-support\n");
> +	return true;
> +}
> +
>  /**
>   * 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_not_initialized())
>  		return -EOPNOTSUPP;

Technically, any Landlock user needs to go through the
landlock_create_ruleset() system call anyway; it might be enough to
just add it in that place and leave the other system calls as they
were.  Then you could also omit the special function.

Reviewed-by: Günther Noack <gnoack3000@gmail.com>

–Günther

Re: [PATCH] landlock: Warn once if a Landlock action is requested while disabled

[Mickaël Salaün]

On Wed, Feb 21, 2024 at 10:35:50PM +0100, Günther Noack wrote:
> Hello!
> 
> I think this is a good idea.
> Some minor implementation remarks below.
> 
> On Mon, Feb 19, 2024 at 08:18:04PM +0100, Mickaël Salaün wrote:
> > 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: Günther Noack <gnoack@google.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 265885daf3e5 ("landlock: Add syscall implementations")
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  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 f0bc50003b46..b5b424819dee 100644
> > --- a/security/landlock/syscalls.c
> > +++ b/security/landlock/syscalls.c
> > @@ -33,6 +33,18 @@
> >  #include "ruleset.h"
> >  #include "setup.h"
> >  
> > +static bool is_not_initialized(void)
> > +{
> > +	if (likely(landlock_initialized))
> > +		return false;
> 
> Optional stylistic remark; I try to avoid predicate functions which
> have a "negated" meaning, because double negations are slightly more
> error prone.  (We return false here, so Landlock is not not
> initialized.)

I agree, I was also bothered about this double negation. I'll send a v2
with the same behavior but an is_initialized() helper instead.

> 
> > +
> > +	pr_warn_once(
> > +		"Disabled but requested by user space. "
> > +		"You should enable Landlock at boot time: "
> > +		"https://docs.kernel.org/userspace-api/landlock.html#kernel-support\n");
> > +	return true;
> > +}
> > +
> >  /**
> >   * 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_not_initialized())
> >  		return -EOPNOTSUPP;
> 
> Technically, any Landlock user needs to go through the
> landlock_create_ruleset() system call anyway; it might be enough to
> just add it in that place and leave the other system calls as they
> were.  Then you could also omit the special function.

True, but we never know. I prefer to cover all entry points the same
way.  It makes things more consistent and easier to review.

> 
> Reviewed-by: Günther Noack <gnoack3000@gmail.com>
> 
> –Günther
>