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

[Mickaël Salaün <mic@xxxxxxxxxxx>]

On 23/09/2022 23:02, Günther Noack wrote:
> Reviewed-by: Günther Noack <gnoack3000@xxxxxxxxx>
>
> 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

I agree. The sandboxer is a sample, and such sandboxer is not the best 
place to configure Landlock according to the app semantic. At the end it 
should be done in the app itself.

I would like this sample to be as simple as possible but still useful. 
To properly handle all "refer" use cases, it would require a dedicated 
configuration (e.g. LL_FS_REFER), which will make it more difficult to 
understand, and this approach will not scale with future (FS) access rights.

We can maybe add a comment in this sample to explain that. Your 
explanation looks like a good start. If you agree, could you send a 
patch to add such comment (on top of this series)?

> —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@xxxxxxxxx>
> > Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20220923154207.3311629-2-mic@xxxxxxxxxxx
> > ---
> >   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