[PATCH v7 0/1] landlock.7: Explain best-effort fallback in example

[PATCH v7 0/1] landlock.7: Explain best-effort fallback in example

[Günther Noack]

Hello!

Same patch as before, with these changes:

 * Use the MIN() macro instead of an explicit "if".
 * Point out in a comment what the error scenarios are
   when we can not retrieve the Landlock ABI version.

I'm avoiding to spell out the exact error codes,
as they are already documented in the respective man page
for the syscall.

–Günther


Previous mail thread:
v6: https://lore.kernel.org/linux-man/20230414155926.6937-1-gnoack3000@gmail.com/

Günther Noack (1):
  landlock.7: Explain the best-effort fallback mechanism in the example

 man7/landlock.7 | 73 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)


base-commit: 6263befb32fdc99dd5d02b6afdd5613db9df4c3b
-- 
2.40.0

[PATCH v7 1/1] landlock.7: Explain the best-effort fallback mechanism in the example

[Günther Noack]

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 man7/landlock.7 | 73 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/man7/landlock.7 b/man7/landlock.7
index 24488465e..16feef42c 100644
--- a/man7/landlock.7
+++ b/man7/landlock.7
@@ -394,11 +394,14 @@ accessible through these system call families:
 Future Landlock evolutions will enable to restrict them.
 .SH EXAMPLES
 We first need to create the ruleset that will contain our rules.
+.PP
 For this example,
 the ruleset will contain rules that only allow read actions,
 but write actions will be denied.
 The ruleset then needs to handle both of these kinds of actions.
-See below for the description of filesystem actions.
+See the
+.B DESCRIPTION
+section for the description of filesystem actions.
 .PP
 .in +4n
 .EX
@@ -421,7 +424,65 @@ attr.handled_access_fs =
         LANDLOCK_ACCESS_FS_MAKE_SYM |
         LANDLOCK_ACCESS_FS_REFER |
         LANDLOCK_ACCESS_FS_TRUNCATE;
+.EE
+.in
+.PP
+To be compatible with older Linux versions,
+we detect the available Landlock ABI version,
+and only use the available subset of access rights:
+.PP
+.in +4n
+.EX
+/*
+ * Table of available file system access rights by ABI version,
+ * numbers hardcoded to keep the example short.
+ */
+__u64 landlock_fs_access_rights[] = {
+    (1ULL << 13) \- 1,  /* ABI v1                 */
+    (1ULL << 14) \- 1,  /* ABI v2: add "refer"    */
+    (1ULL << 15) \- 1,  /* ABI v3: add "truncate" */
+};
+
+int abi = landlock_create_ruleset(NULL, 0,
+                                  LANDLOCK_CREATE_RULESET_VERSION);
+if (abi <= 0) {
+    /*
+     * Kernel too old, not compiled with Landlock,
+     * or Landlock was not enabled at boot time.
+     */
+    perror("Giving up \- No Landlock support");
+    exit(EXIT_FAILURE);
+}
+abi = MIN(abi, 3);
 
