Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720518 > unrolled thread

Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to  filesystem Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-26 03:20 +0200
    Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to  filesystem Mickaël Salaün <mic@digikod.net> - 2017-08-27 15:40 +0200
      Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to  filesystem Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-28 07:30 +0200

#1720518 — Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem

On Fri, Aug 25, 2017 at 10:16:39AM +0200, Mickaël Salaün wrote:
> > 
> >> +/* WRAP_ARG_SB */
> >> +#define WRAP_ARG_SB_TYPE	WRAP_TYPE_FS
> >> +#define WRAP_ARG_SB_DEC(arg)					\
> >> +	EXPAND_C(WRAP_TYPE_FS) wrap_##arg =			\
> >> +	{ .type = BPF_HANDLE_FS_TYPE_DENTRY, .dentry = arg->s_root };
> >> +#define WRAP_ARG_SB_VAL(arg)	((uintptr_t)&wrap_##arg)
> >> +#define WRAP_ARG_SB_OK(arg)	(arg && arg->s_root)
> > ...
> > 
> >> +HOOK_NEW_FS(sb_remount, 2,
> >> +	struct super_block *, sb,
> >> +	void *, data,
> >> +	WRAP_ARG_SB, sb,
> >> +	WRAP_ARG_RAW, LANDLOCK_ACTION_FS_WRITE
> >> +);
> > 
> > this looks wrong. casting super_block to dentry?
> 
> This is called when remounting a block device. The WRAP_ARG_SB take the
> sb->s_root as a dentry, it is not a cast. What do you expect from this hook?

got it. I missed -> part. Now it makes sense.

> > 
> >> +/* 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 ?
I guess that will simplify the rules indeed.
I suspect you're doing it to simplify the LSM->landlock shim layer as well, right?

> 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?
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.

[toc] | [next] | [standalone]

#1720822

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1721026

On Sun, Aug 27, 2017 at 03:31:35PM +0200, Mickaël Salaün wrote:
> 
> > 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.

that's exactly my point. To add another argument FS event
to a subset of hooks will require either new FS_FOO and
to be backwards compatible these hooks will call _both_ FS and FS_FOO
or some magic logic on kernel side that will allow old FS rules
to be attached to FS_FOO hooks?
Two calls doesn't scale and if we do 'magic logic' can we do it now
and avoid introducing events altogether?
Like all landlock programs can be landlock type and they would need
to declare what arg1, arg2, argN they expect. Then at attach
time the kernel only needs to verify that hook arg types match
what program requested.

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

I don't think that abi/version field adds anything in this context.
I still think it should simply be removed.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web