Re: [PATCH v1 1/3] samples/landlock: Print hints about ABI versions

["Günther Noack" <gnoack3000@gmail.com>]

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

This patch is a strict improvement over what the sample code was
before, so that's fine with me review wise.

I still think it would be good to point out more explicitly that the
"refer" right needs a different fallback strategy for the best effort
mode than the other rights will do in the future, as discussed in [1].

In many "best effort" scenarios that people need for their code, the
part that is actually fixed are the access rights that their code
declares that it needs. So if they actually need the "refer" right for
their programs to work, they cannot use Landlock on kernels that only
support Landlock ABI v1, because under ABI v1 they will never be able
to hardlink or rename between directories when Landlock is enabled.

The way that the sandboxer example is dealing with it, it just gives
the user a smaller set of access rights than they requested if the
kernel just supports ABI v1. It's somewhat reasonable for the
sandboxer tool to do because it doesn't give hard guarantees in its
command line interface, but it might not be negotiable in more
practical examples. :)

[1] https://docs.google.com/document/d/1SkFpl_Xxyl4E6G2uYIlzL0gY2PFo-Nl8ikblLvnpvlU/edit

—Günther

On Fri, Sep 23, 2022 at 05:42:05PM +0200, Mickaël Salaün wrote:
> Extend the help with the latest Landlock ABI version supported by the
> sandboxer.
> 
> Inform users about the sandboxer or the kernel not being up-to-date.
> 
> Make the version check code easier to update and harder to misuse.
> 
> Cc: Günther Noack <gnoack3000@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20220923154207.3311629-2-mic@digikod.net
> ---
>  samples/landlock/sandboxer.c | 37 ++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index 3e404e51ec64..f29bb3c72230 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -162,11 +162,10 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
>  	LANDLOCK_ACCESS_FS_MAKE_SYM | \
>  	LANDLOCK_ACCESS_FS_REFER)
>  
> -#define ACCESS_ABI_2 ( \
> -	LANDLOCK_ACCESS_FS_REFER)
> -
>  /* clang-format on */
>  
> +#define LANDLOCK_ABI_LAST 2
> +
>  int main(const int argc, char *const argv[], char *const *const envp)
>  {
>  	const char *cmd_path;
> @@ -196,8 +195,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  			"\nexample:\n"
>  			"%s=\"/bin:/lib:/usr:/proc:/etc:/dev/urandom\" "
>  			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> -			"%s bash -i\n",
> +			"%s bash -i\n\n",
>  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, argv[0]);
> +		fprintf(stderr,
> +			"This sandboxer can use Landlock features "
> +			"up to ABI version %d.\n",
> +			LANDLOCK_ABI_LAST);
>  		return 1;
>  	}
>  
> @@ -225,12 +228,30 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  		}
>  		return 1;
>  	}
> +
>  	/* Best-effort security. */
> -	if (abi < 2) {
> -		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
> -		access_fs_ro &= ~ACCESS_ABI_2;
> -		access_fs_rw &= ~ACCESS_ABI_2;
> +	switch (abi) {
> +	case 1:
> +		/* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
> +		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> +
> +		fprintf(stderr,
> +			"Hint: You should update the running kernel "
> +			"to leverage Landlock features "
> +			"provided by ABI version %d (instead of %d).\n",
> +			LANDLOCK_ABI_LAST, abi);
> +		__attribute__((fallthrough));
> +	case LANDLOCK_ABI_LAST:
> +		break;
> +	default:
> +		fprintf(stderr,
> +			"Hint: You should update this sandboxer "
> +			"to leverage Landlock features "
> +			"provided by ABI version %d (instead of %d).\n",
> +			abi, LANDLOCK_ABI_LAST);
>  	}
> +	access_fs_ro &= ruleset_attr.handled_access_fs;
> +	access_fs_rw &= ruleset_attr.handled_access_fs;
>  
>  	ruleset_fd =
>  		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> -- 
> 2.37.2
> 

--