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 <m...@digikod.net> >> Cc: Alexei Starovoitov <a...@kernel.org> >> Cc: Andy Lutomirski <l...@amacapital.net> >> Cc: Daniel Borkmann <dan...@iogearbox.net> >> Cc: David S. Miller <da...@davemloft.net> >> Cc: James Morris <james.l.mor...@oracle.com> >> Cc: Kees Cook <keesc...@chromium.org> >> Cc: Serge E. Hallyn <se...@hallyn.com> >> 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 = &file->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
