Re: [RFC v3 07/22] landlock: Handle file comparisons

[Kees Cook]

On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün  wrote:
> Add eBPF functions to compare file system access with a Landlock file
> system handle:
> * bpf_landlock_cmp_fs_prop_with_struct_file(prop, map, map_op, file)
>   This function allows to compare the dentry, inode, device or mount
>   point of the currently accessed file, with a reference handle.
> * bpf_landlock_cmp_fs_beneath_with_struct_file(opt, map, map_op, file)
>   This function allows an eBPF program to check if the current accessed
>   file is the same or in the hierarchy of a reference handle.
>
> The goal of file system handle is to abstract kernel objects such as a
> struct file or a struct inode. Userland can create this kind of handle
> thanks to the BPF_MAP_UPDATE_ELEM command. The element is a struct
> landlock_handle containing the handle type (e.g.
> BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD) and a file descriptor. This could
> also be any descriptions able to match a struct file or a struct inode
> (e.g. path or glob string).
>
> Changes since v2:
> * add MNT_INTERNAL check to only add file handle from user-visible FS
>   (e.g. no anonymous inode)
> * replace struct file* with struct path* in map_landlock_handle
> * add BPF protos
> * fix bpf_landlock_cmp_fs_prop_with_struct_file()
>
> Signed-off-by: Mickaël Salaün 
> Cc: Alexei Starovoitov 
> Cc: Andy Lutomirski 
> Cc: Daniel Borkmann 
> Cc: David S. Miller 
> Cc: James Morris 
> Cc: Kees Cook 
> Cc: Serge E. Hallyn 
> Link: 
> https://lkml.kernel.org/r/calcetrwwtiz3kztkegow24-dvhqq6lftwexh77fd2g5o71y...@mail.gmail.com
> ---
>  include/linux/bpf.h|  10 +++
>  include/uapi/linux/bpf.h   |  49 +++
>  kernel/bpf/arraymap.c  |  21 +
>  kernel/bpf/verifier.c  |   8 ++
>  security/landlock/Makefile |   2 +-
>  security/landlock/checker_fs.c | 179 
> +
>  security/landlock/checker_fs.h |  20 +
>  security/landlock/lsm.c|   6 ++
>  8 files changed, 294 insertions(+), 1 deletion(-)
>  create mode 100644 security/landlock/checker_fs.c
>  create mode 100644 security/landlock/checker_fs.h
> [...]
> diff --git a/security/landlock/checker_fs.c b/security/landlock/checker_fs.c
> new file mode 100644
> index ..39eb85dc7d18
> --- /dev/null
> +++ b/security/landlock/checker_fs.c
> @@ -0,0 +1,179 @@
> +/*
> + * Landlock LSM - File System Checkers
> + *
> + * Copyright (C) 2016  Mickaël Salaün 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#include  /* enum bpf_map_array_op */
> +#include 
> +#include  /* path_is_under() */
> +#include  /* struct path */
> +
> +#include "checker_fs.h"
> +
> +#define EQUAL_NOT_NULL(a, b) (a && a == b)
> +
> +/*
> + * bpf_landlock_cmp_fs_prop_with_struct_file
> + *
> + * Cf. include/uapi/linux/bpf.h
> + */
> +static inline u64 bpf_landlock_cmp_fs_prop_with_struct_file(u64 r1_property,
> +   u64 r2_map, u64 r3_map_op, u64 r4_file, u64 r5)
> +{
> +   u8 property = (u8) r1_property;
> +   struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
> +   enum bpf_map_array_op map_op = r3_map_op;
> +   struct file *file = (struct file *) (unsigned long) r4_file;
> +   struct bpf_array *array = container_of(map, struct bpf_array, map);
> +   struct path *p1, *p2;
> +   struct map_landlock_handle *handle;
> +   int i;
> +
> +   /* ARG_CONST_PTR_TO_LANDLOCK_HANDLE_FS is an arraymap */
> +   if (unlikely(!map)) {
> +   WARN_ON(1);
> +   return -EFAULT;
> +   }

Just some minor style/readability nits...

This is more readable as:

if (WARN_ON(!map))
return -EFAULT;

(WARN_ON already includes the unlikely() and passes through the test result.)

> +   if (unlikely(!file))
> +   return -ENOENT;
> +   if (unlikely((property | _LANDLOCK_FLAG_FS_MASK) != 
> _LANDLOCK_FLAG_FS_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;
> +   }
> +   p2 = >f_path;
> +
> +   synchronize_rcu();
> +
> +   for (i = 0; i < array->n_entries; i++) {
> +   bool result_dentry = !(property & LANDLOCK_FLAG_FS_DENTRY);
> +   bool result_inode = !(property & LANDLOCK_FLAG_FS_INODE);
> +   bool result_device = !(property & LANDLOCK_FLAG_FS_DEVICE);
> +   bool result_mount = 

Re: lsm naming dilemma. Re: [RFC v3 07/22] landlock: Handle file comparisons

[Mickaël Salaün]

On 20/09/2016 03:10, Sargun Dhillon wrote:
> I'm fine giving up the Checmate name. Landlock seems easy enough to
> Google. I haven't gotten a chance to look through the entire patchset
> yet, but it does seem like they are somewhat similar.

Excellent! I'm looking forward for your review.


> 
> On Mon, Sep 19, 2016 at 5:12 PM, Alexei Starovoitov
>  wrote:
>> On Thu, Sep 15, 2016 at 11:25:10PM +0200, Mickaël Salaün wrote:
> Agreed. With this RFC, the Checmate features (i.e. network helpers)
> should be able to sit on top of Landlock.

 I think neither of them should be called fancy names for no technical 
 reason.
 We will have only one bpf based lsm. That's it and it doesn't
 need an obscure name. Directory name can be security/bpf/..stuff.c
>>>
>>> I disagree on an LSM named "BPF". I first started with the "seccomp LSM"
>>> name (first RFC) but I later realized that it is confusing because
>>> seccomp is associated to its syscall and the underlying features. Same
>>> thing goes for BPF. It is also artificially hard to grep on a name too
>>> used in the kernel source tree.
>>> Making an association between the generic eBPF mechanism and a security
>>> centric approach (i.e. LSM) seems a bit reductive (for BPF). Moreover,
>>> the seccomp interface [1] can still be used.
>>
>> agree with above.
>>
>>> Landlock is a nice name to depict a sandbox as an enclave (i.e. a
>>> landlocked country/state). I want to keep this name, which is simple,
>>> express the goal of Landlock nicely and is comparable to other sandbox
>>> mechanisms as Seatbelt or Pledge.
>>> Landlock should not be confused with the underlying eBPF implementation.
>>> Landlock could use more than only eBPF in the future and eBPF could be
>>> used in other LSM as well.
>>
>> there will not be two bpf based LSMs.
>> Therefore unless you can convince Sargun to give up his 'checmate' name,
>> nothing goes in.
>> The features you both need are 90% the same, so they must be done
>> as part of single LSM whatever you both agree to call it.
>>
> 



signature.asc
Description: OpenPGP digital signature

Re: lsm naming dilemma. Re: [RFC v3 07/22] landlock: Handle file comparisons

[Sargun Dhillon]

I'm fine giving up the Checmate name. Landlock seems easy enough to
Google. I haven't gotten a chance to look through the entire patchset
yet, but it does seem like they are somewhat similar.

On Mon, Sep 19, 2016 at 5:12 PM, Alexei Starovoitov
 wrote:
> On Thu, Sep 15, 2016 at 11:25:10PM +0200, Mickaël Salaün wrote:
>> >> Agreed. With this RFC, the Checmate features (i.e. network helpers)
>> >> should be able to sit on top of Landlock.
>> >
>> > I think neither of them should be called fancy names for no technical 
>> > reason.
>> > We will have only one bpf based lsm. That's it and it doesn't
>> > need an obscure name. Directory name can be security/bpf/..stuff.c
>>
>> I disagree on an LSM named "BPF". I first started with the "seccomp LSM"
>> name (first RFC) but I later realized that it is confusing because
>> seccomp is associated to its syscall and the underlying features. Same
>> thing goes for BPF. It is also artificially hard to grep on a name too
>> used in the kernel source tree.
>> Making an association between the generic eBPF mechanism and a security
>> centric approach (i.e. LSM) seems a bit reductive (for BPF). Moreover,
>> the seccomp interface [1] can still be used.
>
> agree with above.
>
>> Landlock is a nice name to depict a sandbox as an enclave (i.e. a
>> landlocked country/state). I want to keep this name, which is simple,
>> express the goal of Landlock nicely and is comparable to other sandbox
>> mechanisms as Seatbelt or Pledge.
>> Landlock should not be confused with the underlying eBPF implementation.
>> Landlock could use more than only eBPF in the future and eBPF could be
>> used in other LSM as well.
>
> there will not be two bpf based LSMs.
> Therefore unless you can convince Sargun to give up his 'checmate' name,
> nothing goes in.
> The features you both need are 90% the same, so they must be done
> as part of single LSM whatever you both agree to call it.
>

lsm naming dilemma. Re: [RFC v3 07/22] landlock: Handle file comparisons

[Alexei Starovoitov]

On Thu, Sep 15, 2016 at 11:25:10PM +0200, Mickaël Salaün wrote:
> >> Agreed. With this RFC, the Checmate features (i.e. network helpers)
> >> should be able to sit on top of Landlock.
> > 
> > I think neither of them should be called fancy names for no technical 
> > reason.
> > We will have only one bpf based lsm. That's it and it doesn't
> > need an obscure name. Directory name can be security/bpf/..stuff.c
> 
> I disagree on an LSM named "BPF". I first started with the "seccomp LSM"
> name (first RFC) but I later realized that it is confusing because
> seccomp is associated to its syscall and the underlying features. Same
> thing goes for BPF. It is also artificially hard to grep on a name too
> used in the kernel source tree.
> Making an association between the generic eBPF mechanism and a security
> centric approach (i.e. LSM) seems a bit reductive (for BPF). Moreover,
> the seccomp interface [1] can still be used.

agree with above.

> Landlock is a nice name to depict a sandbox as an enclave (i.e. a
> landlocked country/state). I want to keep this name, which is simple,
> express the goal of Landlock nicely and is comparable to other sandbox
> mechanisms as Seatbelt or Pledge.
> Landlock should not be confused with the underlying eBPF implementation.
> Landlock could use more than only eBPF in the future and eBPF could be
> used in other LSM as well.

there will not be two bpf based LSMs.
Therefore unless you can convince Sargun to give up his 'checmate' name,
nothing goes in.
The features you both need are 90% the same, so they must be done
as part of single LSM whatever you both agree to call it.

Re: [RFC v3 07/22] landlock: Handle file comparisons

[Mickaël Salaün]

On 15/09/2016 01:24, Alexei Starovoitov wrote:
> On Thu, Sep 15, 2016 at 01:02:22AM +0200, Mickaël Salaün wrote:
>>>
>>> I would suggest for the next RFC to do minimal 7 patches up to this point
>>> with simple example that demonstrates the use case.
>>> I would avoid all unpriv stuff and all of seccomp for the next RFC as well,
>>> otherwise I don't think we can realistically make forward progress, since
>>> there are too many issues raised in the subsequent patches.
>>
>> I hope we will find a common agreement about seccomp vs cgroup… I think
>> both approaches have their advantages, can be complementary and nicely
>> combined.
> 
> I don't mind having both task based lsm and cgroup based as long as
> infrastracture is not duplicated and scaling issues from earlier version
> are resolved.

It should be much better with this RFC.

> I'm proposing to do cgroup only for the next RFC, since mine and Sargun's
> use case for this bpf+lsm+cgroup is _not_ security or sandboxing.

Well, LSM purpose is to do security stuff. The main goal of Landlock is
to bring security features to userland, including unprivileged
processes, at least via the seccomp interface [1].

> No need for unpriv, no_new_priv to cgroups are other things that Andy
> is concerned about.

I'm concern about security too! :)

> 
>> Unprivileged sandboxing is the main goal of Landlock. This should not be
>> a problem, even for privileged features, thanks to the new subtype/access.
> 
> yes. the point that unpriv stuff can come later after agreement is reached.
> If we keep arguing about seccomp details this set won't go anywhere.
> Even in basic part (which is cgroup+bpf+lsm) are plenty of questions
> to be still agreed.

Using the seccomp(2) (unpriv) *interface* is OK according to a more
recent thread [1].

[1]
https://lkml.kernel.org/r/20160915044852.ga66...@ast-mbp.thefacebook.com

> 
>> Agreed. With this RFC, the Checmate features (i.e. network helpers)
>> should be able to sit on top of Landlock.
> 
> I think neither of them should be called fancy names for no technical reason.
> We will have only one bpf based lsm. That's it and it doesn't
> need an obscure name. Directory name can be security/bpf/..stuff.c

I disagree on an LSM named "BPF". I first started with the "seccomp LSM"
name (first RFC) but I later realized that it is confusing because
seccomp is associated to its syscall and the underlying features. Same
thing goes for BPF. It is also artificially hard to grep on a name too
used in the kernel source tree.
Making an association between the generic eBPF mechanism and a security
centric approach (i.e. LSM) seems a bit reductive (for BPF). Moreover,
the seccomp interface [1] can still be used.

Landlock is a nice name to depict a sandbox as an enclave (i.e. a
landlocked country/state). I want to keep this name, which is simple,
express the goal of Landlock nicely and is comparable to other sandbox
mechanisms as Seatbelt or Pledge.
Landlock should not be confused with the underlying eBPF implementation.
Landlock could use more than only eBPF in the future and eBPF could be
used in other LSM as well.

 Mickaël



signature.asc
Description: OpenPGP digital signature

Re: [RFC v3 07/22] landlock: Handle file comparisons

[Alexei Starovoitov]

On Thu, Sep 15, 2016 at 01:02:22AM +0200, Mickaël Salaün wrote:
> > 
> > I would suggest for the next RFC to do minimal 7 patches up to this point
> > with simple example that demonstrates the use case.
> > I would avoid all unpriv stuff and all of seccomp for the next RFC as well,
> > otherwise I don't think we can realistically make forward progress, since
> > there are too many issues raised in the subsequent patches.
> 
> I hope we will find a common agreement about seccomp vs cgroup… I think
> both approaches have their advantages, can be complementary and nicely
> combined.

I don't mind having both task based lsm and cgroup based as long as
infrastracture is not duplicated and scaling issues from earlier version
are resolved.
I'm proposing to do cgroup only for the next RFC, since mine and Sargun's
use case for this bpf+lsm+cgroup is _not_ security or sandboxing.
No need for unpriv, no_new_priv to cgroups are other things that Andy
is concerned about.

> Unprivileged sandboxing is the main goal of Landlock. This should not be
> a problem, even for privileged features, thanks to the new subtype/access.

yes. the point that unpriv stuff can come later after agreement is reached.
If we keep arguing about seccomp details this set won't go anywhere.
Even in basic part (which is cgroup+bpf+lsm) are plenty of questions
to be still agreed.

> Agreed. With this RFC, the Checmate features (i.e. network helpers)
> should be able to sit on top of Landlock.

I think neither of them should be called fancy names for no technical reason.
We will have only one bpf based lsm. That's it and it doesn't
need an obscure name. Directory name can be security/bpf/..stuff.c

Re: [RFC v3 07/22] landlock: Handle file comparisons

[Mickaël Salaün]

On 14/09/2016 23:06, Alexei Starovoitov wrote:
> On Wed, Sep 14, 2016 at 09:24:00AM +0200, Mickaël Salaün wrote:
>> Add eBPF functions to compare file system access with a Landlock file
>> system handle:
>> * bpf_landlock_cmp_fs_prop_with_struct_file(prop, map, map_op, file)
>>   This function allows to compare the dentry, inode, device or mount
>>   point of the currently accessed file, with a reference handle.
>> * bpf_landlock_cmp_fs_beneath_with_struct_file(opt, map, map_op, file)
>>   This function allows an eBPF program to check if the current accessed
>>   file is the same or in the hierarchy of a reference handle.
>>
>> The goal of file system handle is to abstract kernel objects such as a
>> struct file or a struct inode. Userland can create this kind of handle
>> thanks to the BPF_MAP_UPDATE_ELEM command. The element is a struct
>> landlock_handle containing the handle type (e.g.
>> BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD) and a file descriptor. This could
>> also be any descriptions able to match a struct file or a struct inode
>> (e.g. path or glob string).
>>
>> Changes since v2:
>> * add MNT_INTERNAL check to only add file handle from user-visible FS
>>   (e.g. no anonymous inode)
>> * replace struct file* with struct path* in map_landlock_handle
>> * add BPF protos
>> * fix bpf_landlock_cmp_fs_prop_with_struct_file()
>>
>> Signed-off-by: Mickaël Salaün 
>> Cc: Alexei Starovoitov 
>> Cc: Andy Lutomirski 
>> Cc: Daniel Borkmann 
>> Cc: David S. Miller 
>> Cc: James Morris 
>> Cc: Kees Cook 
>> Cc: Serge E. Hallyn 
>> Link: 
>> https://lkml.kernel.org/r/calcetrwwtiz3kztkegow24-dvhqq6lftwexh77fd2g5o71y...@mail.gmail.com
> 
> thanks for keeping the links to the previous discussion.
> Long term it should help, though I worry we already at the point
> where there are too many outstanding issues to resolve before we
> can proceed with reasonable code review.
> 
>> +/*
>> + * bpf_landlock_cmp_fs_prop_with_struct_file
>> + *
>> + * Cf. include/uapi/linux/bpf.h
>> + */
>> +static inline u64 bpf_landlock_cmp_fs_prop_with_struct_file(u64 r1_property,
>> +u64 r2_map, u64 r3_map_op, u64 r4_file, u64 r5)
>> +{
>> +u8 property = (u8) r1_property;
>> +struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
>> +enum bpf_map_array_op map_op = r3_map_op;
>> +struct file *file = (struct file *) (unsigned long) r4_file;
> 
> please use just added BPF_CALL_ macros. They will help readability of the 
> above.
> 
>> +struct bpf_array *array = container_of(map, struct bpf_array, map);
>> +struct path *p1, *p2;
>> +struct map_landlock_handle *handle;
>> +int i;
>> +
>> +/* ARG_CONST_PTR_TO_LANDLOCK_HANDLE_FS is an arraymap */
>> +if (unlikely(!map)) {
>> +WARN_ON(1);
>> +return -EFAULT;
>> +}
>> +if (unlikely(!file))
>> +return -ENOENT;
>> +if (unlikely((property | _LANDLOCK_FLAG_FS_MASK) != 
>> _LANDLOCK_FLAG_FS_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;
>> +}
>> +p2 = >f_path;
>> +
>> +synchronize_rcu();
> 
> that is completely broken.
> bpf programs are executing under rcu_lock.
> please enable CONFIG_PROVE_RCU and retest everything.

Thanks for the tip. I will fix this.

> 
> I would suggest for the next RFC to do minimal 7 patches up to this point
> with simple example that demonstrates the use case.
> I would avoid all unpriv stuff and all of seccomp for the next RFC as well,
> otherwise I don't think we can realistically make forward progress, since
> there are too many issues raised in the subsequent patches.

I hope we will find a common agreement about seccomp vs cgroup… I think
both approaches have their advantages, can be complementary and nicely
combined.

Unprivileged sandboxing is the main goal of Landlock. This should not be
a problem, even for privileged features, thanks to the new subtype/access.

> 
> The common part that is mergeable is prog's subtype extension to
> the verifier that can be used for better tracing and is the common
> piece of infra needed for both landlock and checmate LSMs
> (which must be one LSM anyway)

Agreed. With this RFC, the Checmate features (i.e. network helpers)
should be able to sit on top of Landlock.



signature.asc
Description: OpenPGP digital signature

Re: [RFC v3 07/22] landlock: Handle file comparisons

[Mickaël Salaün]

On 14/09/2016 21:07, Jann Horn wrote:
> On Wed, Sep 14, 2016 at 09:24:00AM +0200, Mickaël Salaün wrote:
>> Add eBPF functions to compare file system access with a Landlock file
>> system handle:
>> * bpf_landlock_cmp_fs_prop_with_struct_file(prop, map, map_op, file)
>>   This function allows to compare the dentry, inode, device or mount
>>   point of the currently accessed file, with a reference handle.
>> * bpf_landlock_cmp_fs_beneath_with_struct_file(opt, map, map_op, file)
>>   This function allows an eBPF program to check if the current accessed
>>   file is the same or in the hierarchy of a reference handle.
> [...]
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index 94256597eacd..edaab4c87292 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -603,6 +605,9 @@ static void landlock_put_handle(struct 
>> map_landlock_handle *handle)
>>  enum bpf_map_handle_type handle_type = handle->type;
>>  
>>  switch (handle_type) {
>> +case BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD:
>> +path_put(>path);
>> +break;
>>  case BPF_MAP_HANDLE_TYPE_UNSPEC:
>>  default:
>>  WARN_ON(1);
> [...]
>> diff --git a/security/landlock/checker_fs.c b/security/landlock/checker_fs.c
>> new file mode 100644
>> index ..39eb85dc7d18
>> --- /dev/null
>> +++ b/security/landlock/checker_fs.c
> [...]
>> +static inline u64 bpf_landlock_cmp_fs_prop_with_struct_file(u64 r1_property,
>> +u64 r2_map, u64 r3_map_op, u64 r4_file, u64 r5)
>> +{
>> +u8 property = (u8) r1_property;
>> +struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
>> +enum bpf_map_array_op map_op = r3_map_op;
>> +struct file *file = (struct file *) (unsigned long) r4_file;
>> +struct bpf_array *array = container_of(map, struct bpf_array, map);
>> +struct path *p1, *p2;
>> +struct map_landlock_handle *handle;
>> +int i;
> 
> Please don't use int when iterating over an array, use size_t.

OK, I will use size_t.

> 
> 
>> +/* for now, only handle OP_OR */
> 
> Is "OP_OR" an appropriate name for something that ANDs the success of
> checks?
> 
> 
> [...]
>> +synchronize_rcu();
> 
> Can you put a comment here that explains what's going on?

Hum, this should not be here.

> 
> 
>> +for (i = 0; i < array->n_entries; i++) {
>> +bool result_dentry = !(property & LANDLOCK_FLAG_FS_DENTRY);
>> +bool result_inode = !(property & LANDLOCK_FLAG_FS_INODE);
>> +bool result_device = !(property & LANDLOCK_FLAG_FS_DEVICE);
>> +bool result_mount = !(property & LANDLOCK_FLAG_FS_MOUNT);
>> +
>> +handle = (struct map_landlock_handle *)
>> +(array->value + array->elem_size * i);
>> +
>> +if (handle->type != BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD) {
>> +WARN_ON(1);
>> +return -EFAULT;
>> +}
>> +p1 = >path;
>> +
>> +if (!result_dentry && p1->dentry == p2->dentry)
>> +result_dentry = true;
> 
> Why is this safe? As far as I can tell, this is not in an RCU read-side
> critical section (synchronize_rcu() was just called), and no lock has been
> taken. What prevents someone from removing the arraymap entry while we're
> looking at it? Am I missing something?

I will try to properly deal with RCU.



signature.asc
Description: OpenPGP digital signature

Re: [RFC v3 07/22] landlock: Handle file comparisons

[Alexei Starovoitov]

On Wed, Sep 14, 2016 at 09:24:00AM +0200, Mickaël Salaün wrote:
> Add eBPF functions to compare file system access with a Landlock file
> system handle:
> * bpf_landlock_cmp_fs_prop_with_struct_file(prop, map, map_op, file)
>   This function allows to compare the dentry, inode, device or mount
>   point of the currently accessed file, with a reference handle.
> * bpf_landlock_cmp_fs_beneath_with_struct_file(opt, map, map_op, file)
>   This function allows an eBPF program to check if the current accessed
>   file is the same or in the hierarchy of a reference handle.
> 
> The goal of file system handle is to abstract kernel objects such as a
> struct file or a struct inode. Userland can create this kind of handle
> thanks to the BPF_MAP_UPDATE_ELEM command. The element is a struct
> landlock_handle containing the handle type (e.g.
> BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD) and a file descriptor. This could
> also be any descriptions able to match a struct file or a struct inode
> (e.g. path or glob string).
> 
> Changes since v2:
> * add MNT_INTERNAL check to only add file handle from user-visible FS
>   (e.g. no anonymous inode)
> * replace struct file* with struct path* in map_landlock_handle
> * add BPF protos
> * fix bpf_landlock_cmp_fs_prop_with_struct_file()
> 
> Signed-off-by: Mickaël Salaün 
> Cc: Alexei Starovoitov 
> Cc: Andy Lutomirski 
> Cc: Daniel Borkmann 
> Cc: David S. Miller 
> Cc: James Morris 
> Cc: Kees Cook 
> Cc: Serge E. Hallyn 
> Link: 
> https://lkml.kernel.org/r/calcetrwwtiz3kztkegow24-dvhqq6lftwexh77fd2g5o71y...@mail.gmail.com

thanks for keeping the links to the previous discussion.
Long term it should help, though I worry we already at the point
where there are too many outstanding issues to resolve before we
can proceed with reasonable code review.

> +/*
> + * bpf_landlock_cmp_fs_prop_with_struct_file
> + *
> + * Cf. include/uapi/linux/bpf.h
> + */
> +static inline u64 bpf_landlock_cmp_fs_prop_with_struct_file(u64 r1_property,
> + u64 r2_map, u64 r3_map_op, u64 r4_file, u64 r5)
> +{
> + u8 property = (u8) r1_property;
> + struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
> + enum bpf_map_array_op map_op = r3_map_op;
> + struct file *file = (struct file *) (unsigned long) r4_file;

please use just added BPF_CALL_ macros. They will help readability of the above.

> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> + struct path *p1, *p2;
> + struct map_landlock_handle *handle;
> + int i;
> +
> + /* ARG_CONST_PTR_TO_LANDLOCK_HANDLE_FS is an arraymap */
> + if (unlikely(!map)) {
> + WARN_ON(1);
> + return -EFAULT;
> + }
> + if (unlikely(!file))
> + return -ENOENT;
> + if (unlikely((property | _LANDLOCK_FLAG_FS_MASK) != 
> _LANDLOCK_FLAG_FS_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;
> + }
> + p2 = >f_path;
> +
> + synchronize_rcu();

that is completely broken.
bpf programs are executing under rcu_lock.
please enable CONFIG_PROVE_RCU and retest everything.

I would suggest for the next RFC to do minimal 7 patches up to this point
with simple example that demonstrates the use case.
I would avoid all unpriv stuff and all of seccomp for the next RFC as well,
otherwise I don't think we can realistically make forward progress, since
there are too many issues raised in the subsequent patches.

The common part that is mergeable is prog's subtype extension to
the verifier that can be used for better tracing and is the common
piece of infra needed for both landlock and checmate LSMs
(which must be one LSM anyway)

Re: [RFC v3 07/22] landlock: Handle file comparisons

[Jann Horn]

On Wed, Sep 14, 2016 at 09:24:00AM +0200, Mickaël Salaün wrote:
> Add eBPF functions to compare file system access with a Landlock file
> system handle:
> * bpf_landlock_cmp_fs_prop_with_struct_file(prop, map, map_op, file)
>   This function allows to compare the dentry, inode, device or mount
>   point of the currently accessed file, with a reference handle.
> * bpf_landlock_cmp_fs_beneath_with_struct_file(opt, map, map_op, file)
>   This function allows an eBPF program to check if the current accessed
>   file is the same or in the hierarchy of a reference handle.
[...]
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 94256597eacd..edaab4c87292 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -603,6 +605,9 @@ static void landlock_put_handle(struct 
> map_landlock_handle *handle)
>   enum bpf_map_handle_type handle_type = handle->type;
>  
>   switch (handle_type) {
> + case BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD:
> + path_put(>path);
> + break;
>   case BPF_MAP_HANDLE_TYPE_UNSPEC:
>   default:
>   WARN_ON(1);
[...]
> diff --git a/security/landlock/checker_fs.c b/security/landlock/checker_fs.c
> new file mode 100644
> index ..39eb85dc7d18
> --- /dev/null
> +++ b/security/landlock/checker_fs.c
[...]
> +static inline u64 bpf_landlock_cmp_fs_prop_with_struct_file(u64 r1_property,
> + u64 r2_map, u64 r3_map_op, u64 r4_file, u64 r5)
> +{
> + u8 property = (u8) r1_property;
> + struct bpf_map *map = (struct bpf_map *) (unsigned long) r2_map;
> + enum bpf_map_array_op map_op = r3_map_op;
> + struct file *file = (struct file *) (unsigned long) r4_file;
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> + struct path *p1, *p2;
> + struct map_landlock_handle *handle;
> + int i;

Please don't use int when iterating over an array, use size_t.


> + /* for now, only handle OP_OR */

Is "OP_OR" an appropriate name for something that ANDs the success of
checks?


[...]
> + synchronize_rcu();

Can you put a comment here that explains what's going on?


> + for (i = 0; i < array->n_entries; i++) {
> + bool result_dentry = !(property & LANDLOCK_FLAG_FS_DENTRY);
> + bool result_inode = !(property & LANDLOCK_FLAG_FS_INODE);
> + bool result_device = !(property & LANDLOCK_FLAG_FS_DEVICE);
> + bool result_mount = !(property & LANDLOCK_FLAG_FS_MOUNT);
> +
> + handle = (struct map_landlock_handle *)
> + (array->value + array->elem_size * i);
> +
> + if (handle->type != BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD) {
> + WARN_ON(1);
> + return -EFAULT;
> + }
> + p1 = >path;
> +
> + if (!result_dentry && p1->dentry == p2->dentry)
> + result_dentry = true;

Why is this safe? As far as I can tell, this is not in an RCU read-side
critical section (synchronize_rcu() was just called), and no lock has been
taken. What prevents someone from removing the arraymap entry while we're
looking at it? Am I missing something?


[...]
> +static inline u64 bpf_landlock_cmp_fs_beneath_with_struct_file(u64 r1_option,
> + u64 r2_map, u64 r3_map_op, u64 r4_file, 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 file *file = (struct file *) (unsigned long) r4_file;
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> + struct path *p1, *p2;
> + struct map_landlock_handle *handle;
> + int i;

As above, please use size_t.


signature.asc
Description: Digital signature