[PATCH] landlock: Clarify documentation for struct landlock_ruleset_attr

[PATCH] landlock: Clarify documentation for struct landlock_ruleset_attr

[Günther Noack]

The explanation for @handled_access_fs and @handled_access_net has
significant overlap and is better explained together.  I tried to clarify
the wording and break up longer sentences as well.  I am putting emphasis
on the word "handled" to make it clearer that "handled" is a term with
special meaning in the context of Landlock.

I'd like to transfer this wording into the man pages as well.

Signed-off-by: Günther Noack <gnoack@google.com>
Cc: Alejandro Colomar <alx@kernel.org>
Cc: Mickaël Salaün <mic@digikod.net>
Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: linux-security-module@vger.kernel.org
---
 include/uapi/linux/landlock.h | 42 ++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..6f1b05c6995b 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -12,30 +12,32 @@
 #include <linux/types.h>
 
 /**
- * struct landlock_ruleset_attr - Ruleset definition
+ * struct landlock_ruleset_attr - Ruleset definition.
  *
- * Argument of sys_landlock_create_ruleset().  This structure can grow in
- * future versions.
+ * @handled_access_fs: Bitmask of handled filesystem actions (cf. `Filesystem flags`_)
+ * @handled_access_net: Bitmask of handled network actions (cf. `Network flags`_)
+ *
+ * Argument of sys_landlock_create_ruleset().
+ *
+ * This struct defines a set of *handled access rights*, a set of actions on
+ * different object types, which should be denied by default when the ruleset is
+ * enacted.  Vice versa, access rights that are not specifically listed here are
+ * going to be allowed when the ruleset is enacted.
+ *
+ * One exception is the %LANDLOCK_ACCESS_FS_REFER access right, which is always
+ * implicitly *handled*, even when its bit is not set in @handled_access_fs.
+ * However, in order to add new rules with this access right, the bit must still
+ * be set explicitly.
+ *
+ * The explicit listing of *handled access rights* is required for backwards
+ * compatibility reasons.  In most use cases, processes that use Landlock will
+ * *handle* a wide range or all access rights that they know about at build
+ * time.
+ *
+ * This structure can grow in future Landlock versions.
  */
 struct landlock_ruleset_attr {
-	/**
-	 * @handled_access_fs: Bitmask of actions (cf. `Filesystem flags`_)
-	 * that is handled by this ruleset and should then be forbidden if no
-	 * rule explicitly allow them: it is a deny-by-default list that should
-	 * contain as much Landlock access rights as possible. Indeed, all
-	 * Landlock filesystem access rights that are not part of
-	 * handled_access_fs are allowed.  This is needed for backward
-	 * compatibility reasons.  One exception is the
-	 * %LANDLOCK_ACCESS_FS_REFER access right, which is always implicitly
-	 * handled, but must still be explicitly handled to add new rules with
-	 * this access right.
-	 */
 	__u64 handled_access_fs;
-	/**
-	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
-	 * that is handled by this ruleset and should then be forbidden if no
-	 * rule explicitly allow them.
-	 */
 	__u64 handled_access_net;
 };
 
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH] landlock: Clarify documentation for struct landlock_ruleset_attr

[Alejandro Colomar]

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

Hi Günther,

