Re: [RFC v2 09/10] landlock: Handle cgroups

Re: [RFC v2 09/10] landlock: Handle cgroups

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 1850 bytes --]


On 25/08/2016 13:09, Andy Lutomirski wrote:
> On Thu, Aug 25, 2016 at 3:32 AM, Mickaël Salaün <mic@digikod.net> wrote:
>> Add an eBPF function bpf_landlock_cmp_cgroup_beneath(opt, map, map_op)
>> to compare the current process cgroup with a cgroup handle, The handle
>> can match the current cgroup if it is the same or a child. This allows
>> to make conditional rules according to the current cgroup.
>>
>> A cgroup handle is a map entry created from a file descriptor referring
>> a cgroup directory (e.g. by opening /sys/fs/cgroup/X). In this case, the
>> map entry is of type BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD and the
>> inferred array map is of type BPF_MAP_ARRAY_TYPE_LANDLOCK_CGROUP.
> 
> Can you elaborate on why this is useful?  I.e. why not just supply
> different policies to different subtrees.

The main use case I see is to load the security policies at the start of
a user session for all processes but not enforce them right away. The
user can then keep a shell for Landlock administration tasks and lock
the other processes with a dedicated cgroup on the fly. This allows the
user to make unremovable Landlock security policies but only activate
them when needed for specific processes.

> 
> Also, how does this interact with the current cgroup v1 vs v2 mess?
> As far as I can tell, no one can even really agree on what "what
> cgroup am I in" means right now.

I tested with cgroup-v2 but indeed, it seems a bit different with
cgroup-v1 :)
Does anyone know how to handle both cases?

> 
>>
>> An unprivileged process can create and manipulate cgroups thanks to
>> cgroup delegation.
> 
> What is cgroup delegation?

This is simply the action of changing the owner of cgroup sysfs files to
allow an unprivileged user to handle them (cf. Documentation/cgroup-v2.txt)

 Mickaël


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups

[Tejun Heo]

Hello,

On Thu, Aug 25, 2016 at 04:44:13PM +0200, Mickaël Salaün wrote:
> I tested with cgroup-v2 but indeed, it seems a bit different with
> cgroup-v1 :)
> Does anyone know how to handle both cases?

If you wanna do cgroup membership test, just do cgroup v2 membership
test.  No need to introduce a new controller and possibly struct sock
association field for that.  That's what all new cgroup aware network
operations are using anyway and doesn't conflicts with whether other
controllers are v1 or v2.

For examples of using cgroup v2 membership test, please take a look at
cgroup_mt_v1().

Thanks.

-- 
tejun

Re: [RFC v2 09/10] landlock: Handle cgroups

[Andy Lutomirski]

On Thu, Aug 25, 2016 at 7:44 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 25/08/2016 13:09, Andy Lutomirski wrote:
>> On Thu, Aug 25, 2016 at 3:32 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>> Add an eBPF function bpf_landlock_cmp_cgroup_beneath(opt, map, map_op)
>>> to compare the current process cgroup with a cgroup handle, The handle
>>> can match the current cgroup if it is the same or a child. This allows
>>> to make conditional rules according to the current cgroup.
>>>
>>> A cgroup handle is a map entry created from a file descriptor referring
>>> a cgroup directory (e.g. by opening /sys/fs/cgroup/X). In this case, the
>>> map entry is of type BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD and the
>>> inferred array map is of type BPF_MAP_ARRAY_TYPE_LANDLOCK_CGROUP.
>>
>> Can you elaborate on why this is useful?  I.e. why not just supply
>> different policies to different subtrees.
>
> The main use case I see is to load the security policies at the start of
> a user session for all processes but not enforce them right away. The
> user can then keep a shell for Landlock administration tasks and lock
> the other processes with a dedicated cgroup on the fly. This allows the
> user to make unremovable Landlock security policies but only activate
> them when needed for specific processes.

This seems like a bit of a dubious use case to me.  The landlock
mechanism should be flexible enough to do this kind of thing even
without cgroups, and "spawn a process, wait a while, and then confine
it by fiddling with cgroups" seems a lot dicier than just loading the
right policy in the first place, especially since eBPF policies can be
stateful.

>
>>
>> Also, how does this interact with the current cgroup v1 vs v2 mess?
>> As far as I can tell, no one can even really agree on what "what
>> cgroup am I in" means right now.
>
> I tested with cgroup-v2 but indeed, it seems a bit different with
> cgroup-v1 :)
> Does anyone know how to handle both cases?
>
>>
>>>
>>> An unprivileged process can create and manipulate cgroups thanks to
>>> cgroup delegation.
>>
>> What is cgroup delegation?
>
> This is simply the action of changing the owner of cgroup sysfs files to
> allow an unprivileged user to handle them (cf. Documentation/cgroup-v2.txt)

As far as I can tell, Tejun and systemd both actively discourage doing
this.  Maybe I misunderstand.  But in any event, the admin giving you
a cgroup hierarchy you can use for this means that the admin has to
cooperate with your policy, and it further requires (with cgroup v2 or
similar, which is most likely the future) that your lockdown policy be
compatible with your resource control policy.

I would suggest dropping this lockdown feature until a use case
emerges that really can't be addressed adequately without it.

--Andy

Re: [RFC v2 09/10] landlock: Handle cgroups

[Tejun Heo]

Hello,

On Fri, Aug 26, 2016 at 07:20:35AM -0700, Andy Lutomirski wrote:
> > This is simply the action of changing the owner of cgroup sysfs files to
> > allow an unprivileged user to handle them (cf. Documentation/cgroup-v2.txt)
> 
> As far as I can tell, Tejun and systemd both actively discourage doing
> this.  Maybe I misunderstand.  But in any event, the admin giving you

Please refer to "2-5. Delegation" of Documentation/cgroup-v2.txt.
Delegation on v1 is broken on both core and specific controller
behaviors and thus discouraged.  On v2, delegation should work just
fine.

I haven't looked in detail but in general I'm not too excited about
layering security mechanism on top of cgroup.  Maybe it makes some
sense when security domain coincides with resource domains but at any
rate please keep me in the loop.

Thanks.

-- 
tejun

Re: [RFC v2 09/10] landlock: Handle cgroups

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 10169 bytes --]