+/* Only use the available rights in the ruleset. */
+attr.handled_access_fs &= landlock_fs_access_rights[abi \- 1];
+.EE
+.in
+.PP
+The available access rights for each ABI version are listed in the
+.B VERSIONS
+section.
+.PP
+If our program needed to create hard links
+or rename files between different directories
+.RB ( LANDLOCK_ACCESS_FS_REFER ),
+we would require the following change to the backwards compatibility logic:
+Directory reparenting is not possible
+in a process restricted with Landlock ABI version 1.
+Therefore,
+if the program needed to do file reparenting,
+and if only Landlock ABI version 1 was available,
+we could not restrict the process.
+.PP
+Now that the ruleset attributes are determined,
+we create the Landlock ruleset
+and acquire a file descriptor as a handle to it,
+using
+.BR landlock_create_ruleset (2):
+.PP
+.in +4n
+.EX
 ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
 if (ruleset_fd == \-1) {
     perror("Failed to create a ruleset");
@@ -430,9 +491,13 @@ if (ruleset_fd == \-1) {
 .EE
 .in
 .PP
-We can now add a new rule to this ruleset thanks to the returned file
-descriptor referring to this ruleset.
-The rule will only allow reading the file hierarchy
+We can now add a new rule to the ruleset through the ruleset's file descriptor.
+The requested access rights must be a subset of the access rights
+which were specified in
+.I attr.handled_access_fs
+at ruleset creation time.
+.PP
+In this example, the rule will only allow reading the file hierarchy
 .IR /usr .
 Without another rule, write actions would then be denied by the ruleset.
 To add
-- 
2.40.0

Re: [PATCH v7 0/1] landlock.7: Explain best-effort fallback in example

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]

Hola!

On 4/17/23 19:25, Günther Noack wrote:
> Hello!
> 
> Same patch as before, with these changes:
> 
>  * Use the MIN() macro instead of an explicit "if".
>  * Point out in a comment what the error scenarios are
>    when we can not retrieve the Landlock ABI version.
> 
> I'm avoiding to spell out the exact error codes,
> as they are already documented in the respective man page
> for the syscall.

Makes sense.

> 
> –Günther
> 
> 
> Previous mail thread:
> v6: https://lore.kernel.org/linux-man/20230414155926.6937-1-gnoack3000@gmail.com/
> 
> Günther Noack (1):
>   landlock.7: Explain the best-effort fallback mechanism in the example

Patch applied.

BTW, it would be nice to use --range-diff in git-format-patch(1).  :)

Cheers,
Alex

> 
>  man7/landlock.7 | 73 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
> 
> 
> base-commit: 6263befb32fdc99dd5d02b6afdd5613db9df4c3b

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

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

Re: [PATCH v7 1/1] landlock.7: Explain the best-effort fallback mechanism in the example

[Mickaël Salaün]