On Wed, Jul 10, 2024 at 12:01:34PM +0000, Günther Noack wrote:
> The explanation for @handled_access_fs and @handled_access_net has
> significant overlap and is better explained together.  I tried to clarify
> the wording and break up longer sentences as well.  I am putting emphasis
> on the word "handled" to make it clearer that "handled" is a term with
> special meaning in the context of Landlock.
> 
> I'd like to transfer this wording into the man pages as well.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> Cc: Alejandro Colomar <alx@kernel.org>
> Cc: Mickaël Salaün <mic@digikod.net>
> Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> Cc: linux-security-module@vger.kernel.org
> ---
>  include/uapi/linux/landlock.h | 42 ++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..6f1b05c6995b 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -12,30 +12,32 @@
>  #include <linux/types.h>
>  
>  /**
> - * struct landlock_ruleset_attr - Ruleset definition
> + * struct landlock_ruleset_attr - Ruleset definition.
>   *
> - * Argument of sys_landlock_create_ruleset().  This structure can grow in
> - * future versions.
> + * @handled_access_fs: Bitmask of handled filesystem actions (cf. `Filesystem flags`_)
> + * @handled_access_net: Bitmask of handled network actions (cf. `Network flags`_)
> + *
> + * Argument of sys_landlock_create_ruleset().
> + *
> + * This struct defines a set of *handled access rights*, a set of actions on

s/struct/structure/

> + * different object types, which should be denied by default when the ruleset is
> + * enacted.  Vice versa, access rights that are not specifically listed here are
> + * going to be allowed when the ruleset is enacted.
> + *
> + * One exception is the %LANDLOCK_ACCESS_FS_REFER access right, which is always
> + * implicitly *handled*, even when its bit is not set in @handled_access_fs.
> + * However, in order to add new rules with this access right, the bit must still
> + * be set explicitly.
> + *
> + * The explicit listing of *handled access rights* is required for backwards
> + * compatibility reasons.  In most use cases, processes that use Landlock will
> + * *handle* a wide range or all access rights that they know about at build
> + * time.
> + *
> + * This structure can grow in future Landlock versions.
>   */
>  struct landlock_ruleset_attr {
> -	/**
> -	 * @handled_access_fs: Bitmask of actions (cf. `Filesystem flags`_)
> -	 * that is handled by this ruleset and should then be forbidden if no
> -	 * rule explicitly allow them: it is a deny-by-default list that should
> -	 * contain as much Landlock access rights as possible. Indeed, all
> -	 * Landlock filesystem access rights that are not part of
> -	 * handled_access_fs are allowed.  This is needed for backward
> -	 * compatibility reasons.  One exception is the
> -	 * %LANDLOCK_ACCESS_FS_REFER access right, which is always implicitly
> -	 * handled, but must still be explicitly handled to add new rules with
> -	 * this access right.
> -	 */
>  	__u64 handled_access_fs;
> -	/**
> -	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
> -	 * that is handled by this ruleset and should then be forbidden if no
> -	 * rule explicitly allow them.
> -	 */

LGTM.

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

Cheers,
Alex

>  	__u64 handled_access_net;
>  };
>  
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

-- 
<https://www.alejandro-colomar.es/>

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

Re: [PATCH] landlock: Clarify documentation for struct landlock_ruleset_attr

[Mickaël Salaün]

On Wed, Jul 10, 2024 at 12:01:34PM +0000, Günther Noack wrote:
> The explanation for @handled_access_fs and @handled_access_net has
> significant overlap and is better explained together.  I tried to clarify
> the wording and break up longer sentences as well.  I am putting emphasis
> on the word "handled" to make it clearer that "handled" is a term with
> special meaning in the context of Landlock.
> 
> I'd like to transfer this wording into the man pages as well.

Thanks for working on this.

> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> Cc: Alejandro Colomar <alx@kernel.org>
> Cc: Mickaël Salaün <mic@digikod.net>
> Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> Cc: linux-security-module@vger.kernel.org
> ---
>  include/uapi/linux/landlock.h | 42 ++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..6f1b05c6995b 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -12,30 +12,32 @@
>  #include <linux/types.h>
>  
>  /**
> - * struct landlock_ruleset_attr - Ruleset definition
> + * struct landlock_ruleset_attr - Ruleset definition.
>   *
> - * Argument of sys_landlock_create_ruleset().  This structure can grow in
> - * future versions.
> + * @handled_access_fs: Bitmask of handled filesystem actions (cf. `Filesystem flags`_)
> + * @handled_access_net: Bitmask of handled network actions (cf. `Network flags`_)

These @handled_* lines should be kept close the the related fields to
ease maintenance and consistency.  It looks like the Sphinx rendering
would be the same.

> + *
> + * Argument of sys_landlock_create_ruleset().
> + *
> + * This struct defines a set of *handled access rights*, a set of actions on
> + * different object types, which should be denied by default when the ruleset is

> + * enacted.  Vice versa, access rights that are not specifically listed here are
> + * going to be allowed when the ruleset is enacted.

They could still be denied because of other access controls or parent
Landlock domains.

> + *
> + * One exception is the %LANDLOCK_ACCESS_FS_REFER access right, which is always
> + * implicitly *handled*, even when its bit is not set in @handled_access_fs.

I wrote this sentence but I now think it might be confusing.
LANDLOCK_ACCESS_FS_REFER couldn't be handled before it was introduced
(with Linux 5.19).  I couldn't find a better way to explain it though.

> + * However, in order to add new rules with this access right, the bit must still
> + * be set explicitly.
> + *
> + * The explicit listing of *handled access rights* is required for backwards
> + * compatibility reasons.  In most use cases, processes that use Landlock will
> + * *handle* a wide range or all access rights that they know about at build
> + * time.

...and that they tested with a kernel supporting all of them.

> + *
> + * This structure can grow in future Landlock versions.
>   */
>  struct landlock_ruleset_attr {
> -	/**
> -	 * @handled_access_fs: Bitmask of actions (cf. `Filesystem flags`_)
> -	 * that is handled by this ruleset and should then be forbidden if no
> -	 * rule explicitly allow them: it is a deny-by-default list that should
> -	 * contain as much Landlock access rights as possible. Indeed, all
> -	 * Landlock filesystem access rights that are not part of
> -	 * handled_access_fs are allowed.  This is needed for backward
> -	 * compatibility reasons.  One exception is the
> -	 * %LANDLOCK_ACCESS_FS_REFER access right, which is always implicitly
> -	 * handled, but must still be explicitly handled to add new rules with
> -	 * this access right.
> -	 */
>  	__u64 handled_access_fs;
> -	/**
> -	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
> -	 * that is handled by this ruleset and should then be forbidden if no
> -	 * rule explicitly allow them.
> -	 */
>  	__u64 handled_access_net;
>  };
>  
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 
> 

Re: [PATCH] landlock: Clarify documentation for struct landlock_ruleset_attr

[Günther Noack]

On Wed, Jul 10, 2024 at 04:15:47PM +0200, Mickaël Salaün wrote:
> On Wed, Jul 10, 2024 at 12:01:34PM +0000, Günther Noack wrote:
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > Cc: Alejandro Colomar <alx@kernel.org>
> > Cc: Mickaël Salaün <mic@digikod.net>
> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> > Cc: linux-security-module@vger.kernel.org
> > ---
> >  include/uapi/linux/landlock.h | 42 ++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 68625e728f43..6f1b05c6995b 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -12,30 +12,32 @@
> >  #include <linux/types.h>
> >  
> >  /**
> > - * struct landlock_ruleset_attr - Ruleset definition
> > + * struct landlock_ruleset_attr - Ruleset definition.
> >   *
> > - * Argument of sys_landlock_create_ruleset().  This structure can grow in
> > - * future versions.
> > + * @handled_access_fs: Bitmask of handled filesystem actions (cf. `Filesystem flags`_)
> > + * @handled_access_net: Bitmask of handled network actions (cf. `Network flags`_)
> 
> These @handled_* lines should be kept close the the related fields to
> ease maintenance and consistency.  It looks like the Sphinx rendering
> would be the same.

Done.


> > + * Argument of sys_landlock_create_ruleset().
> > + *
> > + * This struct defines a set of *handled access rights*, a set of actions on
> > + * different object types, which should be denied by default when the ruleset is
> 
> > + * enacted.  Vice versa, access rights that are not specifically listed here are
> > + * going to be allowed when the ruleset is enacted.
> 
> They could still be denied because of other access controls or parent
> Landlock domains.

Done, reworded as

    Vice versa, access rights that are not specifically listed here are not
    going to be denied by this ruleset when it is enacted.

Now it's more technically correct, without having to add a lot more text.


> > + * One exception is the %LANDLOCK_ACCESS_FS_REFER access right, which is always
> > + * implicitly *handled*, even when its bit is not set in @handled_access_fs.
> 
> I wrote this sentence but I now think it might be confusing.
> LANDLOCK_ACCESS_FS_REFER couldn't be handled before it was introduced
> (with Linux 5.19).  I couldn't find a better way to explain it though.

I think the best description we have is in the "Filesystem flags" section
further down in this header file, where all the individual flags are explained.
I reworded it slightly and added a cross-reference to that section.

> 
> > + * However, in order to add new rules with this access right, the bit must still
> > + * be set explicitly.
> > + *
> > + * The explicit listing of *handled access rights* is required for backwards
> > + * compatibility reasons.  In most use cases, processes that use Landlock will
> > + * *handle* a wide range or all access rights that they know about at build
> > + * time.
> 
> ...and that they tested with a kernel supporting all of them.

Added.

Thank you for the review!
—Günther