On 26/08/2016 04:14, Alexei Starovoitov wrote:
> On Thu, Aug 25, 2016 at 12:32:44PM +0200, Mickaël Salaün wrote:
>> Add an eBPF function bpf_landlock_cmp_cgroup_beneath(opt, map, map_op)
>> to compare the current process cgroup with a cgroup handle, The handle
>> can match the current cgroup if it is the same or a child. This allows
>> to make conditional rules according to the current cgroup.
>>
>> A cgroup handle is a map entry created from a file descriptor referring
>> a cgroup directory (e.g. by opening /sys/fs/cgroup/X). In this case, the
>> map entry is of type BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD and the
>> inferred array map is of type BPF_MAP_ARRAY_TYPE_LANDLOCK_CGROUP.
>>
>> An unprivileged process can create and manipulate cgroups thanks to
>> cgroup delegation.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ...
>> +static inline u64 bpf_landlock_cmp_cgroup_beneath(u64 r1_option, u64 r2_map,
>> +		u64 r3_map_op, u64 r4, u64 r5)
>> +{
>> +	u8 option = (u8) r1_option;
>> +	struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
>> +	enum bpf_map_array_op map_op = r3_map_op;
>> +	struct bpf_array *array = container_of(map, struct bpf_array, map);
>> +	struct cgroup *cg1, *cg2;
>> +	struct map_landlock_handle *handle;
>> +	int i;
>> +
>> +	/* ARG_CONST_PTR_TO_LANDLOCK_HANDLE_CGROUP is an arraymap */
>> +	if (unlikely(!map)) {
>> +		WARN_ON(1);
>> +		return -EFAULT;
>> +	}
>> +	if (unlikely((option | _LANDLOCK_FLAG_OPT_MASK) != _LANDLOCK_FLAG_OPT_MASK))
>> +		return -EINVAL;
>> +
>> +	/* for now, only handle OP_OR */
>> +	switch (map_op) {
>> +	case BPF_MAP_ARRAY_OP_OR:
>> +		break;
>> +	case BPF_MAP_ARRAY_OP_UNSPEC:
>> +	case BPF_MAP_ARRAY_OP_AND:
>> +	case BPF_MAP_ARRAY_OP_XOR:
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	synchronize_rcu();
>> +
>> +	for (i = 0; i < array->n_entries; i++) {
>> +		handle = (struct map_landlock_handle *)
>> +				(array->value + array->elem_size * i);
>> +
>> +		/* protected by the proto types, should not happen */
>> +		if (unlikely(handle->type != BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD)) {
>> +			WARN_ON(1);
>> +			return -EFAULT;
>> +		}
>> +		if (unlikely(!handle->css)) {
>> +			WARN_ON(1);
>> +			return -EFAULT;
>> +		}
>> +
>> +		if (option & LANDLOCK_FLAG_OPT_REVERSE) {
>> +			cg1 = handle->css->cgroup;
>> +			cg2 = task_css_set(current)->dfl_cgrp;
>> +		} else {
>> +			cg1 = task_css_set(current)->dfl_cgrp;
>> +			cg2 = handle->css->cgroup;
>> +		}
>> +
>> +		if (cgroup_is_descendant(cg1, cg2))
>> +			return 0;
>> +	}
>> +	return 1;
>> +}
> 
> - please take a loook at exisiting bpf_current_task_under_cgroup and
> reuse BPF_MAP_TYPE_CGROUP_ARRAY as a minimum. Doing new cgroup array
> is nothing but duplication of the code.

Oh, I didn't know about this patchset and the new helper. Indeed, it
looks a lot like mine except there is no static verification of the map
type as I did with the arraymap of handles, and no batch mode either. I
think the return value of bpf_current_task_under_cgroup is error-prone
if an eBPF program do an "if(ret)" test on the value (because of the
negative ERRNO return value). Inverting the 0 and 1 return values should
fix this (0 == succeed, 1 == failed, <0 == error).


To sum up, there is four related patchsets:
* "Landlock LSM: Unprivileged sandboxing" (this series)
* "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
* "Networking cgroup controller" (Anoop Naravaram)
* "Add eBPF hooks for cgroups" (Daniel Mack)

The three other series (Sargun's, Anoop's and Daniel's) are mainly
focused on network access-control via cgroup for *containers*. As far as
I can tell, only a *root* user (CAP_SYS_ADMIN) can use them. Landlock's
goal is to empower all processes (privileged or not) to create their own
sandbox. This also means, like explained in "[RFC v2 00/10] Landlock
LSM: Unprivileged sandboxing", there is more constraints. For example,
it is not acceptable to let a process probe the kernel memory as it
wish. More details are in the Landlock cover-letter.


Another important point is that supporting cgroup for Landlock is
optional. It does not rely on cgroup to be usable but is only a feature
available when (unprivileged) users can manage there own cgroup, which
is an important constraint. Put another way, Landlock should not rely on
cgroup to create sandboxes. Indeed, a process creating a sandbox do not
necessarily have access to the cgroup mount point (directly or not).


> 
> - I don't think such 'for' loop can scale. The solution needs to work
> with thousands of containers and thousands of cgroups.
> In the patch 06/10 the proposal is to use 'current' as holder of
> the programs:
> +   for (prog = current->seccomp.landlock_prog;
> +                   prog; prog = prog->prev) {
> +           if (prog->filter == landlock_ret->filter) {
> +                   cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
> +                   break;
> +           }
> +   }
> imo that's the root of scalability issue.
> I think to be able to scale the bpf programs have to be attached to
> cgroups instead of tasks.
> That would be very different api. seccomp doesn't need to be touched.
> But that is the only way I see to be able to scale.

Landlock is inspired from seccomp which also use a BPF program per
thread. For seccomp, each BPF programs are executed for each syscall.
For Landlock, some BPF programs are executed for some LSM hooks. I don't
see why it is a scale issue for Landlock comparing to seccomp. I also
don't see why storing the BPF program list pointer in the cgroup struct
instead of the task struct change a lot here. The BPF programs execution
will be the same anyway (for each LSM hook). Kees should probably have a
better opinion on this.


> May be another way of thinking about it is 'lsm cgroup controller'
> that Sargun is proposing.
> The lsm hooks will provide stable execution points and the programs
> will be called like:
> prog = task_css_set(current)->dfl_cgrp->bpf.prog_effective[lsm_hook_id];
> BPF_PROG_RUN(prog, ctx);
> The delegation functionality and 'prog_effective' logic that
> Daniel Mack is proposing will be fully reused here.
> External container management software will be able to apply bpf
> programs to control tasks under cgroup and such
> bpf_landlock_cmp_cgroup_beneath() helper won't be necessary.
> The user will be able to register different programs for different lsm hooks.
> If I understand the patch 6/10 correctly, there is one (or a list) prog for
> all lsm hooks per task which is not flexible enough.

For each LSM hook triggered by a thread, all of its Landlock eBPF
programs (dedicated for this kind of hook) will be evaluated (if
needed). This is the same behavior as seccomp (list of BPF programs
attached to a process hierarchy) except the BPF programs are not
evaluated for syscall but for LSM hooks. There is no way to make it more
fine-grained :)


> Anoop Naravaram's use case is to control the ports the applications
> under cgroup can bind and listen on.
> Such use case can be solved by such 'lsm cgroup controller' by
> attaching bpf program to security_socket_bind lsm hook and
> filtering sockaddr.
> Furthermore Sargun's use case is to allow further sockaddr rewrites
> from the bpf program which can be done as natural extension
> of such mechanism.
> 
> If I understood Daniel's Anoop's Sargun's and yours use cases
> correctly the common piece of kernel infrastructure that can solve
> them all can start from Daniel's current set of patches that
> establish a mechanism of attaching bpf program to a cgroup.
> Then adding lsm hooks to it and later allowing argument rewrite
> (since they're already in the kernel and no ToCToU problems exist)

To sum up, the pieces we have in common are the eBPF use and (optionally
for Landlock) there execution depending of the current cgroup.

Moreover, the three other series (Sargun's, Anoop's and Daniel's) do not
deal with unprivileged process which is the main purpose of Landlock.
I'm not sure that allowing sockaddr rewrites is a good idea here... Like
other LSMs, Landlock is dedicated to access-control.


For the network-related series, I think it make more sense to simply
create a netfilter rule matching a cgroup and then add more features to
netfilter (restrict port ranges and so on) thanks to eBPF programs.
Containers are (usually) in a dedicated network namespace, which open
the possibility to not only rely on cgroups (e.g. match UID,
netmask...). It would also be more flexible to be able to load a BPF
program in netfilter and update its maps on the fly to make dynamic
rules, like ipset does, but in a more generic way.


> 
> As far as safety and type checking that bpf programs has to do,
> I like the approach of patch 06/10:
> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> +       PTR_TO_STRUCT_FILE, struct file *, file,
> +       PTR_TO_STRUCT_CRED, const struct cred *, cred
> +)
> teaching verifier to recognize struct file, cred, sockaddr
> will let bpf program access them naturally without any overhead.
> Though:
> @@ -102,6 +102,9 @@ enum bpf_prog_type {
>         BPF_PROG_TYPE_SCHED_CLS,
>         BPF_PROG_TYPE_SCHED_ACT,
>         BPF_PROG_TYPE_TRACEPOINT,
> +       BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> +       BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> +       BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
>  };
> is a bit of overkill.
> I think it would be cleaner to have single
> BPF_PROG_TYPE_LSM and at program load time pass
> lsm_hook_id as well, so that verifier can do safety checks
> based on type info provided in LANDLOCK_HOOKs

I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
verifier check programs according to their types. If we need to check
specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
dedicated program types. I don't see any other way to do it with the
current verifier code. Moreover it's the purpose of program types, right?

Regards,
 Mickaël


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups

[Alexei Starovoitov]

On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> 

trimming cc list again. When it's too big vger will consider it as spam.

> On 26/08/2016 04:14, Alexei Starovoitov wrote:
> > On Thu, Aug 25, 2016 at 12:32:44PM +0200, Mickaël Salaün wrote:
> >> Add an eBPF function bpf_landlock_cmp_cgroup_beneath(opt, map, map_op)
> >> to compare the current process cgroup with a cgroup handle, The handle
> >> can match the current cgroup if it is the same or a child. This allows
> >> to make conditional rules according to the current cgroup.
> >>
> >> A cgroup handle is a map entry created from a file descriptor referring
> >> a cgroup directory (e.g. by opening /sys/fs/cgroup/X). In this case, the
> >> map entry is of type BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD and the
> >> inferred array map is of type BPF_MAP_ARRAY_TYPE_LANDLOCK_CGROUP.
> >>
> >> An unprivileged process can create and manipulate cgroups thanks to
> >> cgroup delegation.
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static inline u64 bpf_landlock_cmp_cgroup_beneath(u64 r1_option, u64 r2_map,
> >> +		u64 r3_map_op, u64 r4, u64 r5)
> >> +{
> >> +	u8 option = (u8) r1_option;
> >> +	struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
> >> +	enum bpf_map_array_op map_op = r3_map_op;
> >> +	struct bpf_array *array = container_of(map, struct bpf_array, map);
> >> +	struct cgroup *cg1, *cg2;
> >> +	struct map_landlock_handle *handle;
> >> +	int i;
> >> +
> >> +	/* ARG_CONST_PTR_TO_LANDLOCK_HANDLE_CGROUP is an arraymap */
> >> +	if (unlikely(!map)) {
> >> +		WARN_ON(1);
> >> +		return -EFAULT;
> >> +	}
> >> +	if (unlikely((option | _LANDLOCK_FLAG_OPT_MASK) != _LANDLOCK_FLAG_OPT_MASK))
> >> +		return -EINVAL;
> >> +
> >> +	/* for now, only handle OP_OR */
> >> +	switch (map_op) {
> >> +	case BPF_MAP_ARRAY_OP_OR:
> >> +		break;
> >> +	case BPF_MAP_ARRAY_OP_UNSPEC:
> >> +	case BPF_MAP_ARRAY_OP_AND:
> >> +	case BPF_MAP_ARRAY_OP_XOR:
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	synchronize_rcu();
> >> +
> >> +	for (i = 0; i < array->n_entries; i++) {
> >> +		handle = (struct map_landlock_handle *)
> >> +				(array->value + array->elem_size * i);
> >> +
> >> +		/* protected by the proto types, should not happen */
> >> +		if (unlikely(handle->type != BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD)) {
> >> +			WARN_ON(1);
> >> +			return -EFAULT;
> >> +		}
> >> +		if (unlikely(!handle->css)) {
> >> +			WARN_ON(1);
> >> +			return -EFAULT;
> >> +		}
> >> +
> >> +		if (option & LANDLOCK_FLAG_OPT_REVERSE) {
> >> +			cg1 = handle->css->cgroup;
> >> +			cg2 = task_css_set(current)->dfl_cgrp;
> >> +		} else {
> >> +			cg1 = task_css_set(current)->dfl_cgrp;
> >> +			cg2 = handle->css->cgroup;
> >> +		}
> >> +
> >> +		if (cgroup_is_descendant(cg1, cg2))
> >> +			return 0;
> >> +	}
> >> +	return 1;
> >> +}
> > 
> > - please take a loook at exisiting bpf_current_task_under_cgroup and
> > reuse BPF_MAP_TYPE_CGROUP_ARRAY as a minimum. Doing new cgroup array
> > is nothing but duplication of the code.
> 
> Oh, I didn't know about this patchset and the new helper. Indeed, it
> looks a lot like mine except there is no static verification of the map
> type as I did with the arraymap of handles, and no batch mode either. I
> think the return value of bpf_current_task_under_cgroup is error-prone
> if an eBPF program do an "if(ret)" test on the value (because of the
> negative ERRNO return value). Inverting the 0 and 1 return values should
> fix this (0 == succeed, 1 == failed, <0 == error).

nothing to fix. It's good as-is. Use if (ret > 0) instead.

> 
> To sum up, there is four related patchsets:
> * "Landlock LSM: Unprivileged sandboxing" (this series)
> * "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
> * "Networking cgroup controller" (Anoop Naravaram)
> * "Add eBPF hooks for cgroups" (Daniel Mack)
> 
> The three other series (Sargun's, Anoop's and Daniel's) are mainly
> focused on network access-control via cgroup for *containers*. As far as
> I can tell, only a *root* user (CAP_SYS_ADMIN) can use them. Landlock's
> goal is to empower all processes (privileged or not) to create their own
> sandbox. This also means, like explained in "[RFC v2 00/10] Landlock
> LSM: Unprivileged sandboxing", there is more constraints. For example,
> it is not acceptable to let a process probe the kernel memory as it
> wish. More details are in the Landlock cover-letter.
> 
> 
> Another important point is that supporting cgroup for Landlock is
> optional. It does not rely on cgroup to be usable but is only a feature
> available when (unprivileged) users can manage there own cgroup, which
> is an important constraint. Put another way, Landlock should not rely on
> cgroup to create sandboxes. Indeed, a process creating a sandbox do not
> necessarily have access to the cgroup mount point (directly or not).

cgroup is the common way to group multiple tasks.
Without cgroup only parent<->child relationship will be possible,
which will limit usability of such lsm to a master task that controls
its children. Such api restriction would have been ok, if we could
extend it in the future, but unfortunately task-centric won't allow it
without creating a parallel lsm that is cgroup based.
Therefore I think we have to go with cgroup-centric api and your
application has to use cgroups from the start though only parent-child
would have been enough.
Also I don't think the kernel can afford two bpf based lsm. One task
based and another cgroup based, so we have to find common ground
that suits both use cases.
Having unprivliged access is a subset. There is no strong reason why
cgroup+lsm+bpf should be limited to root only always.
When we can guarantee no pointer leaks, we can allow unpriv.

> > 
> > - I don't think such 'for' loop can scale. The solution needs to work
> > with thousands of containers and thousands of cgroups.
> > In the patch 06/10 the proposal is to use 'current' as holder of
> > the programs:
> > +   for (prog = current->seccomp.landlock_prog;
> > +                   prog; prog = prog->prev) {
> > +           if (prog->filter == landlock_ret->filter) {
> > +                   cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
> > +                   break;
> > +           }
> > +   }
> > imo that's the root of scalability issue.
> > I think to be able to scale the bpf programs have to be attached to
> > cgroups instead of tasks.
> > That would be very different api. seccomp doesn't need to be touched.
> > But that is the only way I see to be able to scale.
> 
> Landlock is inspired from seccomp which also use a BPF program per
> thread. For seccomp, each BPF programs are executed for each syscall.
> For Landlock, some BPF programs are executed for some LSM hooks. I don't
> see why it is a scale issue for Landlock comparing to seccomp. I also
> don't see why storing the BPF program list pointer in the cgroup struct
> instead of the task struct change a lot here. The BPF programs execution
> will be the same anyway (for each LSM hook). Kees should probably have a
> better opinion on this.

seccomp has its own issues and copying them doesn't make this lsm any better.
Like seccomp bpf programs are all gigantic switch statement that looks
for interesting syscall numbers. All syscalls of a task are paying
non-trivial seccomp penalty due to such design. If bpf was attached per
syscall it would have been much cheaper. Of course doing it this way
for seccomp is not easy, but for lsm such facility is already there.
Blank call of a single bpf prog for all lsm hooks is unnecessary
overhead that can and should be avoided.

> > May be another way of thinking about it is 'lsm cgroup controller'
> > that Sargun is proposing.
> > The lsm hooks will provide stable execution points and the programs
> > will be called like:
> > prog = task_css_set(current)->dfl_cgrp->bpf.prog_effective[lsm_hook_id];
> > BPF_PROG_RUN(prog, ctx);
> > The delegation functionality and 'prog_effective' logic that
> > Daniel Mack is proposing will be fully reused here.
> > External container management software will be able to apply bpf
> > programs to control tasks under cgroup and such
> > bpf_landlock_cmp_cgroup_beneath() helper won't be necessary.
> > The user will be able to register different programs for different lsm hooks.
> > If I understand the patch 6/10 correctly, there is one (or a list) prog for
> > all lsm hooks per task which is not flexible enough.
> 
> For each LSM hook triggered by a thread, all of its Landlock eBPF
> programs (dedicated for this kind of hook) will be evaluated (if
> needed). This is the same behavior as seccomp (list of BPF programs
> attached to a process hierarchy) except the BPF programs are not
> evaluated for syscall but for LSM hooks. There is no way to make it more
> fine-grained :)

There is a way to attach different bpf program per cgroup
and per lsm hook. Such approach drastically reduces overhead
of sandboxed application.

> > Anoop Naravaram's use case is to control the ports the applications
> > under cgroup can bind and listen on.
> > Such use case can be solved by such 'lsm cgroup controller' by
> > attaching bpf program to security_socket_bind lsm hook and
> > filtering sockaddr.
> > Furthermore Sargun's use case is to allow further sockaddr rewrites
> > from the bpf program which can be done as natural extension
> > of such mechanism.
> > 
> > If I understood Daniel's Anoop's Sargun's and yours use cases
> > correctly the common piece of kernel infrastructure that can solve
> > them all can start from Daniel's current set of patches that
> > establish a mechanism of attaching bpf program to a cgroup.
> > Then adding lsm hooks to it and later allowing argument rewrite
> > (since they're already in the kernel and no ToCToU problems exist)
> 
> To sum up, the pieces we have in common are the eBPF use and (optionally
> for Landlock) there execution depending of the current cgroup.
> 
> Moreover, the three other series (Sargun's, Anoop's and Daniel's) do not
> deal with unprivileged process which is the main purpose of Landlock.
> I'm not sure that allowing sockaddr rewrites is a good idea here... Like
> other LSMs, Landlock is dedicated to access-control.

we have to find common ground and common infra to solve all these use cases.
Pointing out the differences isn't going to make this snowflake
any more special.

> For the network-related series, I think it make more sense to simply
> create a netfilter rule matching a cgroup and then add more features to
> netfilter (restrict port ranges and so on) thanks to eBPF programs.
> Containers are (usually) in a dedicated network namespace, which open
> the possibility to not only rely on cgroups (e.g. match UID,
> netmask...). It would also be more flexible to be able to load a BPF
> program in netfilter and update its maps on the fly to make dynamic
> rules, like ipset does, but in a more generic way.
> 
> 
> > 
> > As far as safety and type checking that bpf programs has to do,
> > I like the approach of patch 06/10:
> > +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> > +       PTR_TO_STRUCT_FILE, struct file *, file,
> > +       PTR_TO_STRUCT_CRED, const struct cred *, cred
> > +)
> > teaching verifier to recognize struct file, cred, sockaddr
> > will let bpf program access them naturally without any overhead.
> > Though:
> > @@ -102,6 +102,9 @@ enum bpf_prog_type {
> >         BPF_PROG_TYPE_SCHED_CLS,
> >         BPF_PROG_TYPE_SCHED_ACT,
> >         BPF_PROG_TYPE_TRACEPOINT,
> > +       BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> > +       BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> > +       BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
> >  };
> > is a bit of overkill.
> > I think it would be cleaner to have single
> > BPF_PROG_TYPE_LSM and at program load time pass
> > lsm_hook_id as well, so that verifier can do safety checks
> > based on type info provided in LANDLOCK_HOOKs
> 
> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
> verifier check programs according to their types. If we need to check
> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
> dedicated program types. I don't see any other way to do it with the
> current verifier code. Moreover it's the purpose of program types, right?

Adding new bpf program type for every lsm hook is not acceptable.
Either do one new program type + pass lsm_hook_id as suggested
or please come up with an alternative approach.

Re: [RFC v2 09/10] landlock: Handle cgroups

[Andy Lutomirski]

On Aug 27, 2016 1:05 AM, "Alexei Starovoitov"
<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >
>
> trimming cc list again. When it's too big vger will consider it as spam.
>
> > On 26/08/2016 04:14, Alexei Starovoitov wrote:
> > > On Thu, Aug 25, 2016 at 12:32:44PM +0200, Mickaël Salaün wrote:
> > >> Add an eBPF function bpf_landlock_cmp_cgroup_beneath(opt, map, map_op)
> > >> to compare the current process cgroup with a cgroup handle, The handle
> > >> can match the current cgroup if it is the same or a child. This allows
> > >> to make conditional rules according to the current cgroup.
> > >>
> > >> A cgroup handle is a map entry created from a file descriptor referring
> > >> a cgroup directory (e.g. by opening /sys/fs/cgroup/X). In this case, the
> > >> map entry is of type BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD and the
> > >> inferred array map is of type BPF_MAP_ARRAY_TYPE_LANDLOCK_CGROUP.
> > >>
> > >> An unprivileged process can create and manipulate cgroups thanks to
> > >> cgroup delegation.
> > >>
> > >> Signed-off-by: Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
> > > ...
> > >> +static inline u64 bpf_landlock_cmp_cgroup_beneath(u64 r1_option, u64 r2_map,
> > >> +          u64 r3_map_op, u64 r4, u64 r5)
> > >> +{
> > >> +  u8 option = (u8) r1_option;
> > >> +  struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
> > >> +  enum bpf_map_array_op map_op = r3_map_op;
> > >> +  struct bpf_array *array = container_of(map, struct bpf_array, map);
> > >> +  struct cgroup *cg1, *cg2;
> > >> +  struct map_landlock_handle *handle;
> > >> +  int i;
> > >> +
> > >> +  /* ARG_CONST_PTR_TO_LANDLOCK_HANDLE_CGROUP is an arraymap */
> > >> +  if (unlikely(!map)) {
> > >> +          WARN_ON(1);
> > >> +          return -EFAULT;
> > >> +  }
> > >> +  if (unlikely((option | _LANDLOCK_FLAG_OPT_MASK) != _LANDLOCK_FLAG_OPT_MASK))
> > >> +          return -EINVAL;
> > >> +
> > >> +  /* for now, only handle OP_OR */
> > >> +  switch (map_op) {
> > >> +  case BPF_MAP_ARRAY_OP_OR:
> > >> +          break;
> > >> +  case BPF_MAP_ARRAY_OP_UNSPEC:
> > >> +  case BPF_MAP_ARRAY_OP_AND:
> > >> +  case BPF_MAP_ARRAY_OP_XOR:
> > >> +  default:
> > >> +          return -EINVAL;
> > >> +  }
> > >> +
> > >> +  synchronize_rcu();
> > >> +
> > >> +  for (i = 0; i < array->n_entries; i++) {
> > >> +          handle = (struct map_landlock_handle *)
> > >> +                          (array->value + array->elem_size * i);
> > >> +
> > >> +          /* protected by the proto types, should not happen */
> > >> +          if (unlikely(handle->type != BPF_MAP_HANDLE_TYPE_LANDLOCK_CGROUP_FD)) {
> > >> +                  WARN_ON(1);
> > >> +                  return -EFAULT;
> > >> +          }
> > >> +          if (unlikely(!handle->css)) {
> > >> +                  WARN_ON(1);
> > >> +                  return -EFAULT;
> > >> +          }
> > >> +
> > >> +          if (option & LANDLOCK_FLAG_OPT_REVERSE) {
> > >> +                  cg1 = handle->css->cgroup;
> > >> +                  cg2 = task_css_set(current)->dfl_cgrp;
> > >> +          } else {
> > >> +                  cg1 = task_css_set(current)->dfl_cgrp;
> > >> +                  cg2 = handle->css->cgroup;
> > >> +          }
> > >> +
> > >> +          if (cgroup_is_descendant(cg1, cg2))
> > >> +                  return 0;
> > >> +  }
> > >> +  return 1;
> > >> +}
> > >
> > > - please take a loook at exisiting bpf_current_task_under_cgroup and
> > > reuse BPF_MAP_TYPE_CGROUP_ARRAY as a minimum. Doing new cgroup array
> > > is nothing but duplication of the code.
> >
> > Oh, I didn't know about this patchset and the new helper. Indeed, it
> > looks a lot like mine except there is no static verification of the map
> > type as I did with the arraymap of handles, and no batch mode either. I
> > think the return value of bpf_current_task_under_cgroup is error-prone
> > if an eBPF program do an "if(ret)" test on the value (because of the
> > negative ERRNO return value). Inverting the 0 and 1 return values should
> > fix this (0 == succeed, 1 == failed, <0 == error).
>
> nothing to fix. It's good as-is. Use if (ret > 0) instead.
>
> >
> > To sum up, there is four related patchsets:
> > * "Landlock LSM: Unprivileged sandboxing" (this series)
> > * "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
> > * "Networking cgroup controller" (Anoop Naravaram)
> > * "Add eBPF hooks for cgroups" (Daniel Mack)
> >
> > The three other series (Sargun's, Anoop's and Daniel's) are mainly
> > focused on network access-control via cgroup for *containers*. As far as
> > I can tell, only a *root* user (CAP_SYS_ADMIN) can use them. Landlock's
> > goal is to empower all processes (privileged or not) to create their own
> > sandbox. This also means, like explained in "[RFC v2 00/10] Landlock
> > LSM: Unprivileged sandboxing", there is more constraints. For example,
> > it is not acceptable to let a process probe the kernel memory as it
> > wish. More details are in the Landlock cover-letter.
> >
> >
> > Another important point is that supporting cgroup for Landlock is
> > optional. It does not rely on cgroup to be usable but is only a feature
> > available when (unprivileged) users can manage there own cgroup, which
> > is an important constraint. Put another way, Landlock should not rely on
> > cgroup to create sandboxes. Indeed, a process creating a sandbox do not
> > necessarily have access to the cgroup mount point (directly or not).
>
> cgroup is the common way to group multiple tasks.
> Without cgroup only parent<->child relationship will be possible,
> which will limit usability of such lsm to a master task that controls
> its children. Such api restriction would have been ok, if we could
> extend it in the future, but unfortunately task-centric won't allow it
> without creating a parallel lsm that is cgroup based.
> Therefore I think we have to go with cgroup-centric api and your
> application has to use cgroups from the start though only parent-child
> would have been enough.
> Also I don't think the kernel can afford two bpf based lsm. One task
> based and another cgroup based, so we have to find common ground
> that suits both use cases.
> Having unprivliged access is a subset. There is no strong reason why
> cgroup+lsm+bpf should be limited to root only always.
> When we can guarantee no pointer leaks, we can allow unpriv.

I don't really understand what you mean.  In the context of landlock,
which is a *sandbox*, can one of you explain a use case that
materially benefits from this type of cgroup usage?  I haven't thought
of one.

Re: [RFC v2 09/10] landlock: Handle cgroups

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 12:30:36AM -0700, Andy Lutomirski wrote:
> > cgroup is the common way to group multiple tasks.
> > Without cgroup only parent<->child relationship will be possible,
> > which will limit usability of such lsm to a master task that controls
> > its children. Such api restriction would have been ok, if we could
> > extend it in the future, but unfortunately task-centric won't allow it
> > without creating a parallel lsm that is cgroup based.
> > Therefore I think we have to go with cgroup-centric api and your
> > application has to use cgroups from the start though only parent-child
> > would have been enough.
> > Also I don't think the kernel can afford two bpf based lsm. One task
> > based and another cgroup based, so we have to find common ground
> > that suits both use cases.
> > Having unprivliged access is a subset. There is no strong reason why
> > cgroup+lsm+bpf should be limited to root only always.
> > When we can guarantee no pointer leaks, we can allow unpriv.
> 
> I don't really understand what you mean.  In the context of landlock,
> which is a *sandbox*, can one of you explain a use case that
> materially benefits from this type of cgroup usage?  I haven't thought
> of one.

In case of seccomp-like sandbox where parent controls child processes
cgroup is not needed. It's needed when container management software
needs to control a set of applications. If we can have one bpf-based lsm
that works via cgroup and without, I'd be fine with it. Right now
I haven't seen a plausible proposal to do that. Therefore cgroup based
api is a common api that works for sandbox as well, though requiring
parent to create a cgroup just to control a single child is cumbersome.

Re: [RFC v2 09/10] landlock: Handle cgroups

[Andy Lutomirski]

On Aug 27, 2016 8:12 PM, "Alexei Starovoitov"
<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Sat, Aug 27, 2016 at 12:30:36AM -0700, Andy Lutomirski wrote:
> > > cgroup is the common way to group multiple tasks.
> > > Without cgroup only parent<->child relationship will be possible,
> > > which will limit usability of such lsm to a master task that controls
> > > its children. Such api restriction would have been ok, if we could
> > > extend it in the future, but unfortunately task-centric won't allow it
> > > without creating a parallel lsm that is cgroup based.
> > > Therefore I think we have to go with cgroup-centric api and your
> > > application has to use cgroups from the start though only parent-child
> > > would have been enough.
> > > Also I don't think the kernel can afford two bpf based lsm. One task
> > > based and another cgroup based, so we have to find common ground
> > > that suits both use cases.
> > > Having unprivliged access is a subset. There is no strong reason why
> > > cgroup+lsm+bpf should be limited to root only always.
> > > When we can guarantee no pointer leaks, we can allow unpriv.
> >
> > I don't really understand what you mean.  In the context of landlock,
> > which is a *sandbox*, can one of you explain a use case that
> > materially benefits from this type of cgroup usage?  I haven't thought
> > of one.
>
> In case of seccomp-like sandbox where parent controls child processes
> cgroup is not needed. It's needed when container management software
> needs to control a set of applications. If we can have one bpf-based lsm
> that works via cgroup and without, I'd be fine with it. Right now
> I haven't seen a plausible proposal to do that. Therefore cgroup based
> api is a common api that works for sandbox as well, though requiring
> parent to create a cgroup just to control a single child is cumbersome.
>

I don't believe that a common API can work to accomplish your goal.
For privileged container management, the manager is trusted.  For
unprivileged sandboxing, the manager is emphatically not trusted,
which means you need special rules like NO_NEW_PRIVS, and, unless you
want to start restricting setuid and such in some cgroups, you really
do need a different interface for joining the sandbox than whatever
the container manager is using.

What could make sense is to have one BPF-based LSM that supports both
a seccomp-like unprivileged interface and a cgroup-based privileged
interface.  Most of the code for it is the BPF part anyway -- all that
the cgroup or seccomp part needs to do is to figure out which BPF
program(s) to call.

Also, for container management software, you don't really need
everything tied to cgroup -- you just need a way to cleanly add new
processes to the same security context.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 4081 bytes --]


On 27/08/2016 01:05, Alexei Starovoitov wrote:
> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>>
>>>
>>> - I don't think such 'for' loop can scale. The solution needs to work
>>> with thousands of containers and thousands of cgroups.
>>> In the patch 06/10 the proposal is to use 'current' as holder of
>>> the programs:
>>> +   for (prog = current->seccomp.landlock_prog;
>>> +                   prog; prog = prog->prev) {
>>> +           if (prog->filter == landlock_ret->filter) {
>>> +                   cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
>>> +                   break;
>>> +           }
>>> +   }
>>> imo that's the root of scalability issue.
>>> I think to be able to scale the bpf programs have to be attached to
>>> cgroups instead of tasks.
>>> That would be very different api. seccomp doesn't need to be touched.
>>> But that is the only way I see to be able to scale.
>>
>> Landlock is inspired from seccomp which also use a BPF program per
>> thread. For seccomp, each BPF programs are executed for each syscall.
>> For Landlock, some BPF programs are executed for some LSM hooks. I don't
>> see why it is a scale issue for Landlock comparing to seccomp. I also
>> don't see why storing the BPF program list pointer in the cgroup struct
>> instead of the task struct change a lot here. The BPF programs execution
>> will be the same anyway (for each LSM hook). Kees should probably have a
>> better opinion on this.
> 
> seccomp has its own issues and copying them doesn't make this lsm any better.
> Like seccomp bpf programs are all gigantic switch statement that looks
> for interesting syscall numbers. All syscalls of a task are paying
> non-trivial seccomp penalty due to such design. If bpf was attached per
> syscall it would have been much cheaper. Of course doing it this way
> for seccomp is not easy, but for lsm such facility is already there.
> Blank call of a single bpf prog for all lsm hooks is unnecessary
> overhead that can and should be avoided.

It's probably a misunderstanding. Contrary to seccomp which run all the
thread's BPF programs for any syscall, Landlock only run eBPF programs
for the triggered LSM hooks, if their type match. Indeed, thanks to the
multiple eBPF program types and contrary to seccomp, Landlock only run
an eBPF program when needed. Landlock will have almost no performance
overhead if the syscalls do not trigger the watched LSM hooks for the
current process.


> 
>>> May be another way of thinking about it is 'lsm cgroup controller'
>>> that Sargun is proposing.
>>> The lsm hooks will provide stable execution points and the programs
>>> will be called like:
>>> prog = task_css_set(current)->dfl_cgrp->bpf.prog_effective[lsm_hook_id];
>>> BPF_PROG_RUN(prog, ctx);
>>> The delegation functionality and 'prog_effective' logic that
>>> Daniel Mack is proposing will be fully reused here.
>>> External container management software will be able to apply bpf
>>> programs to control tasks under cgroup and such
>>> bpf_landlock_cmp_cgroup_beneath() helper won't be necessary.
>>> The user will be able to register different programs for different lsm hooks.
>>> If I understand the patch 6/10 correctly, there is one (or a list) prog for
>>> all lsm hooks per task which is not flexible enough.
>>
>> For each LSM hook triggered by a thread, all of its Landlock eBPF
>> programs (dedicated for this kind of hook) will be evaluated (if
>> needed). This is the same behavior as seccomp (list of BPF programs
>> attached to a process hierarchy) except the BPF programs are not
>> evaluated for syscall but for LSM hooks. There is no way to make it more
>> fine-grained :)
> 
> There is a way to attach different bpf program per cgroup
> and per lsm hook. Such approach drastically reduces overhead
> of sandboxed application.

As said above, Landlock will not run an eBPF programs when not strictly
needed. Attaching to a cgroup will have the same performance impact as
attaching to a process hierarchy.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>
> >>>
> >>> - I don't think such 'for' loop can scale. The solution needs to work
> >>> with thousands of containers and thousands of cgroups.
> >>> In the patch 06/10 the proposal is to use 'current' as holder of
> >>> the programs:
> >>> +   for (prog = current->seccomp.landlock_prog;
> >>> +                   prog; prog = prog->prev) {
> >>> +           if (prog->filter == landlock_ret->filter) {
> >>> +                   cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
> >>> +                   break;
> >>> +           }
> >>> +   }
> >>> imo that's the root of scalability issue.
> >>> I think to be able to scale the bpf programs have to be attached to
> >>> cgroups instead of tasks.
> >>> That would be very different api. seccomp doesn't need to be touched.
> >>> But that is the only way I see to be able to scale.
> >>
> >> Landlock is inspired from seccomp which also use a BPF program per
> >> thread. For seccomp, each BPF programs are executed for each syscall.
> >> For Landlock, some BPF programs are executed for some LSM hooks. I don't
> >> see why it is a scale issue for Landlock comparing to seccomp. I also
> >> don't see why storing the BPF program list pointer in the cgroup struct
> >> instead of the task struct change a lot here. The BPF programs execution
> >> will be the same anyway (for each LSM hook). Kees should probably have a
> >> better opinion on this.
> > 
> > seccomp has its own issues and copying them doesn't make this lsm any better.
> > Like seccomp bpf programs are all gigantic switch statement that looks
> > for interesting syscall numbers. All syscalls of a task are paying
> > non-trivial seccomp penalty due to such design. If bpf was attached per
> > syscall it would have been much cheaper. Of course doing it this way
> > for seccomp is not easy, but for lsm such facility is already there.
> > Blank call of a single bpf prog for all lsm hooks is unnecessary
> > overhead that can and should be avoided.
> 
> It's probably a misunderstanding. Contrary to seccomp which run all the
> thread's BPF programs for any syscall, Landlock only run eBPF programs
> for the triggered LSM hooks, if their type match. Indeed, thanks to the
> multiple eBPF program types and contrary to seccomp, Landlock only run
> an eBPF program when needed. Landlock will have almost no performance
> overhead if the syscalls do not trigger the watched LSM hooks for the
> current process.

that's not what I see in the patch 06/10:
all lsm_hooks in 'static struct security_hook_list landlock_hooks'
(which eventually means all lsm hooks) will call
static inline int landlock_hook_##NAME
which will call landlock_run_prog()
which does:
+ for (landlock_ret = current->seccomp.landlock_ret;
+      landlock_ret; landlock_ret = landlock_ret->prev) {
+    if (landlock_ret->triggered) {
+       ctx.cookie = landlock_ret->cookie;
+       for (prog = current->seccomp.landlock_prog;
+            prog; prog = prog->prev) {
+               if (prog->filter == landlock_ret->filter) {
+                       cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
+                       break;
+               }
+       }

that is unacceptable overhead and not a scalable design.
It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
as soon as more lsm hooks are added.

> As said above, Landlock will not run an eBPF programs when not strictly
> needed. Attaching to a cgroup will have the same performance impact as
> attaching to a process hierarchy.

Having a prog per cgroup per lsm_hook is the only scalable way I
could come up with. If you see another way, please propose.
current->seccomp.landlock_prog is not the answer.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 4801 bytes --]


On 27/08/2016 20:06, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 01:05, Alexei Starovoitov wrote:
>>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>>>>
>>>>>
>>>>> - I don't think such 'for' loop can scale. The solution needs to work
>>>>> with thousands of containers and thousands of cgroups.
>>>>> In the patch 06/10 the proposal is to use 'current' as holder of
>>>>> the programs:
>>>>> +   for (prog = current->seccomp.landlock_prog;
>>>>> +                   prog; prog = prog->prev) {
>>>>> +           if (prog->filter == landlock_ret->filter) {
>>>>> +                   cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
>>>>> +                   break;
>>>>> +           }
>>>>> +   }
>>>>> imo that's the root of scalability issue.
>>>>> I think to be able to scale the bpf programs have to be attached to
>>>>> cgroups instead of tasks.
>>>>> That would be very different api. seccomp doesn't need to be touched.
>>>>> But that is the only way I see to be able to scale.
>>>>
>>>> Landlock is inspired from seccomp which also use a BPF program per
>>>> thread. For seccomp, each BPF programs are executed for each syscall.
>>>> For Landlock, some BPF programs are executed for some LSM hooks. I don't
>>>> see why it is a scale issue for Landlock comparing to seccomp. I also
>>>> don't see why storing the BPF program list pointer in the cgroup struct
>>>> instead of the task struct change a lot here. The BPF programs execution
>>>> will be the same anyway (for each LSM hook). Kees should probably have a
>>>> better opinion on this.
>>>
>>> seccomp has its own issues and copying them doesn't make this lsm any better.
>>> Like seccomp bpf programs are all gigantic switch statement that looks
>>> for interesting syscall numbers. All syscalls of a task are paying
>>> non-trivial seccomp penalty due to such design. If bpf was attached per
>>> syscall it would have been much cheaper. Of course doing it this way
>>> for seccomp is not easy, but for lsm such facility is already there.
>>> Blank call of a single bpf prog for all lsm hooks is unnecessary
>>> overhead that can and should be avoided.
>>
>> It's probably a misunderstanding. Contrary to seccomp which run all the
>> thread's BPF programs for any syscall, Landlock only run eBPF programs
>> for the triggered LSM hooks, if their type match. Indeed, thanks to the
>> multiple eBPF program types and contrary to seccomp, Landlock only run
>> an eBPF program when needed. Landlock will have almost no performance
>> overhead if the syscalls do not trigger the watched LSM hooks for the
>> current process.
> 
> that's not what I see in the patch 06/10:
> all lsm_hooks in 'static struct security_hook_list landlock_hooks'
> (which eventually means all lsm hooks) will call
> static inline int landlock_hook_##NAME
> which will call landlock_run_prog()
> which does:
> + for (landlock_ret = current->seccomp.landlock_ret;
> +      landlock_ret; landlock_ret = landlock_ret->prev) {
> +    if (landlock_ret->triggered) {
> +       ctx.cookie = landlock_ret->cookie;
> +       for (prog = current->seccomp.landlock_prog;
> +            prog; prog = prog->prev) {
> +               if (prog->filter == landlock_ret->filter) {
> +                       cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
> +                       break;
> +               }
> +       }
> 
> that is unacceptable overhead and not a scalable design.
> It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
> as soon as more lsm hooks are added.

Good catch! I forgot to check the program (sub)type in the loop to only
run the needed programs for the current hook. I will fix this.


> 
>> As said above, Landlock will not run an eBPF programs when not strictly
>> needed. Attaching to a cgroup will have the same performance impact as
>> attaching to a process hierarchy.
> 
> Having a prog per cgroup per lsm_hook is the only scalable way I
> could come up with. If you see another way, please propose.
> current->seccomp.landlock_prog is not the answer.

Hum, I don't see the difference from a performance point of view between
a cgroup-based or a process hierarchy-based system.

Maybe a better option should be to use an array of pointers with N
entries, one for each supported hook, instead of a unique pointer list?

Anyway, being able to attach an LSM hook program to a cgroup thanks to
the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
to use a process hierarchy). The downside will be to handle an LSM hook
program which is not triggered by a seccomp-filter, but this should be
needed anyway to handle interruptions.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 20:06, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> >>
> >> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>>>
> >>>>>
> >>>>> - I don't think such 'for' loop can scale. The solution needs to work
> >>>>> with thousands of containers and thousands of cgroups.
> >>>>> In the patch 06/10 the proposal is to use 'current' as holder of
> >>>>> the programs:
> >>>>> +   for (prog = current->seccomp.landlock_prog;
> >>>>> +                   prog; prog = prog->prev) {
> >>>>> +           if (prog->filter == landlock_ret->filter) {
> >>>>> +                   cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
> >>>>> +                   break;
> >>>>> +           }
> >>>>> +   }
> >>>>> imo that's the root of scalability issue.
> >>>>> I think to be able to scale the bpf programs have to be attached to
> >>>>> cgroups instead of tasks.
> >>>>> That would be very different api. seccomp doesn't need to be touched.
> >>>>> But that is the only way I see to be able to scale.
> >>>>
> >>>> Landlock is inspired from seccomp which also use a BPF program per
> >>>> thread. For seccomp, each BPF programs are executed for each syscall.
> >>>> For Landlock, some BPF programs are executed for some LSM hooks. I don't
> >>>> see why it is a scale issue for Landlock comparing to seccomp. I also
> >>>> don't see why storing the BPF program list pointer in the cgroup struct
> >>>> instead of the task struct change a lot here. The BPF programs execution
> >>>> will be the same anyway (for each LSM hook). Kees should probably have a
> >>>> better opinion on this.
> >>>
> >>> seccomp has its own issues and copying them doesn't make this lsm any better.
> >>> Like seccomp bpf programs are all gigantic switch statement that looks
> >>> for interesting syscall numbers. All syscalls of a task are paying
> >>> non-trivial seccomp penalty due to such design. If bpf was attached per
> >>> syscall it would have been much cheaper. Of course doing it this way
> >>> for seccomp is not easy, but for lsm such facility is already there.
> >>> Blank call of a single bpf prog for all lsm hooks is unnecessary
> >>> overhead that can and should be avoided.
> >>
> >> It's probably a misunderstanding. Contrary to seccomp which run all the
> >> thread's BPF programs for any syscall, Landlock only run eBPF programs
> >> for the triggered LSM hooks, if their type match. Indeed, thanks to the
> >> multiple eBPF program types and contrary to seccomp, Landlock only run
> >> an eBPF program when needed. Landlock will have almost no performance
> >> overhead if the syscalls do not trigger the watched LSM hooks for the
> >> current process.
> > 
> > that's not what I see in the patch 06/10:
> > all lsm_hooks in 'static struct security_hook_list landlock_hooks'
> > (which eventually means all lsm hooks) will call
> > static inline int landlock_hook_##NAME
> > which will call landlock_run_prog()
> > which does:
> > + for (landlock_ret = current->seccomp.landlock_ret;
> > +      landlock_ret; landlock_ret = landlock_ret->prev) {
> > +    if (landlock_ret->triggered) {
> > +       ctx.cookie = landlock_ret->cookie;
> > +       for (prog = current->seccomp.landlock_prog;
> > +            prog; prog = prog->prev) {
> > +               if (prog->filter == landlock_ret->filter) {
> > +                       cur_ret = BPF_PROG_RUN(prog->prog, (void *)&ctx);
> > +                       break;
> > +               }
> > +       }
> > 
> > that is unacceptable overhead and not a scalable design.
> > It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
> > as soon as more lsm hooks are added.
> 
> Good catch! I forgot to check the program (sub)type in the loop to only
> run the needed programs for the current hook. I will fix this.
> 
> 
> > 
> >> As said above, Landlock will not run an eBPF programs when not strictly
> >> needed. Attaching to a cgroup will have the same performance impact as
> >> attaching to a process hierarchy.
> > 
> > Having a prog per cgroup per lsm_hook is the only scalable way I
> > could come up with. If you see another way, please propose.
> > current->seccomp.landlock_prog is not the answer.
> 
> Hum, I don't see the difference from a performance point of view between
> a cgroup-based or a process hierarchy-based system.
> 
> Maybe a better option should be to use an array of pointers with N
> entries, one for each supported hook, instead of a unique pointer list?

yes, clearly array dereference is faster than link list walk.
Now the question is where to keep this prog_array[num_lsm_hooks] ?
Since we cannot keep it inside task_struct, we have to allocate it.
Every time the task is creted then. What to do on the fork? That
will require changes all over. Then the obvious optimization would be
to share this allocated array of prog pointers across multiple tasks...
and little by little this new facility will look like cgroup.
Hence the suggestion to put this array into cgroup from the start.

> Anyway, being able to attach an LSM hook program to a cgroup thanks to
> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
> to use a process hierarchy). The downside will be to handle an LSM hook
> program which is not triggered by a seccomp-filter, but this should be
> needed anyway to handle interruptions.

what do you mean 'not triggered by seccomp' ?
You're not suggesting that this lsm has to enable seccomp to be functional?
imo that's non starter due to overhead.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 2300 bytes --]


On 27/08/2016 22:43, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>>> As said above, Landlock will not run an eBPF programs when not strictly
>>>> needed. Attaching to a cgroup will have the same performance impact as
>>>> attaching to a process hierarchy.
>>>
>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>> could come up with. If you see another way, please propose.
>>> current->seccomp.landlock_prog is not the answer.
>>
>> Hum, I don't see the difference from a performance point of view between
>> a cgroup-based or a process hierarchy-based system.
>>
>> Maybe a better option should be to use an array of pointers with N
>> entries, one for each supported hook, instead of a unique pointer list?
> 
> yes, clearly array dereference is faster than link list walk.
> Now the question is where to keep this prog_array[num_lsm_hooks] ?
> Since we cannot keep it inside task_struct, we have to allocate it.
> Every time the task is creted then. What to do on the fork? That
> will require changes all over. Then the obvious optimization would be
> to share this allocated array of prog pointers across multiple tasks...
> and little by little this new facility will look like cgroup.
> Hence the suggestion to put this array into cgroup from the start.

I see your point :)

> 
>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>> to use a process hierarchy). The downside will be to handle an LSM hook
>> program which is not triggered by a seccomp-filter, but this should be
>> needed anyway to handle interruptions.
> 
> what do you mean 'not triggered by seccomp' ?
> You're not suggesting that this lsm has to enable seccomp to be functional?
> imo that's non starter due to overhead.

Yes, for now, it is triggered by a new seccomp filter return value
RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
be needed but could be useful to bind a seccomp filter security policy
with a Landlock one. Waiting for Kees's point of view…


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Andy Lutomirski]

On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic@digikod.net> wrote:
>
>
> On 27/08/2016 22:43, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
> >> On 27/08/2016 20:06, Alexei Starovoitov wrote:
> >>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> >>>> As said above, Landlock will not run an eBPF programs when not strictly
> >>>> needed. Attaching to a cgroup will have the same performance impact as
> >>>> attaching to a process hierarchy.
> >>>
> >>> Having a prog per cgroup per lsm_hook is the only scalable way I
> >>> could come up with. If you see another way, please propose.
> >>> current->seccomp.landlock_prog is not the answer.
> >>
> >> Hum, I don't see the difference from a performance point of view between
> >> a cgroup-based or a process hierarchy-based system.
> >>
> >> Maybe a better option should be to use an array of pointers with N
> >> entries, one for each supported hook, instead of a unique pointer list?
> >
> > yes, clearly array dereference is faster than link list walk.
> > Now the question is where to keep this prog_array[num_lsm_hooks] ?
> > Since we cannot keep it inside task_struct, we have to allocate it.
> > Every time the task is creted then. What to do on the fork? That
> > will require changes all over. Then the obvious optimization would be
> > to share this allocated array of prog pointers across multiple tasks...
> > and little by little this new facility will look like cgroup.
> > Hence the suggestion to put this array into cgroup from the start.
>
> I see your point :)
>
> >
> >> Anyway, being able to attach an LSM hook program to a cgroup thanks to
> >> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
> >> to use a process hierarchy). The downside will be to handle an LSM hook
> >> program which is not triggered by a seccomp-filter, but this should be
> >> needed anyway to handle interruptions.
> >
> > what do you mean 'not triggered by seccomp' ?
> > You're not suggesting that this lsm has to enable seccomp to be functional?
> > imo that's non starter due to overhead.
>
> Yes, for now, it is triggered by a new seccomp filter return value
> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
> be needed but could be useful to bind a seccomp filter security policy
> with a Landlock one. Waiting for Kees's point of view…
>

I'm not Kees, but I'd be okay with that.  I still think that doing
this by process hierarchy a la seccomp will be easier to use and to
understand (which is quite important for this kind of work) than doing
it by cgroup.

A feature I've wanted to add for a while is to have an fd that
represents a seccomp layer, the idea being that you would set up your
seccomp layer (with syscall filter, landlock hooks, etc) and then you
would have a syscall to install that layer.  Then an unprivileged
sandbox manager could set up its layer and still be able to inject new
processes into it later on, no cgroups needed.

--Andy

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 4048 bytes --]



On 28/08/2016 10:13, Andy Lutomirski wrote:
> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic@digikod.net> wrote:
>>
>>
>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
>>>>>> needed. Attaching to a cgroup will have the same performance impact as
>>>>>> attaching to a process hierarchy.
>>>>>
>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>>>> could come up with. If you see another way, please propose.
>>>>> current->seccomp.landlock_prog is not the answer.
>>>>
>>>> Hum, I don't see the difference from a performance point of view between
>>>> a cgroup-based or a process hierarchy-based system.
>>>>
>>>> Maybe a better option should be to use an array of pointers with N
>>>> entries, one for each supported hook, instead of a unique pointer list?
>>>
>>> yes, clearly array dereference is faster than link list walk.
>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
>>> Since we cannot keep it inside task_struct, we have to allocate it.
>>> Every time the task is creted then. What to do on the fork? That
>>> will require changes all over. Then the obvious optimization would be
>>> to share this allocated array of prog pointers across multiple tasks...
>>> and little by little this new facility will look like cgroup.
>>> Hence the suggestion to put this array into cgroup from the start.
>>
>> I see your point :)
>>
>>>
>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>>>> to use a process hierarchy). The downside will be to handle an LSM hook
>>>> program which is not triggered by a seccomp-filter, but this should be
>>>> needed anyway to handle interruptions.
>>>
>>> what do you mean 'not triggered by seccomp' ?
>>> You're not suggesting that this lsm has to enable seccomp to be functional?
>>> imo that's non starter due to overhead.
>>
>> Yes, for now, it is triggered by a new seccomp filter return value
>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
>> be needed but could be useful to bind a seccomp filter security policy
>> with a Landlock one. Waiting for Kees's point of view…
>>
> 
> I'm not Kees, but I'd be okay with that.  I still think that doing
> this by process hierarchy a la seccomp will be easier to use and to
> understand (which is quite important for this kind of work) than doing
> it by cgroup.
> 
> A feature I've wanted to add for a while is to have an fd that
> represents a seccomp layer, the idea being that you would set up your
> seccomp layer (with syscall filter, landlock hooks, etc) and then you
> would have a syscall to install that layer.  Then an unprivileged
> sandbox manager could set up its layer and still be able to inject new
> processes into it later on, no cgroups needed.

A nice thing I didn't highlight about Landlock is that a process can
prepare a layer of rules (arraymap of handles + Landlock programs) and
pass the file descriptors of the Landlock programs to another process.
This process could then apply this programs to get sandboxed. However,
for now, because a Landlock program is only triggered by a seccomp
filter (which do not follow the Landlock programs as a FD), they will be
useless.

The FD referring to an arraymap of handles can also be used to update a
map and change the behavior of a Landlock program. A master process can
then add or remove restrictions to another process hierarchy on the fly.

However, I think it would make more sense to use cgroups if we want to
move an existing (unwilling) unsandoxed process into a sandboxed
environment. Of course, some more no_new_privs checks would be needed.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Andy Lutomirski]

On Sun, Aug 28, 2016 at 2:42 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 28/08/2016 10:13, Andy Lutomirski wrote:
>> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic@digikod.net> wrote:
>>>
>>>
>>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
>>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
>>>>>>> needed. Attaching to a cgroup will have the same performance impact as
>>>>>>> attaching to a process hierarchy.
>>>>>>
>>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>>>>> could come up with. If you see another way, please propose.
>>>>>> current->seccomp.landlock_prog is not the answer.
>>>>>
>>>>> Hum, I don't see the difference from a performance point of view between
>>>>> a cgroup-based or a process hierarchy-based system.
>>>>>
>>>>> Maybe a better option should be to use an array of pointers with N
>>>>> entries, one for each supported hook, instead of a unique pointer list?
>>>>
>>>> yes, clearly array dereference is faster than link list walk.
>>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
>>>> Since we cannot keep it inside task_struct, we have to allocate it.
>>>> Every time the task is creted then. What to do on the fork? That
>>>> will require changes all over. Then the obvious optimization would be
>>>> to share this allocated array of prog pointers across multiple tasks...
>>>> and little by little this new facility will look like cgroup.
>>>> Hence the suggestion to put this array into cgroup from the start.
>>>
>>> I see your point :)
>>>
>>>>
>>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>>>>> to use a process hierarchy). The downside will be to handle an LSM hook
>>>>> program which is not triggered by a seccomp-filter, but this should be
>>>>> needed anyway to handle interruptions.
>>>>
>>>> what do you mean 'not triggered by seccomp' ?
>>>> You're not suggesting that this lsm has to enable seccomp to be functional?
>>>> imo that's non starter due to overhead.
>>>
>>> Yes, for now, it is triggered by a new seccomp filter return value
>>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
>>> be needed but could be useful to bind a seccomp filter security policy
>>> with a Landlock one. Waiting for Kees's point of view…
>>>
>>
>> I'm not Kees, but I'd be okay with that.  I still think that doing
>> this by process hierarchy a la seccomp will be easier to use and to
>> understand (which is quite important for this kind of work) than doing
>> it by cgroup.
>>
>> A feature I've wanted to add for a while is to have an fd that
>> represents a seccomp layer, the idea being that you would set up your
>> seccomp layer (with syscall filter, landlock hooks, etc) and then you
>> would have a syscall to install that layer.  Then an unprivileged
>> sandbox manager could set up its layer and still be able to inject new
>> processes into it later on, no cgroups needed.
>
> A nice thing I didn't highlight about Landlock is that a process can
> prepare a layer of rules (arraymap of handles + Landlock programs) and
> pass the file descriptors of the Landlock programs to another process.
> This process could then apply this programs to get sandboxed. However,
> for now, because a Landlock program is only triggered by a seccomp
> filter (which do not follow the Landlock programs as a FD), they will be
> useless.
>
> The FD referring to an arraymap of handles can also be used to update a
> map and change the behavior of a Landlock program. A master process can
> then add or remove restrictions to another process hierarchy on the fly.

Maybe this could be extended a little bit.  The fd could hold the
seccomp filter *and* the LSM hook filters.  FMODE_EXECUTE could give
the ability to install it and FMODE_WRITE could give the ability to
modify it.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 4687 bytes --]


On 30/08/2016 20:55, Andy Lutomirski wrote:
> On Sun, Aug 28, 2016 at 2:42 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 28/08/2016 10:13, Andy Lutomirski wrote:
>>> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic@digikod.net> wrote:
>>>>
>>>>
>>>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
>>>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>>>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
>>>>>>>> needed. Attaching to a cgroup will have the same performance impact as
>>>>>>>> attaching to a process hierarchy.
>>>>>>>
>>>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>>>>>> could come up with. If you see another way, please propose.
>>>>>>> current->seccomp.landlock_prog is not the answer.
>>>>>>
>>>>>> Hum, I don't see the difference from a performance point of view between
>>>>>> a cgroup-based or a process hierarchy-based system.
>>>>>>
>>>>>> Maybe a better option should be to use an array of pointers with N
>>>>>> entries, one for each supported hook, instead of a unique pointer list?
>>>>>
>>>>> yes, clearly array dereference is faster than link list walk.
>>>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
>>>>> Since we cannot keep it inside task_struct, we have to allocate it.
>>>>> Every time the task is creted then. What to do on the fork? That
>>>>> will require changes all over. Then the obvious optimization would be
>>>>> to share this allocated array of prog pointers across multiple tasks...
>>>>> and little by little this new facility will look like cgroup.
>>>>> Hence the suggestion to put this array into cgroup from the start.
>>>>
>>>> I see your point :)
>>>>
>>>>>
>>>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>>>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>>>>>> to use a process hierarchy). The downside will be to handle an LSM hook
>>>>>> program which is not triggered by a seccomp-filter, but this should be
>>>>>> needed anyway to handle interruptions.
>>>>>
>>>>> what do you mean 'not triggered by seccomp' ?
>>>>> You're not suggesting that this lsm has to enable seccomp to be functional?
>>>>> imo that's non starter due to overhead.
>>>>
>>>> Yes, for now, it is triggered by a new seccomp filter return value
>>>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
>>>> be needed but could be useful to bind a seccomp filter security policy
>>>> with a Landlock one. Waiting for Kees's point of view…
>>>>
>>>
>>> I'm not Kees, but I'd be okay with that.  I still think that doing
>>> this by process hierarchy a la seccomp will be easier to use and to
>>> understand (which is quite important for this kind of work) than doing
>>> it by cgroup.
>>>
>>> A feature I've wanted to add for a while is to have an fd that
>>> represents a seccomp layer, the idea being that you would set up your
>>> seccomp layer (with syscall filter, landlock hooks, etc) and then you
>>> would have a syscall to install that layer.  Then an unprivileged
>>> sandbox manager could set up its layer and still be able to inject new
>>> processes into it later on, no cgroups needed.
>>
>> A nice thing I didn't highlight about Landlock is that a process can
>> prepare a layer of rules (arraymap of handles + Landlock programs) and
>> pass the file descriptors of the Landlock programs to another process.
>> This process could then apply this programs to get sandboxed. However,
>> for now, because a Landlock program is only triggered by a seccomp
>> filter (which do not follow the Landlock programs as a FD), they will be
>> useless.
>>
>> The FD referring to an arraymap of handles can also be used to update a
>> map and change the behavior of a Landlock program. A master process can
>> then add or remove restrictions to another process hierarchy on the fly.
> 
> Maybe this could be extended a little bit.  The fd could hold the
> seccomp filter *and* the LSM hook filters.  FMODE_EXECUTE could give
> the ability to install it and FMODE_WRITE could give the ability to
> modify it.
> 

This is interesting! It should be possible to append the seccomp stack
of a source process to the seccomp stack of the target process when a
Landlock program is passed and then activated through seccomp(2).

For the FMODE_EXECUTE/FMODE_WRITE, are you suggesting to manage
permission of the eBPF program FD in a specific way?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Andy Lutomirski]

On Tue, Aug 30, 2016 at 1:20 PM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
>
> On 30/08/2016 20:55, Andy Lutomirski wrote:
>> On Sun, Aug 28, 2016 at 2:42 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>>
>>> On 28/08/2016 10:13, Andy Lutomirski wrote:
>>>> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
>>>>>
>>>>>
>>>>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
>>>>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>>>>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>>>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>>>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
>>>>>>>>> needed. Attaching to a cgroup will have the same performance impact as
>>>>>>>>> attaching to a process hierarchy.
>>>>>>>>
>>>>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>>>>>>> could come up with. If you see another way, please propose.
>>>>>>>> current->seccomp.landlock_prog is not the answer.
>>>>>>>
>>>>>>> Hum, I don't see the difference from a performance point of view between
>>>>>>> a cgroup-based or a process hierarchy-based system.
>>>>>>>
>>>>>>> Maybe a better option should be to use an array of pointers with N
>>>>>>> entries, one for each supported hook, instead of a unique pointer list?
>>>>>>
>>>>>> yes, clearly array dereference is faster than link list walk.
>>>>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
>>>>>> Since we cannot keep it inside task_struct, we have to allocate it.
>>>>>> Every time the task is creted then. What to do on the fork? That
>>>>>> will require changes all over. Then the obvious optimization would be
>>>>>> to share this allocated array of prog pointers across multiple tasks...
>>>>>> and little by little this new facility will look like cgroup.
>>>>>> Hence the suggestion to put this array into cgroup from the start.
>>>>>
>>>>> I see your point :)
>>>>>
>>>>>>
>>>>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>>>>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>>>>>>> to use a process hierarchy). The downside will be to handle an LSM hook
>>>>>>> program which is not triggered by a seccomp-filter, but this should be
>>>>>>> needed anyway to handle interruptions.
>>>>>>
>>>>>> what do you mean 'not triggered by seccomp' ?
>>>>>> You're not suggesting that this lsm has to enable seccomp to be functional?
>>>>>> imo that's non starter due to overhead.
>>>>>
>>>>> Yes, for now, it is triggered by a new seccomp filter return value
>>>>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
>>>>> be needed but could be useful to bind a seccomp filter security policy
>>>>> with a Landlock one. Waiting for Kees's point of view…
>>>>>
>>>>
>>>> I'm not Kees, but I'd be okay with that.  I still think that doing
>>>> this by process hierarchy a la seccomp will be easier to use and to
>>>> understand (which is quite important for this kind of work) than doing
>>>> it by cgroup.
>>>>
>>>> A feature I've wanted to add for a while is to have an fd that
>>>> represents a seccomp layer, the idea being that you would set up your
>>>> seccomp layer (with syscall filter, landlock hooks, etc) and then you
>>>> would have a syscall to install that layer.  Then an unprivileged
>>>> sandbox manager could set up its layer and still be able to inject new
>>>> processes into it later on, no cgroups needed.
>>>
>>> A nice thing I didn't highlight about Landlock is that a process can
>>> prepare a layer of rules (arraymap of handles + Landlock programs) and
>>> pass the file descriptors of the Landlock programs to another process.
>>> This process could then apply this programs to get sandboxed. However,
>>> for now, because a Landlock program is only triggered by a seccomp
>>> filter (which do not follow the Landlock programs as a FD), they will be
>>> useless.
>>>
>>> The FD referring to an arraymap of handles can also be used to update a
>>> map and change the behavior of a Landlock program. A master process can
>>> then add or remove restrictions to another process hierarchy on the fly.
>>
>> Maybe this could be extended a little bit.  The fd could hold the
>> seccomp filter *and* the LSM hook filters.  FMODE_EXECUTE could give
>> the ability to install it and FMODE_WRITE could give the ability to
>> modify it.
>>
>
> This is interesting! It should be possible to append the seccomp stack
> of a source process to the seccomp stack of the target process when a
> Landlock program is passed and then activated through seccomp(2).
>
> For the FMODE_EXECUTE/FMODE_WRITE, are you suggesting to manage
> permission of the eBPF program FD in a specific way?
>

This wouldn't be an eBPF program FD -- it would be an FD encapsulating
an entire configuration including seccomp BPF program, whatever
landlock stuff is associated, and eventual seccomp monitor
configuration (once I write that code), etc.

You wouldn't say "attach this process's seccomp stack to me" -- you'd
say "attach this seccomp layer to me".

A decision that we'd have to make would be whether the FD links to the
parent layer or whether it can be attached without regard to what the
parent layer is.

--Andy

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 5619 bytes --]