On 17/04/2023 19:25, Günther Noack wrote:
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   man7/landlock.7 | 73 ++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/man7/landlock.7 b/man7/landlock.7
> index 24488465e..16feef42c 100644
> --- a/man7/landlock.7
> +++ b/man7/landlock.7
> @@ -394,11 +394,14 @@ accessible through these system call families:
>   Future Landlock evolutions will enable to restrict them.
>   .SH EXAMPLES
>   We first need to create the ruleset that will contain our rules.
> +.PP
>   For this example,
>   the ruleset will contain rules that only allow read actions,
>   but write actions will be denied.
>   The ruleset then needs to handle both of these kinds of actions.
> -See below for the description of filesystem actions.
> +See the
> +.B DESCRIPTION
> +section for the description of filesystem actions.
>   .PP
>   .in +4n
>   .EX
> @@ -421,7 +424,65 @@ attr.handled_access_fs =
>           LANDLOCK_ACCESS_FS_MAKE_SYM |
>           LANDLOCK_ACCESS_FS_REFER |
>           LANDLOCK_ACCESS_FS_TRUNCATE;
> +.EE
> +.in
> +.PP
> +To be compatible with older Linux versions,
> +we detect the available Landlock ABI version,
> +and only use the available subset of access rights:
> +.PP
> +.in +4n
> +.EX
> +/*
> + * Table of available file system access rights by ABI version,
> + * numbers hardcoded to keep the example short.
> + */
> +__u64 landlock_fs_access_rights[] = {
> +    (1ULL << 13) \- 1,  /* ABI v1                 */

This would be more explicit and avoid hardcoded values with:
(LANDLOCK_ACCESS_FS_MAKE_SYM << 1) - 1,
(LANDLOCK_ACCESS_FS_REFER << 1) - 1,
(LANDLOCK_ACCESS_FS_TRUNCATE << 1) - 1,


> +    (1ULL << 14) \- 1,  /* ABI v2: add "refer"    */
> +    (1ULL << 15) \- 1,  /* ABI v3: add "truncate" */
> +};
> +
> +int abi = landlock_create_ruleset(NULL, 0,
> +                                  LANDLOCK_CREATE_RULESET_VERSION);
> +if (abi <= 0) {
> +    /*
> +     * Kernel too old, not compiled with Landlock,
> +     * or Landlock was not enabled at boot time.
> +     */
> +    perror("Giving up \- No Landlock support");

The cause of the error will be appended by perror, so we can just say 
that we cannot use it:
perror("Unable to use Landlock");

As a side note, this syscall and this flag should never return 0, but if 
it does (e.g. because of weird seccomp filter), the errno value might be 
unspecified.


> +    exit(EXIT_FAILURE);

I'm not sure this example code should exit if Landlock is not supported 
because (most) developers don't want to exit if some (optional) security 
features are not available.


> +}
> +abi = MIN(abi, 3);
>   
> +/* Only use the available rights in the ruleset. */
> +attr.handled_access_fs &= landlock_fs_access_rights[abi \- 1];
> +.EE
> +.in
> +.PP
> +The available access rights for each ABI version are listed in the
> +.B VERSIONS
> +section.
> +.PP
> +If our program needed to create hard links
> +or rename files between different directories
> +.RB ( LANDLOCK_ACCESS_FS_REFER ),
> +we would require the following change to the backwards compatibility logic:
> +Directory reparenting is not possible
> +in a process restricted with Landlock ABI version 1.
> +Therefore,
> +if the program needed to do file reparenting,
> +and if only Landlock ABI version 1 was available,
> +we could not restrict the process.
> +.PP
> +Now that the ruleset attributes are determined,
> +we create the Landlock ruleset
> +and acquire a file descriptor as a handle to it,
> +using
> +.BR landlock_create_ruleset (2):
> +.PP
> +.in +4n
> +.EX
>   ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
>   if (ruleset_fd == \-1) {
>       perror("Failed to create a ruleset");
> @@ -430,9 +491,13 @@ if (ruleset_fd == \-1) {
>   .EE
>   .in
>   .PP
> -We can now add a new rule to this ruleset thanks to the returned file
> -descriptor referring to this ruleset.
> -The rule will only allow reading the file hierarchy
> +We can now add a new rule to the ruleset through the ruleset's file descriptor.
> +The requested access rights must be a subset of the access rights
> +which were specified in
> +.I attr.handled_access_fs
> +at ruleset creation time.
> +.PP
> +In this example, the rule will only allow reading the file hierarchy
>   .IR /usr .
>   Without another rule, write actions would then be denied by the ruleset.
>   To add

Thanks Günther!

Re: [PATCH v7 1/1] landlock.7: Explain the best-effort fallback mechanism in the example

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 891 bytes --]

Hi Mickaël,

On 4/17/23 22:45, Mickaël Salaün wrote:

[...]

>> +int abi = landlock_create_ruleset(NULL, 0,
>> +                                  LANDLOCK_CREATE_RULESET_VERSION);
>> +if (abi <= 0) {
>> +    /*
>> +     * Kernel too old, not compiled with Landlock,
>> +     * or Landlock was not enabled at boot time.
>> +     */
>> +    perror("Giving up \- No Landlock support");

[...]

> As a side note, this syscall and this flag should never return 0, but if 
> it does (e.g. because of weird seccomp filter), the errno value might be 
> unspecified.

Hmm, good catch, we should test for `== -1`, rather than `< 0`.
Michael Kerrisk explicitly wanted this, and I agree with him, as it makes
the code slightly more readable (explicit).

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

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

Re: [PATCH v7 1/1] landlock.7: Explain the best-effort fallback mechanism in the example

[Günther Noack]

On Tue, Apr 18, 2023 at 04:24:49PM +0200, Alejandro Colomar wrote:
> Hi Mickaël,
> 
> On 4/17/23 22:45, Mickaël Salaün wrote:
> 
> [...]
> 
> >> +int abi = landlock_create_ruleset(NULL, 0,
> >> +                                  LANDLOCK_CREATE_RULESET_VERSION);
> >> +if (abi <= 0) {
> >> +    /*
> >> +     * Kernel too old, not compiled with Landlock,
> >> +     * or Landlock was not enabled at boot time.
> >> +     */
> >> +    perror("Giving up \- No Landlock support");
> 
> [...]
> 
> > As a side note, this syscall and this flag should never return 0, but if 
> > it does (e.g. because of weird seccomp filter), the errno value might be 
> > unspecified.
> 
> Hmm, good catch, we should test for `== -1`, rather than `< 0`.
> Michael Kerrisk explicitly wanted this, and I agree with him, as it makes
> the code slightly more readable (explicit).

Ack, I'll send a small follow-up.

–Günther