Date:   Sun, 27 Aug 2017 15:31:35 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     linux-kernel@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Drysdale <drysdale@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        James Morris <james.l.morris@...cle.com>,
        Jann Horn <jann@...jh.net>, Jonathan Corbet <corbet@....net>,
        Matthew Garrett <mjg59@...f.ucam.org>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Paul Moore <paul@...l-moore.com>,
        Sargun Dhillon <sargun@...gun.me>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Shuah Khan <shuah@...nel.org>, Tejun Heo <tj@...nel.org>,
        Thomas Graf <tgraf@...g.ch>, Will Drewry <wad@...omium.org>,
        kernel-hardening@...ts.openwall.com, linux-api@...r.kernel.org,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to
 filesystem


On 26/08/2017 03:16, Alexei Starovoitov wrote:
> On Fri, Aug 25, 2017 at 10:16:39AM +0200, Mickaël Salaün wrote:

>>>
>>>> +/* a directory inode contains only one dentry */
>>>> +HOOK_NEW_FS(inode_create, 3,
>>>> +	struct inode *, dir,
>>>> +	struct dentry *, dentry,
>>>> +	umode_t, mode,
>>>> +	WRAP_ARG_INODE, dir,
>>>> +	WRAP_ARG_RAW, LANDLOCK_ACTION_FS_WRITE
>>>> +);
>>>
>>> more general question: why you're not wrapping all useful
>>> arguments? Like in the above dentry can be acted upon
>>> by the landlock rule and it's readily available...
>>
>> The context used for the FS event must have the exact same types for all
>> calls. This event is meant to be generic but we can add more specific
>> ones if needed, like I do with FS_IOCTL.
> 
> I see. So all FS events will have dentry as first argument
> regardless of how it is in LSM hook ?

All FS events will have a const struct bpf_handle_fs pointer as first
argument, which wrap either a struct file, a struct dentry, a struct
path or a struct inode. Having only one type (struct bpf_handle_fs) is
needed for the eBPF type checker to verify if a Landlock rule (tied to
an event) can access a context field and which operation is allowed
(with this pointer).

> I guess that will simplify the rules indeed.
> I suspect you're doing it to simplify the LSM->landlock shim layer as well, right?

That's right. This ABI is independent from the LSM API and much more
simpler to use.

> 
>> The idea is to enable people to write simple rules, while being able to
>> write fine grain rules for special cases (e.g. IOCTL) if needed.
>>
>>>
>>> The limitation of only 2 args looks odd.
>>> Is it a hard limitation ? how hard to extend?
>>
>> It's not a hard limit at all. Actually, the FS_FNCTL event should have
>> three arguments (I'll add them in the next series): FS handle, FCNTL
>> command and FCNTL argument. I made sure that it's really easy to add
>> more arguments to the context of an event.
> 
> The reason I'm asking, because I'm not completely convinced that
> adding another argument to existing event will be backwards compatible.
> It looks like you're expecting only two args for all FS events, right?

There is four events right now: FS, FS_IOCTL, FS_LOCK and FS_FCNTL. Each
of them are independent. Their context fields can be of the same or
different eBPF type (e.g. scalar, file handle) and numbers. Actually,
these four events have the same arg1 field (file handle) and the same
arg2 eBPF type (scalar), even if arg2 does not have the same semantic
(i.e. abstract FS action, IOCTL command…).

For example, if we want to extend the FS_FCNTL's context in the future,
we will just have to add an arg3. The check is performed in
landlock_is_valid_access() and landlock_decide(). If a field is not used
by an event, then this field will have a NOT_INIT type and accessing it
will be denied.

> How can you add 3rd argument? All FS events would have to get it,
> but in some LSM hooks such argument will be meaningless, whereas
> in other places it will carry useful info that rule can operate on.
> Would that mean that we'll have FS_3 event type and only few LSM
> hooks will be converted to it. That works, but then we'll lose
> compatiblity with old rules written for FS event and that given hook.
> Otherwise we'd need to have fancy logic to accept old FS event
> into FS_3 LSM hook.

If we want to add a third argument to the FS event, then it will become
accessible because its type will be different than NOT_INIT. This keep
the compatibility with old rules because this new field was then denied.

If we want to add a new argument but only for a subset of the hooks used
by the FS event, then we need to create a new event, like FS_FCNTL. For
example, we may want to add a FS_RENAME event to be able to tie the
source file and the destination file of a rename call.

Anyway, I added the subtype/ABI version as a safeguard in case of
unexpected future evolution.



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists