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

[Mickaël Salaün]

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

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 at gmail.com>
>> Cc: Paul Moore <paul at paul-moore.com>
>> Signed-off-by: Mickaël Salaün <mic at 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
>>
>