On 11/02/2022 11:39, Stefan Sperling wrote:
> On Thu, Feb 10, 2022 at 06:34:54PM +0100, Mickaël Salaün wrote:
>> Hi,
>>
>> I noticed Omar Polo added support for Landlock to the Linux version of Game
>> Of Trees [1]. This is great! However, the handled filesystem access is only
>> LANDLOCK_ACCESS_FS_READ_FILE, and it will still be allowed to do multiple
>> filesystem-related actions (e.g. write to files, remove files…). I don't
>> know much about Game Of Trees but, according to the commit message, I think
>> you would like to revoke any (currently supported) filesystem access. You
>> should then add the 12 remaining access rights [2]. There is also a typo in
>> the errno check, it should be EOPNOTSUPP (not ENOTSUP). You'll find a small
>> patch attached. Let me know if I can help.
> 
> Thank you for looking at this Mickaël, it helps us a great deal!

You're welcome. Omar did the hardest part, thanks to him!

>   
>> In a nutshell, the ruleset's handled_access_fs 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.
> 
> I suspect Omar trimmed down the list of fs flags was in response to
> a question I asked during code review. The code initially followed
> the example given in your docs, but we ended up diverging from it.
> 
> If the point you made above was explained in your docs it would be
> much easier to understand why there is a long list of access rules
> in struct rndlock_ruleset_attr, many of which may not seem relevant
> to the application's purposes. Character or block devices will rarely
> be needed by applications, for example. The name "handled_access_fs"
> does not immediately bring to mind that this could be a default deny list.

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:

"[...] 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."