On 30/08/2016 22:23, Andy Lutomirski wrote:
> On Tue, Aug 30, 2016 at 1:20 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 30/08/2016 20:55, Andy Lutomirski wrote:
>>> On Sun, Aug 28, 2016 at 2:42 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>>
>>>> On 28/08/2016 10:13, Andy Lutomirski wrote:
>>>>> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic@digikod.net> wrote:
>>>>>>
>>>>>>
>>>>>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
>>>>>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>>>>>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>>>>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>>>>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
>>>>>>>>>> needed. Attaching to a cgroup will have the same performance impact as
>>>>>>>>>> attaching to a process hierarchy.
>>>>>>>>>
>>>>>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>>>>>>>> could come up with. If you see another way, please propose.
>>>>>>>>> current->seccomp.landlock_prog is not the answer.
>>>>>>>>
>>>>>>>> Hum, I don't see the difference from a performance point of view between
>>>>>>>> a cgroup-based or a process hierarchy-based system.
>>>>>>>>
>>>>>>>> Maybe a better option should be to use an array of pointers with N
>>>>>>>> entries, one for each supported hook, instead of a unique pointer list?
>>>>>>>
>>>>>>> yes, clearly array dereference is faster than link list walk.
>>>>>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
>>>>>>> Since we cannot keep it inside task_struct, we have to allocate it.
>>>>>>> Every time the task is creted then. What to do on the fork? That
>>>>>>> will require changes all over. Then the obvious optimization would be
>>>>>>> to share this allocated array of prog pointers across multiple tasks...
>>>>>>> and little by little this new facility will look like cgroup.
>>>>>>> Hence the suggestion to put this array into cgroup from the start.
>>>>>>
>>>>>> I see your point :)
>>>>>>
>>>>>>>
>>>>>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>>>>>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>>>>>>>> to use a process hierarchy). The downside will be to handle an LSM hook
>>>>>>>> program which is not triggered by a seccomp-filter, but this should be
>>>>>>>> needed anyway to handle interruptions.
>>>>>>>
>>>>>>> what do you mean 'not triggered by seccomp' ?
>>>>>>> You're not suggesting that this lsm has to enable seccomp to be functional?
>>>>>>> imo that's non starter due to overhead.
>>>>>>
>>>>>> Yes, for now, it is triggered by a new seccomp filter return value
>>>>>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
>>>>>> be needed but could be useful to bind a seccomp filter security policy
>>>>>> with a Landlock one. Waiting for Kees's point of view…
>>>>>>
>>>>>
>>>>> I'm not Kees, but I'd be okay with that.  I still think that doing
>>>>> this by process hierarchy a la seccomp will be easier to use and to
>>>>> understand (which is quite important for this kind of work) than doing
>>>>> it by cgroup.
>>>>>
>>>>> A feature I've wanted to add for a while is to have an fd that
>>>>> represents a seccomp layer, the idea being that you would set up your
>>>>> seccomp layer (with syscall filter, landlock hooks, etc) and then you
>>>>> would have a syscall to install that layer.  Then an unprivileged
>>>>> sandbox manager could set up its layer and still be able to inject new
>>>>> processes into it later on, no cgroups needed.
>>>>
>>>> A nice thing I didn't highlight about Landlock is that a process can
>>>> prepare a layer of rules (arraymap of handles + Landlock programs) and
>>>> pass the file descriptors of the Landlock programs to another process.
>>>> This process could then apply this programs to get sandboxed. However,
>>>> for now, because a Landlock program is only triggered by a seccomp
>>>> filter (which do not follow the Landlock programs as a FD), they will be
>>>> useless.
>>>>
>>>> The FD referring to an arraymap of handles can also be used to update a
>>>> map and change the behavior of a Landlock program. A master process can
>>>> then add or remove restrictions to another process hierarchy on the fly.
>>>
>>> Maybe this could be extended a little bit.  The fd could hold the
>>> seccomp filter *and* the LSM hook filters.  FMODE_EXECUTE could give
>>> the ability to install it and FMODE_WRITE could give the ability to
>>> modify it.
>>>
>>
>> This is interesting! It should be possible to append the seccomp stack
>> of a source process to the seccomp stack of the target process when a
>> Landlock program is passed and then activated through seccomp(2).
>>
>> For the FMODE_EXECUTE/FMODE_WRITE, are you suggesting to manage
>> permission of the eBPF program FD in a specific way?
>>
> 
> This wouldn't be an eBPF program FD -- it would be an FD encapsulating
> an entire configuration including seccomp BPF program, whatever
> landlock stuff is associated, and eventual seccomp monitor
> configuration (once I write that code), etc.
> 
> You wouldn't say "attach this process's seccomp stack to me" -- you'd
> say "attach this seccomp layer to me".
> 
> A decision that we'd have to make would be whether the FD links to the
> parent layer or whether it can be attached without regard to what the
> parent layer is.

OK, I like that, but I think it could be done on a second time. :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Tue, Aug 30, 2016 at 10:33:31PM +0200, Mickaël Salaün wrote:
> 
> 
> On 30/08/2016 22:23, Andy Lutomirski wrote:
> > On Tue, Aug 30, 2016 at 1:20 PM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
> >>
> >> On 30/08/2016 20:55, Andy Lutomirski wrote:
> >>> On Sun, Aug 28, 2016 at 2:42 AM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
> >>>>
> >>>>
> >>>> On 28/08/2016 10:13, Andy Lutomirski wrote:
> >>>>> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
> >>>>>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
> >>>>>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
> >>>>>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> >>>>>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
> >>>>>>>>>> needed. Attaching to a cgroup will have the same performance impact as
> >>>>>>>>>> attaching to a process hierarchy.
> >>>>>>>>>
> >>>>>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
> >>>>>>>>> could come up with. If you see another way, please propose.
> >>>>>>>>> current->seccomp.landlock_prog is not the answer.
> >>>>>>>>
> >>>>>>>> Hum, I don't see the difference from a performance point of view between
> >>>>>>>> a cgroup-based or a process hierarchy-based system.
> >>>>>>>>
> >>>>>>>> Maybe a better option should be to use an array of pointers with N
> >>>>>>>> entries, one for each supported hook, instead of a unique pointer list?
> >>>>>>>
> >>>>>>> yes, clearly array dereference is faster than link list walk.
> >>>>>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
> >>>>>>> Since we cannot keep it inside task_struct, we have to allocate it.
> >>>>>>> Every time the task is creted then. What to do on the fork? That
> >>>>>>> will require changes all over. Then the obvious optimization would be
> >>>>>>> to share this allocated array of prog pointers across multiple tasks...
> >>>>>>> and little by little this new facility will look like cgroup.
> >>>>>>> Hence the suggestion to put this array into cgroup from the start.
> >>>>>>
> >>>>>> I see your point :)
> >>>>>>
> >>>>>>>
> >>>>>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
> >>>>>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
> >>>>>>>> to use a process hierarchy). The downside will be to handle an LSM hook
> >>>>>>>> program which is not triggered by a seccomp-filter, but this should be
> >>>>>>>> needed anyway to handle interruptions.
> >>>>>>>
> >>>>>>> what do you mean 'not triggered by seccomp' ?
> >>>>>>> You're not suggesting that this lsm has to enable seccomp to be functional?
> >>>>>>> imo that's non starter due to overhead.
> >>>>>>
> >>>>>> Yes, for now, it is triggered by a new seccomp filter return value
> >>>>>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
> >>>>>> be needed but could be useful to bind a seccomp filter security policy
> >>>>>> with a Landlock one. Waiting for Kees's point of view…
> >>>>>>
> >>>>>
> >>>>> I'm not Kees, but I'd be okay with that.  I still think that doing
> >>>>> this by process hierarchy a la seccomp will be easier to use and to
> >>>>> understand (which is quite important for this kind of work) than doing
> >>>>> it by cgroup.
> >>>>>
> >>>>> A feature I've wanted to add for a while is to have an fd that
> >>>>> represents a seccomp layer, the idea being that you would set up your
> >>>>> seccomp layer (with syscall filter, landlock hooks, etc) and then you
> >>>>> would have a syscall to install that layer.  Then an unprivileged
> >>>>> sandbox manager could set up its layer and still be able to inject new
> >>>>> processes into it later on, no cgroups needed.
> >>>>
> >>>> A nice thing I didn't highlight about Landlock is that a process can
> >>>> prepare a layer of rules (arraymap of handles + Landlock programs) and
> >>>> pass the file descriptors of the Landlock programs to another process.
> >>>> This process could then apply this programs to get sandboxed. However,
> >>>> for now, because a Landlock program is only triggered by a seccomp
> >>>> filter (which do not follow the Landlock programs as a FD), they will be
> >>>> useless.
> >>>>
> >>>> The FD referring to an arraymap of handles can also be used to update a
> >>>> map and change the behavior of a Landlock program. A master process can
> >>>> then add or remove restrictions to another process hierarchy on the fly.
> >>>
> >>> Maybe this could be extended a little bit.  The fd could hold the
> >>> seccomp filter *and* the LSM hook filters.  FMODE_EXECUTE could give
> >>> the ability to install it and FMODE_WRITE could give the ability to
> >>> modify it.
> >>>
> >>
> >> This is interesting! It should be possible to append the seccomp stack
> >> of a source process to the seccomp stack of the target process when a
> >> Landlock program is passed and then activated through seccomp(2).
> >>
> >> For the FMODE_EXECUTE/FMODE_WRITE, are you suggesting to manage
> >> permission of the eBPF program FD in a specific way?
> >>
> > 
> > This wouldn't be an eBPF program FD -- it would be an FD encapsulating
> > an entire configuration including seccomp BPF program, whatever
> > landlock stuff is associated, and eventual seccomp monitor
> > configuration (once I write that code), etc.
> > 
> > You wouldn't say "attach this process's seccomp stack to me" -- you'd
> > say "attach this seccomp layer to me".
> > 
> > A decision that we'd have to make would be whether the FD links to the
> > parent layer or whether it can be attached without regard to what the
> > parent layer is.
> 
> OK, I like that, but I think it could be done on a second time. :)

I don't. Single FD that is a collection of objects seems an odd abstraction
to me. I also don't see what it actually solves.
I think lsm and seccomp should be orthogonal and not tied into each other.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Andy Lutomirski]

On Aug 30, 2016 1:56 PM, "Alexei Starovoitov"
<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Tue, Aug 30, 2016 at 10:33:31PM +0200, Mickaël Salaün wrote:
> >
> >
> > On 30/08/2016 22:23, Andy Lutomirski wrote:
> > > On Tue, Aug 30, 2016 at 1:20 PM, Mickaël Salaün <mic@digikod.net> wrote:
> > >>
> > >> On 30/08/2016 20:55, Andy Lutomirski wrote:
> > >>> On Sun, Aug 28, 2016 at 2:42 AM, Mickaël Salaün <mic@digikod.net> wrote:
> > >>>>
> > >>>>
> > >>>> On 28/08/2016 10:13, Andy Lutomirski wrote:
> > >>>>> On Aug 27, 2016 11:14 PM, "Mickaël Salaün" <mic@digikod.net> wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>> On 27/08/2016 22:43, Alexei Starovoitov wrote:
> > >>>>>>> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
> > >>>>>>>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
> > >>>>>>>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> > >>>>>>>>>> As said above, Landlock will not run an eBPF programs when not strictly
> > >>>>>>>>>> needed. Attaching to a cgroup will have the same performance impact as
> > >>>>>>>>>> attaching to a process hierarchy.
> > >>>>>>>>>
> > >>>>>>>>> Having a prog per cgroup per lsm_hook is the only scalable way I
> > >>>>>>>>> could come up with. If you see another way, please propose.
> > >>>>>>>>> current->seccomp.landlock_prog is not the answer.
> > >>>>>>>>
> > >>>>>>>> Hum, I don't see the difference from a performance point of view between
> > >>>>>>>> a cgroup-based or a process hierarchy-based system.
> > >>>>>>>>
> > >>>>>>>> Maybe a better option should be to use an array of pointers with N
> > >>>>>>>> entries, one for each supported hook, instead of a unique pointer list?
> > >>>>>>>
> > >>>>>>> yes, clearly array dereference is faster than link list walk.
> > >>>>>>> Now the question is where to keep this prog_array[num_lsm_hooks] ?
> > >>>>>>> Since we cannot keep it inside task_struct, we have to allocate it.
> > >>>>>>> Every time the task is creted then. What to do on the fork? That
> > >>>>>>> will require changes all over. Then the obvious optimization would be
> > >>>>>>> to share this allocated array of prog pointers across multiple tasks...
> > >>>>>>> and little by little this new facility will look like cgroup.
> > >>>>>>> Hence the suggestion to put this array into cgroup from the start.
> > >>>>>>
> > >>>>>> I see your point :)
> > >>>>>>
> > >>>>>>>
> > >>>>>>>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
> > >>>>>>>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
> > >>>>>>>> to use a process hierarchy). The downside will be to handle an LSM hook
> > >>>>>>>> program which is not triggered by a seccomp-filter, but this should be
> > >>>>>>>> needed anyway to handle interruptions.
> > >>>>>>>
> > >>>>>>> what do you mean 'not triggered by seccomp' ?
> > >>>>>>> You're not suggesting that this lsm has to enable seccomp to be functional?
> > >>>>>>> imo that's non starter due to overhead.
> > >>>>>>
> > >>>>>> Yes, for now, it is triggered by a new seccomp filter return value
> > >>>>>> RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
> > >>>>>> be needed but could be useful to bind a seccomp filter security policy
> > >>>>>> with a Landlock one. Waiting for Kees's point of view…
> > >>>>>>
> > >>>>>
> > >>>>> I'm not Kees, but I'd be okay with that.  I still think that doing
> > >>>>> this by process hierarchy a la seccomp will be easier to use and to
> > >>>>> understand (which is quite important for this kind of work) than doing
> > >>>>> it by cgroup.
> > >>>>>
> > >>>>> A feature I've wanted to add for a while is to have an fd that
> > >>>>> represents a seccomp layer, the idea being that you would set up your
> > >>>>> seccomp layer (with syscall filter, landlock hooks, etc) and then you
> > >>>>> would have a syscall to install that layer.  Then an unprivileged
> > >>>>> sandbox manager could set up its layer and still be able to inject new
> > >>>>> processes into it later on, no cgroups needed.
> > >>>>
> > >>>> A nice thing I didn't highlight about Landlock is that a process can
> > >>>> prepare a layer of rules (arraymap of handles + Landlock programs) and
> > >>>> pass the file descriptors of the Landlock programs to another process.
> > >>>> This process could then apply this programs to get sandboxed. However,
> > >>>> for now, because a Landlock program is only triggered by a seccomp
> > >>>> filter (which do not follow the Landlock programs as a FD), they will be
> > >>>> useless.
> > >>>>
> > >>>> The FD referring to an arraymap of handles can also be used to update a
> > >>>> map and change the behavior of a Landlock program. A master process can
> > >>>> then add or remove restrictions to another process hierarchy on the fly.
> > >>>
> > >>> Maybe this could be extended a little bit.  The fd could hold the
> > >>> seccomp filter *and* the LSM hook filters.  FMODE_EXECUTE could give
> > >>> the ability to install it and FMODE_WRITE could give the ability to
> > >>> modify it.
> > >>>
> > >>
> > >> This is interesting! It should be possible to append the seccomp stack
> > >> of a source process to the seccomp stack of the target process when a
> > >> Landlock program is passed and then activated through seccomp(2).
> > >>
> > >> For the FMODE_EXECUTE/FMODE_WRITE, are you suggesting to manage
> > >> permission of the eBPF program FD in a specific way?
> > >>
> > >
> > > This wouldn't be an eBPF program FD -- it would be an FD encapsulating
> > > an entire configuration including seccomp BPF program, whatever
> > > landlock stuff is associated, and eventual seccomp monitor
> > > configuration (once I write that code), etc.
> > >
> > > You wouldn't say "attach this process's seccomp stack to me" -- you'd
> > > say "attach this seccomp layer to me".
> > >
> > > A decision that we'd have to make would be whether the FD links to the
> > > parent layer or whether it can be attached without regard to what the
> > > parent layer is.
> >
> > OK, I like that, but I think it could be done on a second time. :)
>
> I don't. Single FD that is a collection of objects seems an odd abstraction
> to me. I also don't see what it actually solves.
> I think lsm and seccomp should be orthogonal and not tied into each other.
>

