On 11/02/2022 21:11, Stefan Sperling wrote:
> On Fri, Feb 11, 2022 at 06:15:43PM +0100, Mickaël Salaün wrote:
>> The documentation about handled_access_fs says: "Bitmask of actions (cf.
>> Filesystem flags) that is handled by this ruleset and should then be
>> forbidden if no rule explicitly allow them. This is needed for backward
>> compatibility reasons."
>>
>> I guess it is too succinct. What do you think about that:

(applied to https://docs.kernel.org/userspace-api/landlock.html)

>>
>> "[...] 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 [...]"
>>
>> And before that, in the example, after "The ruleset then needs to handle
>> both of these kind of actions", I can add "This is required for backward and
>> forward compatibility (i.e. the kernel and user space may not know each
>> other's supported restrictions), hence the need to be explicit about the
>> denied-by-default access rights."
>   
> Yes, this would make the docs a lot clearer.
> 
> I suppose developers using landlock are supposed to keep their applications
> up-to-date and expand their bitmask definition whenever new flags are
> added to the landlock API?
> 
> If so, it might be worth pointing this out in the documentation, to ensure
> readers understand that writing this list in their code is not a one-time
> action, and their attention will be required when new flags are added.
> Something like:
> "Future versions of landlock may define additional filesystem flags,
> which applications must add to handled_access_fs if a deny-by-default
> policy is desired."

Right, I'll improve the documentation in this area.

> 
> Which makes me wonder if applications which prefer to always fail closed
> could simply set all the bits. Would this break anything?
> 
> struct landlock_ruleset_attr ruleset_attr = {
>      .handled_access_fs = 0xffffffffffffffff;
> };
> 
> Otherwise, how about providing a macro which programs like Got could
> use to initialize this mask to all currently known filesystem flags?
> 
> #define LANDLOCK_ACCESS_FS_POLICY_DENY_ALL \
> 	(LANDLOCK_ACCESS_FS_EXECUTE | \
> 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> 	... etc.)
> 	
> 
> struct landlock_ruleset_attr ruleset_attr = {
>      .handled_access_fs = LANDLOCK_ACCESS_FS_POLICY_DENY_ALL;
> };
> 
> With this, only one copy of a default-deny mask would need to be maintained.
> Applications could stay in sync and keep failing closed by recompiling them
> against new landlock header files. Would this work?

The issue with these approaches is that we don't know the future and new 
Landlock accesses could break an application. I don't want this to be 
silent: the same (application) code should produce the same restrictions 
(which would not be the true if an included variable change over time). 
I agree that there is a use case to block all filesystem-related actions 
(even if the frontier with other subsystems might be blurry, e.g. UNIX 
socket), but I think it would be too bug-prone for applications to rely 
on that. As a kernel developer, I cannot assure that applications using 
this API would not suddenly break if built with a new set of 
(kernel-provided) headers. In this case, a conservative approach seems 
better for users but also for (the sanity of) most developers. ;)

One of the goal of the landlock@lists.linux.dev [1] mailing list is for 
developers to keep an eye on Landlock evolution.

[1] https://subspace.kernel.org/lists.linux.dev.html