It's not a random collection of objects.  It's a fully configured
sandboxing layer.

One might argue that landlock shouldn't be tied to seccomp (in theory,
attached progs could be given access to syscall_get_xyz()), but I
think that the seccomp attachment mechanism is the right way to
install unprivileged filters.  It handles the no_new_privs stuff, it
allows TSYNC, it's totally independent of systemwide policy, etc.

Trying to use cgroups or similar for this is going to be much nastier.
Some tighter sandboxes (Sandstorm, etc) aren't even going to dream of
putting cgroupfs in their containers, so requiring cgroups or similar
would be a mess for that type of application.

--Andy

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Tue, Aug 30, 2016 at 02:45:14PM -0700, Andy Lutomirski wrote:
> 
> One might argue that landlock shouldn't be tied to seccomp (in theory,
> attached progs could be given access to syscall_get_xyz()), but I

proposed lsm is way more powerful than syscall_get_xyz.
no need to dumb it down.

> think that the seccomp attachment mechanism is the right way to
> install unprivileged filters.  It handles the no_new_privs stuff, it
> allows TSYNC, it's totally independent of systemwide policy, etc.
> 
> Trying to use cgroups or similar for this is going to be much nastier.
> Some tighter sandboxes (Sandstorm, etc) aren't even going to dream of
> putting cgroupfs in their containers, so requiring cgroups or similar
> would be a mess for that type of application.

I don't see why it is a 'mess'. cgroups are already used by majority
of the systems, so I don't see why requiring a cgroup is such a big deal.
But let's say we don't do them. How implementation is going to look like
for task based hierarchy? Note that we need an array of bpf_prog pointers.
One for each lsm hook. Where this array is going to be stored?
We cannot put in task_struct, since it's too large. Cannot put it
into 'struct seccomp' directly either, unless it will become a pointer.
Is that the proposal?
So now we will be wasting extra 1kbyte of memory per task. Not great.
We'd want to optimize it by sharing this such struct seccomp with prog array
across threads of the same task? Or dynimically allocating it when
landlock is in use? May sound nice, but how to account for that kernel
memory? I guess also solvable by charging memlock.
With cgroup based approach we don't need to worry about all that.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Andy Lutomirski]

On Tue, Aug 30, 2016 at 6:36 PM, Alexei Starovoitov
<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Aug 30, 2016 at 02:45:14PM -0700, Andy Lutomirski wrote:
>>
>> One might argue that landlock shouldn't be tied to seccomp (in theory,
>> attached progs could be given access to syscall_get_xyz()), but I
>
> proposed lsm is way more powerful than syscall_get_xyz.
> no need to dumb it down.

I think you're misunderstanding me.

Mickaël's code allows one to make the LSM hook filters depend on the
syscall using SECCOMP_RET_LANDLOCK.  I'm suggesting that a similar
effect could be achieved by allowing the eBPF LSM hook to call
syscall_get_xyz() if it wants to.

>
>> think that the seccomp attachment mechanism is the right way to
>> install unprivileged filters.  It handles the no_new_privs stuff, it
>> allows TSYNC, it's totally independent of systemwide policy, etc.
>>
>> Trying to use cgroups or similar for this is going to be much nastier.
>> Some tighter sandboxes (Sandstorm, etc) aren't even going to dream of
>> putting cgroupfs in their containers, so requiring cgroups or similar
>> would be a mess for that type of application.
>
> I don't see why it is a 'mess'. cgroups are already used by majority
> of the systems, so I don't see why requiring a cgroup is such a big deal.

Requiring cgroup to be configured in isn't a big deal.  Requiring

> But let's say we don't do them. How implementation is going to look like
> for task based hierarchy? Note that we need an array of bpf_prog pointers.
> One for each lsm hook. Where this array is going to be stored?
> We cannot put in task_struct, since it's too large. Cannot put it
> into 'struct seccomp' directly either, unless it will become a pointer.
> Is that the proposal?

It would go in struct seccomp_filter or in something pointed to from there.

> So now we will be wasting extra 1kbyte of memory per task. Not great.
> We'd want to optimize it by sharing this such struct seccomp with prog array
> across threads of the same task? Or dynimically allocating it when
> landlock is in use? May sound nice, but how to account for that kernel
> memory? I guess also solvable by charging memlock.
> With cgroup based approach we don't need to worry about all that.
>

The considerations are essentially identical either way.

With cgroups, if you want to share the memory between multiple
separate sandboxes (Firejail instances, Sandstorm grains, Chromium
instances, xdg-apps, etc), you'd need to get them to all coordinate to
share a cgroup.  With a seccomp-like interface, you'd need to get them
to coordinate to share an installed layer (using my FD idea or
similar).

There would *not* be any duplication of this memory just because a
sandboxed process called fork().

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC v2 09/10] landlock: Handle cgroups (netfilter match)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 1858 bytes --]


On 27/08/2016 01:05, Alexei Starovoitov wrote:
> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>> To sum up, there is four related patchsets:
>> * "Landlock LSM: Unprivileged sandboxing" (this series)
>> * "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
>> * "Networking cgroup controller" (Anoop Naravaram)
>> * "Add eBPF hooks for cgroups" (Daniel Mack)

>>> Anoop Naravaram's use case is to control the ports the applications
>>> under cgroup can bind and listen on.
>>> Such use case can be solved by such 'lsm cgroup controller' by
>>> attaching bpf program to security_socket_bind lsm hook and
>>> filtering sockaddr.
>>> Furthermore Sargun's use case is to allow further sockaddr rewrites
>>> from the bpf program which can be done as natural extension
>>> of such mechanism.
>>>
>>> If I understood Daniel's Anoop's Sargun's and yours use cases
>>> correctly the common piece of kernel infrastructure that can solve
>>> them all can start from Daniel's current set of patches that
>>> establish a mechanism of attaching bpf program to a cgroup.
>>> Then adding lsm hooks to it and later allowing argument rewrite
>>> (since they're already in the kernel and no ToCToU problems exist)

>> For the network-related series, I think it make more sense to simply
>> create a netfilter rule matching a cgroup and then add more features to
>> netfilter (restrict port ranges and so on) thanks to eBPF programs.
>> Containers are (usually) in a dedicated network namespace, which open
>> the possibility to not only rely on cgroups (e.g. match UID,
>> netmask...). It would also be more flexible to be able to load a BPF
>> program in netfilter and update its maps on the fly to make dynamic
>> rules, like ipset does, but in a more generic way.

What do the netdev folks think about this design?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC v2 09/10] landlock: Handle cgroups (netfilter match)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:19:05PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >> To sum up, there is four related patchsets:
> >> * "Landlock LSM: Unprivileged sandboxing" (this series)
> >> * "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
> >> * "Networking cgroup controller" (Anoop Naravaram)
> >> * "Add eBPF hooks for cgroups" (Daniel Mack)
> 
> >>> Anoop Naravaram's use case is to control the ports the applications
> >>> under cgroup can bind and listen on.
> >>> Such use case can be solved by such 'lsm cgroup controller' by
> >>> attaching bpf program to security_socket_bind lsm hook and
> >>> filtering sockaddr.
> >>> Furthermore Sargun's use case is to allow further sockaddr rewrites
> >>> from the bpf program which can be done as natural extension
> >>> of such mechanism.
> >>>
> >>> If I understood Daniel's Anoop's Sargun's and yours use cases
> >>> correctly the common piece of kernel infrastructure that can solve
> >>> them all can start from Daniel's current set of patches that
> >>> establish a mechanism of attaching bpf program to a cgroup.
> >>> Then adding lsm hooks to it and later allowing argument rewrite
> >>> (since they're already in the kernel and no ToCToU problems exist)
> 
> >> For the network-related series, I think it make more sense to simply
> >> create a netfilter rule matching a cgroup and then add more features to
> >> netfilter (restrict port ranges and so on) thanks to eBPF programs.
> >> Containers are (usually) in a dedicated network namespace, which open
> >> the possibility to not only rely on cgroups (e.g. match UID,
> >> netmask...). It would also be more flexible to be able to load a BPF
> >> program in netfilter and update its maps on the fly to make dynamic
> >> rules, like ipset does, but in a more generic way.
> 
> What do the netdev folks think about this design?

such design doesn't scale when used for container management and
that's what we need to solve.
netns has its overhead and management issues. There are proposals to
solve that but that is orthogonal to containers in general.
A lot of them don't use netns.

Re: [RFC v2 00/10] Landlock LSM: Unprivileged sandboxing (cgroup delegation)

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 1324 bytes --]

Cc Tejun and the cgroups ML.

On 27/08/2016 17:10, Mickaël Salaün wrote:
> On 27/08/2016 09:40, Andy Lutomirski wrote:
>> On Thu, Aug 25, 2016 at 3:32 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>> # Sandbox example with conditional access control depending on cgroup
>>>
>>>   $ mkdir /sys/fs/cgroup/sandboxed
>>>   $ ls /home
>>>   user1
>>>   $ LANDLOCK_CGROUPS='/sys/fs/cgroup/sandboxed' \
>>>       LANDLOCK_ALLOWED='/bin:/lib:/usr:/tmp:/proc/self/fd/0' \
>>>       ./sandbox /bin/sh -i
>>>   $ ls /home
>>>   user1
>>>   $ echo $$ > /sys/fs/cgroup/sandboxed/cgroup.procs
>>>   $ ls /home
>>>   ls: cannot open directory '/home': Permission denied
>>>
>>
>> Something occurs to me that isn't strictly relevant to landlock but
>> may be relevant to unprivileged cgroups: can you cause trouble by
>> setting up a nastily-configured cgroup and running a setuid program in
>> it?
>>
> 
> I hope not… But the use of cgroups should not be mandatory for Landlock.
> 

In a previous email:

On 26/08/2016 17:50, Tejun Heo wrote:
> I haven't looked in detail but in general I'm not too excited about
> layering security mechanism on top of cgroup.  Maybe it makes some
> sense when security domain coincides with resource domains but at any
> rate please keep me in the loop.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]