* Re: [PATCH net-next v6 08/11] bpf: Add a Landlock sandbox example
From: Kees Cook @ 2017-04-18 23:06 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <20170328234650.19695-9-mic@digikod.net>
On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
> Add a basic sandbox tool to create a process isolated from some part of
> the system. This sandbox create a read-only environment. It is only
> allowed to write to a character device such as a TTY:
>
> # :> X
> # echo $?
> 0
> # ./samples/bpf/landlock1 /bin/sh -i
> Launching a new sandboxed process.
> # :> Y
> cannot create Y: Operation not permitted
>
> Changes since v5:
> * cosmetic fixes
> * rebase
>
> Changes since v4:
> * write Landlock rule in C and compiled it with LLVM
> * remove cgroup handling
> * remove path handling: only handle a read-only environment
> * remove errno return codes
>
> Changes since v3:
> * remove seccomp and origin field: completely free from seccomp programs
> * handle more FS-related hooks
> * handle inode hooks and directory traversal
> * add faked but consistent view thanks to ENOENT
> * add /lib64 in the example
> * fix spelling
> * rename some types and definitions (e.g. SECCOMP_ADD_LANDLOCK_RULE)
>
> Changes since v2:
> * use BPF_PROG_ATTACH for cgroup handling
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> ---
> samples/bpf/Makefile | 4 ++
> samples/bpf/bpf_load.c | 31 +++++++++++--
> samples/bpf/landlock1_kern.c | 46 +++++++++++++++++++
> samples/bpf/landlock1_user.c | 102 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 179 insertions(+), 4 deletions(-)
> create mode 100644 samples/bpf/landlock1_kern.c
> create mode 100644 samples/bpf/landlock1_user.c
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index d42b495b0992..4743674a3fa3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -36,6 +36,7 @@ hostprogs-y += lwt_len_hist
> hostprogs-y += xdp_tx_iptunnel
> hostprogs-y += test_map_in_map
> hostprogs-y += per_socket_stats_example
> +hostprogs-y += landlock1
>
> # Libbpf dependencies
> LIBBPF := ../../tools/lib/bpf/bpf.o
> @@ -76,6 +77,7 @@ lwt_len_hist-objs := bpf_load.o $(LIBBPF) lwt_len_hist_user.o
> xdp_tx_iptunnel-objs := bpf_load.o $(LIBBPF) xdp_tx_iptunnel_user.o
> test_map_in_map-objs := bpf_load.o $(LIBBPF) test_map_in_map_user.o
> per_socket_stats_example-objs := $(LIBBPF) cookie_uid_helper_example.o
> +landlock1-objs := bpf_load.o $(LIBBPF) landlock1_user.o
>
> # Tell kbuild to always build the programs
> always := $(hostprogs-y)
> @@ -111,6 +113,7 @@ always += lwt_len_hist_kern.o
> always += xdp_tx_iptunnel_kern.o
> always += test_map_in_map_kern.o
> always += cookie_uid_helper_example.o
> +always += landlock1_kern.o
>
> HOSTCFLAGS += -I$(objtree)/usr/include
> HOSTCFLAGS += -I$(srctree)/tools/lib/
> @@ -146,6 +149,7 @@ HOSTLOADLIBES_tc_l2_redirect += -l elf
> HOSTLOADLIBES_lwt_len_hist += -l elf
> HOSTLOADLIBES_xdp_tx_iptunnel += -lelf
> HOSTLOADLIBES_test_map_in_map += -lelf
> +HOSTLOADLIBES_landlock1 += -lelf
>
> # Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
> # make samples/bpf/ LLC=~/git/llvm/build/bin/llc CLANG=~/git/llvm/build/bin/clang
> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> index 4a3460d7c01f..3713e5e2e998 100644
> --- a/samples/bpf/bpf_load.c
> +++ b/samples/bpf/bpf_load.c
> @@ -29,6 +29,8 @@
>
> static char license[128];
> static int kern_version;
> +static union bpf_prog_subtype subtype = {};
> +static bool has_subtype;
> static bool processed_sec[128];
> char bpf_log_buf[BPF_LOG_BUF_SIZE];
> int map_fd[MAX_MAPS];
> @@ -68,6 +70,7 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
> bool is_perf_event = strncmp(event, "perf_event", 10) == 0;
> bool is_cgroup_skb = strncmp(event, "cgroup/skb", 10) == 0;
> bool is_cgroup_sk = strncmp(event, "cgroup/sock", 11) == 0;
> + bool is_landlock = strncmp(event, "landlock", 8) == 0;
> size_t insns_cnt = size / sizeof(struct bpf_insn);
> enum bpf_prog_type prog_type;
> char buf[256];
> @@ -94,6 +97,13 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
> prog_type = BPF_PROG_TYPE_CGROUP_SKB;
> } else if (is_cgroup_sk) {
> prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
> + } else if (is_landlock) {
> + prog_type = BPF_PROG_TYPE_LANDLOCK;
> + if (!has_subtype) {
> + printf("No subtype\n");
> + return -1;
> + }
> + st = &subtype;
> } else {
> printf("Unknown event '%s'\n", event);
> return -1;
> @@ -108,7 +118,8 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>
> prog_fd[prog_cnt++] = fd;
>
> - if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk)
> + if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk ||
> + is_landlock)
> return 0;
>
> if (is_socket) {
> @@ -294,6 +305,7 @@ int load_bpf_file(char *path)
> kern_version = 0;
> memset(license, 0, sizeof(license));
> memset(processed_sec, 0, sizeof(processed_sec));
> + has_subtype = false;
>
> if (elf_version(EV_CURRENT) == EV_NONE)
> return 1;
> @@ -339,6 +351,16 @@ int load_bpf_file(char *path)
> processed_sec[i] = true;
> if (load_maps(data->d_buf, data->d_size))
> return 1;
> + } else if (strcmp(shname, "subtype") == 0) {
> + processed_sec[i] = true;
> + if (data->d_size != sizeof(union bpf_prog_subtype)) {
> + printf("invalid size of subtype section %zd\n",
> + data->d_size);
> + return 1;
> + }
> + memcpy(&subtype, data->d_buf,
> + sizeof(union bpf_prog_subtype));
> + has_subtype = true;
> } else if (shdr.sh_type == SHT_SYMTAB) {
> symbols = data;
> }
> @@ -376,14 +398,14 @@ int load_bpf_file(char *path)
> memcmp(shname_prog, "xdp", 3) == 0 ||
> memcmp(shname_prog, "perf_event", 10) == 0 ||
> memcmp(shname_prog, "socket", 6) == 0 ||
> - memcmp(shname_prog, "cgroup/", 7) == 0)
> + memcmp(shname_prog, "cgroup/", 7) == 0 ||
> + memcmp(shname_prog, "landlock", 8) == 0)
> load_and_attach(shname_prog, insns, data_prog->d_size);
> }
> }
>
> /* load programs that don't use maps */
> for (i = 1; i < ehdr.e_shnum; i++) {
> -
> if (processed_sec[i])
> continue;
>
> @@ -396,7 +418,8 @@ int load_bpf_file(char *path)
> memcmp(shname, "xdp", 3) == 0 ||
> memcmp(shname, "perf_event", 10) == 0 ||
> memcmp(shname, "socket", 6) == 0 ||
> - memcmp(shname, "cgroup/", 7) == 0)
> + memcmp(shname, "cgroup/", 7) == 0 ||
> + memcmp(shname, "landlock", 8) == 0)
> load_and_attach(shname, data->d_buf, data->d_size);
> }
>
> diff --git a/samples/bpf/landlock1_kern.c b/samples/bpf/landlock1_kern.c
> new file mode 100644
> index 000000000000..b8a9b0ca84c9
> --- /dev/null
> +++ b/samples/bpf/landlock1_kern.c
> @@ -0,0 +1,46 @@
> +/*
> + * Landlock rule - partial read-only filesystem
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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.
> + */
> +
> +#define KBUILD_MODNAME "foo"
> +#include <uapi/linux/bpf.h>
> +#include <uapi/linux/stat.h> /* S_ISCHR() */
> +#include "bpf_helpers.h"
> +
> +SEC("landlock1")
> +static int landlock_fs_prog1(struct landlock_context *ctx)
Since this is in samples, I think this needs a lot more comments to
describe each pieces, how it fits together, etc. This is where people
are going to come to learn how to use landlock, so making it as clear
as possible is important. This is especially true for each step of the
rule logic. (e.g. some will wonder why is S_ISCHR excluded, etc.)
> +{
> + char fmt_error[] = "landlock1: error: get_mode:%lld\n";
> + char fmt_name[] = "landlock1: syscall:%d\n";
> + long long ret;
> +
> + if (!(ctx->arg2 & LANDLOCK_ACTION_FS_WRITE))
> + return 0;
> + ret = bpf_handle_fs_get_mode((void *)ctx->arg1);
> + if (ret < 0) {
> + bpf_trace_printk(fmt_error, sizeof(fmt_error), ret);
> + return 1;
> + }
> + if (S_ISCHR(ret))
> + return 0;
> + bpf_trace_printk(fmt_name, sizeof(fmt_name), ctx->syscall_nr);
> + return 1;
> +}
> +
> +SEC("subtype")
> +static union bpf_prog_subtype _subtype = {
Can this be const?
> + .landlock_rule = {
> + .version = 1,
> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
> + .ability = LANDLOCK_SUBTYPE_ABILITY_DEBUG,
> + }
> +};
> +
> +SEC("license")
> +static const char _license[] = "GPL";
> diff --git a/samples/bpf/landlock1_user.c b/samples/bpf/landlock1_user.c
> new file mode 100644
> index 000000000000..6f79eb0ee6db
> --- /dev/null
> +++ b/samples/bpf/landlock1_user.c
> @@ -0,0 +1,102 @@
> +/*
> + * Landlock sandbox - partial read-only filesystem
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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 "bpf_load.h"
> +#include "libbpf.h"
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h> /* open() */
> +#include <linux/bpf.h>
> +#include <linux/filter.h>
> +#include <linux/prctl.h>
> +#include <linux/seccomp.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/prctl.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +
> +#ifndef seccomp
> +static int seccomp(unsigned int op, unsigned int flags, void *args)
> +{
> + errno = 0;
> + return syscall(__NR_seccomp, op, flags, args);
> +}
> +#endif
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +#define MAX_ERRNO 4095
Is MAX_ERRNO actually needed?
> +
> +
> +struct landlock_rule {
> + enum landlock_subtype_event event;
> + struct bpf_insn *bpf;
> + size_t size;
> +};
> +
> +static int apply_sandbox(int prog_fd)
> +{
> + int ret = 0;
> +
> + /* set up the test sandbox */
> + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> + perror("prctl(no_new_priv)");
> + return 1;
> + }
> + if (seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &prog_fd)) {
> + perror("seccomp(set_hook)");
> + ret = 1;
> + }
> + close(prog_fd);
> +
> + return ret;
> +}
> +
> +int main(int argc, char * const argv[], char * const *envp)
> +{
> + char filename[256];
> + char *cmd_path;
> + char * const *cmd_argv;
> +
> + if (argc < 2) {
> + fprintf(stderr, "usage: %s <cmd> [args]...\n\n", argv[0]);
> + fprintf(stderr, "Launch a command in a read-only environment "
> + "(except for character devices).\n");
> + fprintf(stderr, "Display debug with: "
> + "cat /sys/kernel/debug/tracing/trace_pipe &\n");
> + return 1;
> + }
> +
> + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> + if (load_bpf_file(filename)) {
> + printf("%s", bpf_log_buf);
> + return 1;
> + }
> + if (!prog_fd[0]) {
> + if (errno) {
> + printf("load_bpf_file: %s\n", strerror(errno));
> + } else {
> + printf("load_bpf_file: Error\n");
> + }
> + return 1;
> + }
> +
> + if (apply_sandbox(prog_fd[0]))
> + return 1;
> + cmd_path = argv[1];
> + cmd_argv = argv + 1;
> + fprintf(stderr, "Launching a new sandboxed process.\n");
> + execve(cmd_path, cmd_argv, envp);
> + perror("execve");
> + return 1;
> +}
I like this example. It's a very powerful rule to for a program to
enforce on itself. :)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net-next] net: ipv6: Add early demux handler for UDP unicast
From: Simon Horman @ 2017-04-18 23:06 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: David Miller, eric.dumazet, subashab, netdev, netdev-owner
In-Reply-To: <1492538948.1052073.948360488.79052FB2@webmail.messagingengine.com>
On Tue, Apr 18, 2017 at 08:09:08PM +0200, Hannes Frederic Sowa wrote:
>
>
> On Tue, Apr 18, 2017, at 17:16, David Miller wrote:
> > From: Simon Horman <simon.horman@netronome.com>
> > Date: Tue, 18 Apr 2017 17:09:04 +0900
> >
> > > On Wed, Mar 08, 2017 at 11:22:01AM -0800, Eric Dumazet wrote:
> > >> On Wed, 2017-03-08 at 12:11 -0700, Subash Abhinov Kasiviswanathan wrote:
> > >> > On 2017-03-08 11:40, Eric Dumazet wrote:
> > >> > > Well, this 'optimization' actually hurts when UDP sockets are not
> > >> > > connected, since this adds an extra cache line miss per incoming
> > >> > > packet.
> > >> > >
> > >> > > (DNS servers for example)
> > >> >
> > >> > Hi Eric
> > >> >
> > >> > Thanks for your comments. Would it be preferable to disable early demux
> > >> > for the
> > >> > servers with large unconnected workloads in that case?
> > >>
> > >> Well, many servers handle both TCP and UDP.
> > >>
> > >> For TCP, there is no question about early demux, this is definitely a
> > >> win.
> > >>
> > >> We probably should have one sysctl to enable TCP early demux, one for
> > >> UDP early demux.
> > >
> > > If early demux is a clear win for TCP then I wonder if it is
> > > unnecessary and by some leap also undesirable to have a configuration
> > > knob for that case.
> >
> > For forwarding workloads it is pure overhead since the early demux will
> > never find a local socket, and therefore it is wasted work.
>
> Also for some more complicated fib rules setups the early demux logic
> could end up causing doing wrong lookups, because some route might not
> be actually local in one fib rule but it is in another one.
Thanks for the clarification. Knobs for both TCP and UDP now make perfect
sense to me.
^ permalink raw reply
* Re: [PATCH v3 net-next RFC] Generic XDP
From: Alexei Starovoitov @ 2017-04-18 23:09 UTC (permalink / raw)
To: Johannes Berg; +Cc: David Miller, netdev, xdp-newbies
In-Reply-To: <1492508879.2472.13.camel@sipsolutions.net>
On Tue, Apr 18, 2017 at 11:47:59AM +0200, Johannes Berg wrote:
> > I guess any driver that would care about XDP performance would either
> > implement in-driver XDP or make sure that skb_linearize() doesn't
> > happen in generic XDP by doing build_skb() with the whole packet.
> > The driver can be smart and avoid doing copy-break if generic XDP is
> > on.
>
> Indeed. I think this is less about performance than ease of
> experimentation.
yes. I'm bringing it up only to hopefully find cases where this
generic XDP can be used for real data path too (and not only experimentation).
Like if it can do 6Mpps whereas in-driver XDP will do 20Mpps (when implemented),
for some folks it may be good enough already to start using it.
Which will put more pressure on NIC vendor to implement it.
^ permalink raw reply
* Re: Corrupted SKB
From: Cong Wang @ 2017-04-18 23:12 UTC (permalink / raw)
To: Michael Ma; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAAmHdhx14ZH3ftE2SovFZfy8ZfOCEgsyqqcSbGPHqg87Wc+5SQ@mail.gmail.com>
On Mon, Apr 17, 2017 at 5:39 PM, Michael Ma <make0818@gmail.com> wrote:
> Hi -
>
> We've implemented a "glue" qdisc similar to mqprio which can associate
> one qdisc to multiple txqs as the root qdisc. Reference count of the
> child qdiscs have been adjusted properly in this case so that it
> represents the number of txqs it has been attached to. However when
> sending packets we saw the skb from dequeue_skb() corrupted with the
> following call stack:
>
> [exception RIP: netif_skb_features+51]
> RIP: ffffffff815292b3 RSP: ffff8817f6987940 RFLAGS: 00010246
>
> #9 [ffff8817f6987968] validate_xmit_skb at ffffffff815294aa
> #10 [ffff8817f69879a0] validate_xmit_skb at ffffffff8152a0d9
> #11 [ffff8817f69879b0] __qdisc_run at ffffffff8154a193
> #12 [ffff8817f6987a00] dev_queue_xmit at ffffffff81529e03
>
> It looks like the skb has already been released since its dev pointer
> field is invalid.
>
> Any clue on how this can be investigated further? My current thought
> is to add some instrumentation to the place where skb is released and
> analyze whether there is any race condition happening there. However
Either dropwatch or perf could do the work to instrument kfree_skb().
> by looking through the existing code I think the case where one root
> qdisc is associated with multiple txqs already exists (when mqprio is
> not used) so not sure why it won't work when we group txqs and assign
> each group a root qdisc. Any insight on this issue would be much
> appreciated!
How do you implement ->attach()? How does it work with netdev_pick_tx()?
^ permalink raw reply
* Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem
From: Casey Schaufler @ 2017-04-18 23:16 UTC (permalink / raw)
To: Mickaël Salaün, Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Daniel Borkmann, David Drysdale,
David S . Miller, Eric W . Biederman, James Morris, Jann Horn,
Jonathan Corbet, Matthew Garrett, Michael Kerrisk, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Tejun Heo,
Thomas Graf, Will Drewry, "kernel-hardening@lists.o
In-Reply-To: <9a69055a-b4cf-00b0-da5e-2e45ff88059c-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
> On 19/04/2017 00:17, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
>>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
>>> event: LANDLOCK_SUBTYPE_EVENT_FS.
>>>
>>> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
>>> struct file, struct path...). Multiple LSM hooks can trigger the same
>>> Landlock event.
>>>
>>> Landlock handle nine coarse-grained actions: read, write, execute, new,
>>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
>>> access control in a way that can be extended in the future.
>>>
>>> The Landlock LSM hook registration is done after other LSM to only run
>>> actions from user-space, via eBPF programs, if the access was granted by
>>> major (privileged) LSMs.
>>>
>>> Changes since v5:
>>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch]
>>> * add more documentation
>>> * cosmetic fixes
>>>
>>> Changes since v4:
>>> * add LSM hook abstraction called Landlock event
>>> * use the compiler type checking to verify hooks use by an event
>>> * handle all filesystem related LSM hooks (e.g. file_permission,
>>> mmap_file, sb_mount...)
>>> * register BPF programs for Landlock just after LSM hooks registration
>>> * move hooks registration after other LSMs
>>> * add failsafes to check if a hook is not used by the kernel
>>> * allow partial raw value access form the context (needed for programs
>>> generated by LLVM)
>>>
>>> Changes since v3:
>>> * split commit
>>> * add hooks dealing with struct inode and struct path pointers:
>>> inode_permission and inode_getattr
>>> * add abstraction over eBPF helper arguments thanks to wrapping structs
>>>
>>> Signed-off-by: Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
>>> Cc: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>>> Cc: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
>>> Cc: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
>>> Cc: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> include/linux/lsm_hooks.h | 5 +
>>> security/landlock/Makefile | 4 +-
>>> security/landlock/hooks.c | 115 +++++++++
>>> security/landlock/hooks.h | 177 ++++++++++++++
>>> security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>>> security/landlock/hooks_fs.h | 19 ++
>>> security/landlock/init.c | 13 +
>>> security/security.c | 7 +-
>>> 8 files changed, 901 insertions(+), 2 deletions(-)
>>> create mode 100644 security/landlock/hooks.c
>>> create mode 100644 security/landlock/hooks.h
>>> create mode 100644 security/landlock/hooks_fs.c
>>> create mode 100644 security/landlock/hooks_fs.h
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index e29d4c62a3c8..884289166a0e 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
>>> #else
>>> static inline void loadpin_add_hooks(void) { };
>>> #endif
>>> +#ifdef CONFIG_SECURITY_LANDLOCK
>>> +extern void __init landlock_add_hooks(void);
>>> +#else
>>> +static inline void __init landlock_add_hooks(void) { }
>>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>>>
>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>> index 7205f9a7a2ee..c0db504a6335 100644
>>> --- a/security/landlock/Makefile
>>> +++ b/security/landlock/Makefile
>>> @@ -1,3 +1,5 @@
>>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function
>> Why is this needed? If it can't be avoided, a comment should exist
>> here explaining why.
> This is useful to catch defined but unused hooks: error out if a
> HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct
> security_hook_list landlock_hooks.
>
>>> [...]
>>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>>> .ops = &bpf_landlock_ops,
>>> .type = BPF_PROG_TYPE_LANDLOCK,
>>> };
>>> +
>>> +void __init landlock_add_hooks(void)
>>> +{
>>> + pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>> + landlock_add_hooks_fs();
>>> + security_add_hooks(NULL, 0, "landlock");
>>> + bpf_register_prog_type(&bpf_landlock_type);
>> I'm confused by the separation of hook registration here. The call to
>> security_add_hooks is with count=0 is especially weird. Why isn't this
>> just a single call with security_add_hooks(landlock_hooks,
>> ARRAY_SIZE(landlock_hooks), "landlock")?
> Yes, this is ugly with the new security_add_hooks() with three arguments
> but I wanted to split the hooks definition in multiple files.
Why? I'll buy a good argument, but there are dangers in
allowing multiple calls to security_add_hooks().
>
> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
> is not exported. Unfortunately, calling multiple security_add_hooks()
> with the same LSM name would register multiple names for the same LSM…
> Is it OK if I modify this function to not add duplicated entries?
It may seem absurd, but it's conceivable that a module might
have two hooks it wants called. My example is a module that
counts the number of times SELinux denies a process access to
things (which needs to be called before and after SELinux in
order to detect denials) and takes "appropriate action" if
too many denials occur. It would be weird, wonky and hackish,
but that never stopped anybody before.
>
>
>>> +}
>>> diff --git a/security/security.c b/security/security.c
>>> index d0e07f269b2d..a3e9f4625991 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -64,10 +64,15 @@ int __init security_init(void)
>>> loadpin_add_hooks();
>>>
>>> /*
>>> - * Load all the remaining security modules.
>>> + * Load all remaining privileged security modules.
>>> */
>>> do_security_initcalls();
>>>
>>> + /*
>>> + * Load potentially-unprivileged security modules at the end.
>>> + */
>>> + landlock_add_hooks();
>> Oh, is this to make it last in the list? Is there a reason it has to be last?
> Right, this is the intend. I'm not sure it is the only way to register
> hooks, though.
>
> For an unprivileged access-control, we don't want to give the ability to
> any process to do some checks, through an eBPF program, on kernel
> objects (e.g. files) if they should not be accessible (because of a
> following LSM hook check).
>
> Mickaël
>
^ permalink raw reply
* Re: [PATCH net-next v6 10/11] bpf,landlock: Add tests for Landlock
From: Kees Cook @ 2017-04-18 23:16 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <20170328234650.19695-11-mic@digikod.net>
On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
> Test basic context access, ptrace protection and filesystem event with
> multiple cases.
>
> Changes since v5:
> * add subtype test
> * add ptrace tests
> * split and rename files
> * cleanup and rebase
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Will Drewry <wad@chromium.org>
> ---
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/bpf/test_verifier.c | 64 +++++
> tools/testing/selftests/landlock/.gitignore | 4 +
> tools/testing/selftests/landlock/Makefile | 47 ++++
> tools/testing/selftests/landlock/rules/Makefile | 52 ++++
> tools/testing/selftests/landlock/rules/README.rst | 1 +
> .../testing/selftests/landlock/rules/bpf_helpers.h | 1 +
> .../testing/selftests/landlock/rules/fs_no_open.c | 31 +++
> .../selftests/landlock/rules/fs_read_only.c | 31 +++
> tools/testing/selftests/landlock/test.h | 35 +++
> tools/testing/selftests/landlock/test_base.c | 31 +++
> tools/testing/selftests/landlock/test_fs.c | 305 +++++++++++++++++++++
> tools/testing/selftests/landlock/test_ptrace.c | 161 +++++++++++
> 13 files changed, 764 insertions(+)
> create mode 100644 tools/testing/selftests/landlock/.gitignore
> create mode 100644 tools/testing/selftests/landlock/Makefile
> create mode 100644 tools/testing/selftests/landlock/rules/Makefile
> create mode 120000 tools/testing/selftests/landlock/rules/README.rst
> create mode 120000 tools/testing/selftests/landlock/rules/bpf_helpers.h
> create mode 100644 tools/testing/selftests/landlock/rules/fs_no_open.c
> create mode 100644 tools/testing/selftests/landlock/rules/fs_read_only.c
> create mode 100644 tools/testing/selftests/landlock/test.h
> create mode 100644 tools/testing/selftests/landlock/test_base.c
> create mode 100644 tools/testing/selftests/landlock/test_fs.c
> create mode 100644 tools/testing/selftests/landlock/test_ptrace.c
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index d8593f1251ec..b584ad456428 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -12,6 +12,7 @@ TARGETS += gpio
> TARGETS += intel_pstate
> TARGETS += ipc
> TARGETS += kcmp
> +TARGETS += landlock
> TARGETS += lib
> TARGETS += membarrier
> TARGETS += memfd
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index daa87dd7c80e..77255b14871e 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -4536,6 +4536,70 @@ static struct bpf_test tests[] = {
> .result = REJECT,
> .has_prog_subtype = true,
> },
> + {
> + "missing subtype",
> + .insns = {
> + BPF_MOV32_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .errstr = "",
> + .result = REJECT,
> + .prog_type = BPF_PROG_TYPE_LANDLOCK,
> + },
> + {
> + "landlock/fs: always accept",
> + .insns = {
> + BPF_MOV32_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_LANDLOCK,
> + .has_prog_subtype = true,
> + .prog_subtype = {
> + .landlock_rule = {
> + .version = 1,
> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
> + }
> + },
> + },
> + {
> + "landlock/fs: read context",
> + .insns = {
> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, status)),
> + /* test operations on raw values */
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, arch)),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, syscall_nr)),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, syscall_cmd)),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, event)),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, arg1)),
> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6,
> + offsetof(struct landlock_context, arg2)),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
> + BPF_MOV32_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_LANDLOCK,
> + .has_prog_subtype = true,
> + .prog_subtype = {
> + .landlock_rule = {
> + .version = 1,
> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
> + }
> + },
> + },
> };
>
> static int probe_filter_length(const struct bpf_insn *fp)
> diff --git a/tools/testing/selftests/landlock/.gitignore b/tools/testing/selftests/landlock/.gitignore
> new file mode 100644
> index 000000000000..25b9cd834c3c
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/.gitignore
> @@ -0,0 +1,4 @@
> +/test_base
> +/test_fs
> +/test_ptrace
> +/tmp_*
> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
> new file mode 100644
> index 000000000000..9a52c82d64fa
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/Makefile
> @@ -0,0 +1,47 @@
> +LIBDIR := ../../../lib
> +BPFOBJ := $(LIBDIR)/bpf/bpf.o
> +LOADOBJ := ../../../../samples/bpf/bpf_load.o
Is the selftest tarball creation tool okay with this? IIRC, it should
be fine since it'll be a built object already, but it's a random
thought I had while looking at this.
> +
> +CFLAGS += -Wl,-no-as-needed -Wall -O2 -I../../../include/uapi -I$(LIBDIR)
> +LDFLAGS += -lelf
> +
> +test_src = $(wildcard test_*.c)
> +rule_src = $(wildcard rules/*.c)
> +
> +test_objs := $(test_src:.c=)
> +rule_objs := $(rule_src:.c=.o)
> +
> +TEST_PROGS := $(test_objs)
> +
> +.PHONY: all clean clean_tmp force
> +
> +all: $(test_objs) $(rule_objs)
> +
> +# force a rebuild of BPFOBJ when its dependencies are updated
> +force:
> +
> +$(BPFOBJ): force
> + $(MAKE) -C $(dir $(BPFOBJ))
> +
> +$(LOADOBJ):
> + $(MAKE) -C $(dir $(LOADOBJ))
> +
> +# minimize builds
> +rules/modules.order: $(rule_src)
> + $(MAKE) -C rules
> + @touch $@
> +
> +$(rule_objs): rules/modules.order
> + @
> +
> +$(test_objs): $(BPFOBJ) $(LOADOBJ)
> +
> +include ../lib.mk
> +
> +clean_tmp:
> + $(RM) -r tmp_*
> +
> +clean: clean_tmp
> + $(MAKE) -C rules clean
> + $(RM) $(test_objs)
> +
> diff --git a/tools/testing/selftests/landlock/rules/Makefile b/tools/testing/selftests/landlock/rules/Makefile
> new file mode 100644
> index 000000000000..8d6ff960ff7c
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/rules/Makefile
> @@ -0,0 +1,52 @@
> +# kbuild trick to avoid linker error. Can be omitted if a module is built.
> +obj- := dummy.o
> +
> +# Tell kbuild to always build the programs
> +always := fs_read_only.o
> +always += fs_no_open.o
> +
> +EXTRA_CFLAGS = -Wall -Wextra
> +
> +# Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
> +# make samples/bpf/ LLC=~/git/llvm/build/bin/llc CLANG=~/git/llvm/build/bin/clang
> +LLC ?= llc
> +CLANG ?= clang
> +
> +# Verify LLVM compiler tools are available and bpf target is supported by llc
> +.PHONY: all clean verify_cmds verify_target_bpf $(CLANG) $(LLC)
> +
> +# Trick to allow make to be run from this directory
> +all:
> + $(MAKE) -C ../../../../../ $(CURDIR)/
> +
> +clean:
> + $(MAKE) -C ../../../../../ M=$(CURDIR) clean
Is this really needed? Others don't have it, I think.
> +verify_cmds: $(CLANG) $(LLC)
> + @for TOOL in $^ ; do \
> + if ! (which -- "$${TOOL}" > /dev/null 2>&1); then \
> + echo "*** ERROR: Cannot find LLVM tool $${TOOL}" ;\
> + exit 1; \
> + else true; fi; \
> + done
> +
> +verify_target_bpf: verify_cmds
> + @if ! (${LLC} -march=bpf -mattr=help > /dev/null 2>&1); then \
> + echo "*** ERROR: LLVM (${LLC}) does not support 'bpf' target" ;\
> + echo " NOTICE: LLVM version >= 3.7.1 required" ;\
> + exit 2; \
> + else true; fi
> +
> +%_kern.c: verify_target_bpf
> +
> +# asm/sysreg.h - inline assembly used by it is incompatible with llvm.
> +# But, there is no easy way to fix it, so just exclude it since it is
> +# useless for BPF samples.
> +$(obj)/%.o: $(src)/%.c
> + $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
> + -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value -Wno-pointer-sign \
> + -Wno-compare-distinct-pointer-types \
> + -Wno-gnu-variable-sized-type-not-at-end \
> + -Wno-tautological-compare \
> + -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf -filetype=obj -o $@
Is clang required for the samples and the selftests? That needs to be
avoided... there needs to be a way to show people how to build a
landlock rule without requiring clang.
> +
> diff --git a/tools/testing/selftests/landlock/rules/README.rst b/tools/testing/selftests/landlock/rules/README.rst
> new file mode 120000
> index 000000000000..605f48aa6f72
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/rules/README.rst
> @@ -0,0 +1 @@
> +../../../../../samples/bpf/README.rst
> \ No newline at end of file
> diff --git a/tools/testing/selftests/landlock/rules/bpf_helpers.h b/tools/testing/selftests/landlock/rules/bpf_helpers.h
> new file mode 120000
> index 000000000000..0aa1a521b39a
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/rules/bpf_helpers.h
> @@ -0,0 +1 @@
> +../../../../../samples/bpf/bpf_helpers.h
> \ No newline at end of file
> diff --git a/tools/testing/selftests/landlock/rules/fs_no_open.c b/tools/testing/selftests/landlock/rules/fs_no_open.c
> new file mode 100644
> index 000000000000..c6ea305e58a7
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/rules/fs_no_open.c
> @@ -0,0 +1,31 @@
> +/*
> + * Landlock rule - no-open filesystem
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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 <uapi/linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +SEC("landlock1")
> +static int landlock_fs_prog1(struct landlock_context *ctx)
> +{
> + if (!(ctx->arg2 & LANDLOCK_ACTION_FS_GET))
> + return 0;
> + return 1;
> +}
> +
> +SEC("subtype")
> +static union bpf_prog_subtype _subtype = {
> + .landlock_rule = {
> + .version = 1,
> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
> + }
> +};
> +
> +SEC("license")
> +static const char _license[] = "GPL";
> diff --git a/tools/testing/selftests/landlock/rules/fs_read_only.c b/tools/testing/selftests/landlock/rules/fs_read_only.c
> new file mode 100644
> index 000000000000..212dda7c0c27
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/rules/fs_read_only.c
> @@ -0,0 +1,31 @@
> +/*
> + * Landlock rule - read-only filesystem
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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 <uapi/linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +SEC("landlock1")
> +static int landlock_fs_prog1(struct landlock_context *ctx)
> +{
> + if (!(ctx->arg2 & LANDLOCK_ACTION_FS_WRITE))
> + return 0;
> + return 1;
> +}
> +
> +SEC("subtype")
> +static union bpf_prog_subtype _subtype = {
> + .landlock_rule = {
> + .version = 1,
> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
> + }
> +};
> +
> +SEC("license")
> +static const char _license[] = "GPL";
> diff --git a/tools/testing/selftests/landlock/test.h b/tools/testing/selftests/landlock/test.h
> new file mode 100644
> index 000000000000..7a194815391b
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/test.h
> @@ -0,0 +1,35 @@
> +/*
> + * Landlock helpers
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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 <errno.h>
> +#include <sys/prctl.h>
> +#include <sys/syscall.h>
> +
> +#include "../seccomp/test_harness.h"
> +#include "../../../../samples/bpf/bpf_load.h"
> +
> +#ifndef SECCOMP_APPEND_LANDLOCK_RULE
> +#define SECCOMP_APPEND_LANDLOCK_RULE 2
> +#endif
> +
> +#ifndef seccomp
> +static int seccomp(unsigned int op, unsigned int flags, void *args)
> +{
> + errno = 0;
> + return syscall(__NR_seccomp, op, flags, args);
> +}
> +#endif
> +
> +#define ASSERT_STEP(cond) \
> + { \
> + step--; \
> + if (!(cond)) \
> + _exit(step); \
> + }
Can you explain this in more detail? I'm assuming there is a problem
with writing to the TH_LOG_STREAM fd or something?
> diff --git a/tools/testing/selftests/landlock/test_base.c b/tools/testing/selftests/landlock/test_base.c
> new file mode 100644
> index 000000000000..bdf056edee03
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/test_base.c
> @@ -0,0 +1,31 @@
> +/*
> + * Landlock tests - base
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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.
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +
> +#include "test.h"
> +
> +TEST(seccomp_landlock)
> +{
> + int ret;
> +
> + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
> + }
> + ret = seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, NULL);
> + EXPECT_EQ(-1, ret);
> + EXPECT_EQ(EFAULT, errno) {
> + TH_LOG("Kernel does not support CONFIG_SECURITY_LANDLOCK");
> + }
> +}
> +
> +TEST_HARNESS_MAIN
> diff --git a/tools/testing/selftests/landlock/test_fs.c b/tools/testing/selftests/landlock/test_fs.c
> new file mode 100644
> index 000000000000..e69eda433716
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/test_fs.c
> @@ -0,0 +1,305 @@
> +/*
> + * Landlock tests - filesystem
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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.
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <linux/bpf.h>
> +#include <linux/filter.h>
> +#include <linux/seccomp.h>
> +#include <stddef.h>
> +#include <string.h>
> +#include <sys/prctl.h>
> +#include <sys/syscall.h>
> +
> +#include <fcntl.h> /* open() */
> +#include <sys/mount.h>
> +#include <sys/stat.h> /* mkdir() */
> +#include <sys/mman.h> /* mmap() */
> +
> +#include "test.h"
> +
> +#define TMP_PREFIX "tmp_"
> +
> +struct layout1 {
> + int file_ro;
> + int file_rw;
> + int file_wo;
> +};
> +
> +static void setup_layout1(struct __test_metadata *_metadata,
> + struct layout1 *l1)
> +{
> + int fd;
> + char buf[] = "fs_read_only";
> +
> + l1->file_ro = -1;
> + l1->file_rw = -1;
> + l1->file_wo = -1;
> +
> + fd = open(TMP_PREFIX "file_created",
> + O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600);
> + ASSERT_GE(fd, 0);
> + ASSERT_EQ(sizeof(buf), write(fd, buf, sizeof(buf)));
> + ASSERT_EQ(0, close(fd));
> +
> + fd = mkdir(TMP_PREFIX "dir_created", 0600);
> + ASSERT_GE(fd, 0);
> + ASSERT_EQ(0, close(fd));
> +
> + l1->file_ro = open(TMP_PREFIX "file_ro",
> + O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600);
> + ASSERT_LE(0, l1->file_ro);
> + ASSERT_EQ(sizeof(buf), write(l1->file_ro, buf, sizeof(buf)));
> + ASSERT_EQ(0, close(l1->file_ro));
> + l1->file_ro = open(TMP_PREFIX "file_ro",
> + O_RDONLY | O_CLOEXEC, 0600);
> + ASSERT_LE(0, l1->file_ro);
> +
> + l1->file_rw = open(TMP_PREFIX "file_rw",
> + O_CREAT | O_EXCL | O_RDWR | O_CLOEXEC, 0600);
> + ASSERT_LE(0, l1->file_rw);
> + ASSERT_EQ(sizeof(buf), write(l1->file_rw, buf, sizeof(buf)));
> + ASSERT_EQ(0, lseek(l1->file_rw, 0, SEEK_SET));
> +
> + l1->file_wo = open(TMP_PREFIX "file_wo",
> + O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600);
> + ASSERT_LE(0, l1->file_wo);
> + ASSERT_EQ(sizeof(buf), write(l1->file_wo, buf, sizeof(buf)));
> + ASSERT_EQ(0, lseek(l1->file_wo, 0, SEEK_SET));
> +}
> +
> +static void cleanup_layout1(void)
> +{
> + unlink(TMP_PREFIX "file_created");
> + unlink(TMP_PREFIX "file_ro");
> + unlink(TMP_PREFIX "file_rw");
> + unlink(TMP_PREFIX "file_wo");
> + unlink(TMP_PREFIX "should_not_exist");
> + rmdir(TMP_PREFIX "dir_created");
> +}
> +
> +FIXTURE(fs_read_only) {
> + struct layout1 l1;
> + int prog;
> +};
> +
> +FIXTURE_SETUP(fs_read_only)
> +{
> + cleanup_layout1();
> + setup_layout1(_metadata, &self->l1);
> +
> + ASSERT_EQ(0, load_bpf_file("rules/fs_read_only.o")) {
> + TH_LOG("%s", bpf_log_buf);
> + }
> + self->prog = prog_fd[0];
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0)) {
> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
> + }
> +}
> +
> +FIXTURE_TEARDOWN(fs_read_only)
> +{
> + EXPECT_EQ(0, close(self->prog));
> + /* cleanup_layout1() would be denied here */
> +}
> +
> +TEST_F(fs_read_only, load_prog) {}
> +
> +TEST_F(fs_read_only, read_only_file)
> +{
> + int fd;
> + int step = 0;
> + char buf_write[] = "should not be written";
> + char buf_read[2];
> +
> + ASSERT_EQ(-1, write(self->l1.file_ro, buf_write, sizeof(buf_write)));
> + ASSERT_EQ(EBADF, errno);
> +
> + ASSERT_EQ(-1, read(self->l1.file_wo, buf_read, sizeof(buf_read)));
> + ASSERT_EQ(EBADF, errno);
> +
> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
> + TH_LOG("Failed to apply rule fs_read_only: %s",
> + strerror(errno));
> + }
> +
> + fd = open(".", O_TMPFILE | O_EXCL | O_RDWR | O_CLOEXEC, 0600);
> + ASSERT_STEP(fd == -1);
> + ASSERT_STEP(errno != EOPNOTSUPP)
> + ASSERT_STEP(errno == EPERM);
> +
> + fd = open(TMP_PREFIX "file_created",
> + O_RDONLY | O_CLOEXEC);
> + ASSERT_STEP(fd >= 0);
> + ASSERT_STEP(!close(fd));
> +
> + fd = open(TMP_PREFIX "file_created",
> + O_RDWR | O_CLOEXEC);
> + ASSERT_STEP(fd == -1);
> + ASSERT_STEP(errno == EPERM);
> +
> + fd = open(TMP_PREFIX "file_created",
> + O_WRONLY | O_CLOEXEC);
> + ASSERT_STEP(fd == -1);
> + ASSERT_STEP(errno == EPERM);
> +
> + fd = open(TMP_PREFIX "should_not_exist",
> + O_CREAT | O_EXCL | O_CLOEXEC, 0600);
> + ASSERT_STEP(fd == -1);
> + ASSERT_STEP(errno == EPERM);
> +
> + ASSERT_STEP(-1 ==
> + write(self->l1.file_ro, buf_write, sizeof(buf_write)));
> + ASSERT_STEP(errno == EBADF);
> + ASSERT_STEP(sizeof(buf_read) ==
> + read(self->l1.file_ro, buf_read, sizeof(buf_read)));
> +
> + ASSERT_STEP(-1 ==
> + write(self->l1.file_rw, buf_write, sizeof(buf_write)));
> + ASSERT_STEP(errno == EPERM);
> + ASSERT_STEP(sizeof(buf_read) ==
> + read(self->l1.file_rw, buf_read, sizeof(buf_read)));
> +
> + ASSERT_STEP(-1 == write(self->l1.file_wo, buf_write, sizeof(buf_write)));
> + ASSERT_STEP(errno == EPERM);
> + ASSERT_STEP(-1 == read(self->l1.file_wo, buf_read, sizeof(buf_read)));
> + ASSERT_STEP(errno == EBADF);
> +
> + ASSERT_STEP(-1 == unlink(TMP_PREFIX "file_created"));
> + ASSERT_STEP(errno == EPERM);
> + ASSERT_STEP(-1 == rmdir(TMP_PREFIX "dir_created"));
> + ASSERT_STEP(errno == EPERM);
> +
> + ASSERT_STEP(0 == close(self->l1.file_ro));
> + ASSERT_STEP(0 == close(self->l1.file_rw));
> + ASSERT_STEP(0 == close(self->l1.file_wo));
> +}
> +
> +TEST_F(fs_read_only, read_only_mount)
> +{
> + int step = 0;
> +
> + ASSERT_EQ(0, mount(".", TMP_PREFIX "dir_created",
> + NULL, MS_BIND, NULL));
> + ASSERT_EQ(0, umount2(TMP_PREFIX "dir_created", MNT_FORCE));
> +
> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
> + TH_LOG("Failed to apply rule fs_read_only: %s",
> + strerror(errno));
> + }
> +
> + ASSERT_STEP(-1 == mount(".", TMP_PREFIX "dir_created",
> + NULL, MS_BIND, NULL));
> + ASSERT_STEP(errno == EPERM);
> + ASSERT_STEP(-1 == umount("/"));
> + ASSERT_STEP(errno == EPERM);
> +}
> +
> +TEST_F(fs_read_only, read_only_mem)
> +{
> + int step = 0;
> + void *addr;
> +
> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE,
> + MAP_SHARED, self->l1.file_rw, 0);
> + ASSERT_NE(NULL, addr);
> + ASSERT_EQ(0, munmap(addr, 1));
> +
> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
> + TH_LOG("Failed to apply rule fs_read_only: %s",
> + strerror(errno));
> + }
> +
> + addr = mmap(NULL, 1, PROT_READ, MAP_SHARED,
> + self->l1.file_rw, 0);
> + ASSERT_STEP(addr != NULL);
> + ASSERT_STEP(-1 == mprotect(addr, 1, PROT_WRITE));
> + ASSERT_STEP(errno == EPERM);
> + ASSERT_STEP(0 == munmap(addr, 1));
> +
> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE, MAP_SHARED,
> + self->l1.file_rw, 0);
> + ASSERT_STEP(addr != NULL);
> + ASSERT_STEP(errno == EPERM);
> +
> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE, MAP_PRIVATE,
> + self->l1.file_rw, 0);
> + ASSERT_STEP(addr != NULL);
> + ASSERT_STEP(0 == munmap(addr, 1));
> +}
> +
> +FIXTURE(fs_no_open) {
> + struct layout1 l1;
> + int prog;
> +};
> +
> +FIXTURE_SETUP(fs_no_open)
> +{
> + cleanup_layout1();
> + setup_layout1(_metadata, &self->l1);
> +
> + ASSERT_EQ(0, load_bpf_file("rules/fs_no_open.o")) {
> + TH_LOG("%s", bpf_log_buf);
> + }
> + self->prog = prog_fd[0];
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0)) {
> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
> + }
> +}
> +
> +FIXTURE_TEARDOWN(fs_no_open)
> +{
> + EXPECT_EQ(0, close(self->prog));
> + cleanup_layout1();
> +}
> +
> +static void landlocked_deny_open(struct __test_metadata *_metadata,
> + struct layout1 *l1)
> +{
> + int fd;
> + void *addr;
> +
> + fd = open(".", O_DIRECTORY | O_CLOEXEC);
> + ASSERT_EQ(-1, fd);
> + ASSERT_EQ(EPERM, errno);
> +
> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE,
> + MAP_SHARED, l1->file_rw, 0);
> + ASSERT_NE(NULL, addr);
> + ASSERT_EQ(0, munmap(addr, 1));
> +}
> +
> +TEST_F(fs_no_open, deny_open_for_hierarchy) {
> + int fd;
> + int status;
> + pid_t child;
> +
> + fd = open(".", O_DIRECTORY | O_CLOEXEC);
> + ASSERT_LE(0, fd);
> + ASSERT_EQ(0, close(fd));
> +
> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
> + TH_LOG("Failed to apply rule fs_no_open: %s", strerror(errno));
> + }
> +
> + landlocked_deny_open(_metadata, &self->l1);
> +
> + child = fork();
> + ASSERT_LE(0, child);
> + if (!child) {
> + landlocked_deny_open(_metadata, &self->l1);
> + _exit(1);
> + }
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + ASSERT_TRUE(WIFEXITED(status));
> + _exit(WEXITSTATUS(status));
> +}
> +
> +TEST_HARNESS_MAIN
> diff --git a/tools/testing/selftests/landlock/test_ptrace.c b/tools/testing/selftests/landlock/test_ptrace.c
> new file mode 100644
> index 000000000000..0c940a7fd3d0
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/test_ptrace.c
> @@ -0,0 +1,161 @@
> +/*
> + * Landlock tests - ptrace
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * 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.
> + */
> +
> +#define _GNU_SOURCE
> +#include <signal.h> /* raise */
> +#include <sys/ptrace.h>
> +#include <sys/types.h> /* waitpid */
> +#include <sys/wait.h> /* waitpid */
> +#include <unistd.h> /* fork, pipe */
> +
> +#include "test.h"
> +
> +static void apply_null_sandbox(struct __test_metadata *_metadata)
> +{
> + const struct bpf_insn prog_accept[] = {
> + BPF_MOV32_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + };
> + const union bpf_prog_subtype subtype = {
> + .landlock_rule = {
> + .version = 1,
> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
> + }
> + };
> + int prog;
> + char log[256] = "";
> +
> + prog = bpf_load_program(BPF_PROG_TYPE_LANDLOCK,
> + (const struct bpf_insn *)&prog_accept,
> + sizeof(prog_accept) / sizeof(struct bpf_insn), "GPL",
> + 0, log, sizeof(log), &subtype);
> + ASSERT_NE(-1, prog) {
> + TH_LOG("Failed to load minimal rule: %s\n%s",
> + strerror(errno), log);
> + }
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0)) {
> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
> + }
> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &prog)) {
> + TH_LOG("Failed to apply minimal rule: %s", strerror(errno));
> + }
> + EXPECT_EQ(0, close(prog));
> +}
> +
> +/* PTRACE_TRACEME and PTRACE_ATTACH without Landlock rules effect */
> +static void check_ptrace(struct __test_metadata *_metadata,
> + int sandbox_both, int sandbox_parent, int sandbox_child,
> + int expect_ptrace)
> +{
> + pid_t child;
> + int status;
> + int pipefd[2];
> +
> + ASSERT_EQ(0, pipe(pipefd));
> + if (sandbox_both)
> + apply_null_sandbox(_metadata);
> +
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + char buf;
> +
> + EXPECT_EQ(0, close(pipefd[1]));
> + if (sandbox_child)
> + apply_null_sandbox(_metadata);
> +
> + /* test traceme */
> + ASSERT_EQ(expect_ptrace, ptrace(PTRACE_TRACEME));
> + if (expect_ptrace) {
> + ASSERT_EQ(EPERM, errno);
> + } else {
> + ASSERT_EQ(0, raise(SIGSTOP));
> + }
> +
> + /* sync */
> + ASSERT_EQ(1, read(pipefd[0], &buf, 1)) {
> + TH_LOG("Failed to read() sync from parent");
> + }
> + ASSERT_EQ('.', buf);
> + _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
> + }
> +
> + EXPECT_EQ(0, close(pipefd[0]));
> + if (sandbox_parent)
> + apply_null_sandbox(_metadata);
> +
> + /* test traceme */
> + if (!expect_ptrace) {
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + ASSERT_EQ(1, WIFSTOPPED(status));
> + ASSERT_EQ(0, ptrace(PTRACE_DETACH, child, NULL, 0));
> + }
> + /* test attach */
> + ASSERT_EQ(expect_ptrace, ptrace(PTRACE_ATTACH, child, NULL, 0));
> + if (expect_ptrace) {
> + ASSERT_EQ(EPERM, errno);
> + } else {
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + ASSERT_EQ(1, WIFSTOPPED(status));
> + ASSERT_EQ(0, ptrace(PTRACE_CONT, child, NULL, 0));
> + }
> +
> + /* sync */
> + ASSERT_EQ(1, write(pipefd[1], ".", 1)) {
> + TH_LOG("Failed to write() sync to child");
> + }
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + if (WIFSIGNALED(status) || WEXITSTATUS(status))
> + _metadata->passed = 0;
> +}
> +
> +TEST(ptrace_allow_without_sandbox)
> +{
> + /* no sandbox */
> + check_ptrace(_metadata, 0, 0, 0, 0);
> +}
> +
> +TEST(ptrace_allow_with_one_sandbox)
> +{
> + /* child sandbox */
> + check_ptrace(_metadata, 0, 0, 1, 0);
> +}
> +
> +TEST(ptrace_allow_with_nested_sandbox)
> +{
> + /* inherited and child sandbox */
> + check_ptrace(_metadata, 1, 0, 1, 0);
> +}
> +
> +TEST(ptrace_deny_with_parent_sandbox)
> +{
> + /* parent sandbox */
> + check_ptrace(_metadata, 0, 1, 0, -1);
> +}
> +
> +TEST(ptrace_deny_with_nested_and_parent_sandbox)
> +{
> + /* inherited and parent sandbox */
> + check_ptrace(_metadata, 1, 1, 0, -1);
> +}
> +
> +TEST(ptrace_deny_with_forked_sandbox)
> +{
> + /* inherited, parent and child sandbox */
> + check_ptrace(_metadata, 1, 1, 1, -1);
> +}
> +
> +TEST(ptrace_deny_with_sibling_sandbox)
> +{
> + /* parent and child sandbox */
> + check_ptrace(_metadata, 0, 1, 1, -1);
> +}
> +
> +TEST_HARNESS_MAIN
> --
> 2.11.0
>
Awesome. I love to see all these tests, with both positive and
negative checks. Nice!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* RE: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
From: Woojung.Huh @ 2017-04-18 23:18 UTC (permalink / raw)
To: eric.dumazet; +Cc: james.hughes, netdev, steve.glendinning, UNGLinuxDriver
In-Reply-To: <1492555561.10587.147.camel@edumazet-glaptop3.roam.corp.google.com>
> if (info->tx_fixup) {
> skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>
> if (!skb) { // Note that skb is NULL now
You are right. skb is return value of tx_fixup().
- Woojung
^ permalink raw reply
* Re: net: heap out-of-bounds in fib6_clean_node/rt6_fill_node/fib6_age/fib6_prune_clone
From: David Ahern @ 2017-04-18 23:20 UTC (permalink / raw)
To: Andrey Konovalov, Dmitry Vyukov
Cc: Eric Dumazet, Mahesh Bandewar, Eric Dumazet, David Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Cong Wang, syzkaller
In-Reply-To: <CAAeHK+zx6W_yxnoEQ2Pc1AT4uLXqYZaoi5oQBeuCntdGS38TQg@mail.gmail.com>
On 4/18/17 2:43 PM, Andrey Konovalov wrote:
> I've finally managed to reproduce one of the crashes on commit
> 4f7d029b9bf009fbee76bb10c0c4351a1870d2f3 (4.11-rc7).
>
> I'm not sure if this bug has the same root cause as the first one
> reported in this thread, but it definitely has to do with ipv6
> routing.
>
> C reproducer, syzkaller program and my .config are attached.
>
> Thanks!
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN
> Modules linked in:
> CPU: 1 PID: 4035 Comm: a.out Not tainted 4.11.0-rc7+ #250
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff880069809600 task.stack: ffff880062dc8000
> RIP: 0010:ip6_rt_cache_alloc+0xa6/0x560 net/ipv6/route.c:975
>From a quick glance seems to be a different bug than Dmitry's.
^ permalink raw reply
* Re: [PATCH net-next v6 06/11] seccomp,landlock: Handle Landlock events per process hierarchy
From: Mickaël Salaün @ 2017-04-18 23:24 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <CAGXu5jJ36E8L9=sh62vPy+0t84Bx4n4Y9eiYZU+DtUZom1vpCQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 26816 bytes --]
On 19/04/2017 00:53, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> The seccomp(2) syscall can be used by a task to apply a Landlock rule to
>> itself. As a seccomp filter, a Landlock rule is enforced for the current
>> task and all its future children. A rule is immutable and a task can
>> only add new restricting rules to itself, forming a chain of rules.
>>
>> A Landlock rule is tied to a Landlock event. If the use of a kernel
>> object is allowed by the other Linux security mechanisms (e.g. DAC,
>> capabilities, other LSM), then a Landlock event related to this kind of
>> object is triggered. The chain of rules for this event is then
>> evaluated. Each rule return a 32-bit value which can deny the use of a
>> kernel object with a non-zero value. If every rules of the chain return
>> zero, then the use of the object is allowed.
>>
>> Changes since v5:
>> * remove struct landlock_node and use a similar inheritance mechanisme
>> as seccomp-bpf (requested by Andy Lutomirski)
>> * rename SECCOMP_ADD_LANDLOCK_RULE to SECCOMP_APPEND_LANDLOCK_RULE
>> * rename file manager.c to providers.c
>> * add comments
>> * typo and cosmetic fixes
>>
>> Changes since v4:
>> * merge manager and seccomp patches
>> * return -EFAULT in seccomp(2) when user_bpf_fd is null to easely check
>> if Landlock is supported
>> * only allow a process with the global CAP_SYS_ADMIN to use Landlock
>> (will be lifted in the future)
>> * add an early check to exit as soon as possible if the current process
>> does not have Landlock rules
>>
>> Changes since v3:
>> * remove the hard link with seccomp (suggested by Andy Lutomirski and
>> Kees Cook):
>> * remove the cookie which could imply multiple evaluation of Landlock
>> rules
>> * remove the origin field in struct landlock_data
>> * remove documentation fix (merged upstream)
>> * rename the new seccomp command to SECCOMP_ADD_LANDLOCK_RULE
>> * internal renaming
>> * split commit
>> * new design to be able to inherit on the fly the parent rules
>>
>> Changes since v2:
>> * Landlock programs can now be run without seccomp filter but for any
>> syscall (from the process) or interruption
>> * move Landlock related functions and structs into security/landlock/*
>> (to manage cgroups as well)
>> * fix seccomp filter handling: run Landlock programs for each of their
>> legitimate seccomp filter
>> * properly clean up all seccomp results
>> * cosmetic changes to ease the understanding
>> * fix some ifdef
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: James Morris <james.l.morris@oracle.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> Cc: Will Drewry <wad@chromium.org>
>> Link: https://lkml.kernel.org/r/c10a503d-5e35-7785-2f3d-25ed8dd63fab@digikod.net
>> ---
>> include/linux/landlock.h | 36 +++++++
>> include/linux/seccomp.h | 8 ++
>> include/uapi/linux/seccomp.h | 1 +
>> kernel/fork.c | 14 ++-
>> kernel/seccomp.c | 8 ++
>> security/landlock/Makefile | 2 +-
>> security/landlock/hooks.c | 37 +++++++
>> security/landlock/hooks.h | 5 +
>> security/landlock/init.c | 3 +-
>> security/landlock/providers.c | 232 ++++++++++++++++++++++++++++++++++++++++++
>> 10 files changed, 342 insertions(+), 4 deletions(-)
>> create mode 100644 security/landlock/providers.c
>>
>> diff --git a/include/linux/landlock.h b/include/linux/landlock.h
>> index 53013dc374fe..c40ee78e86e0 100644
>> --- a/include/linux/landlock.h
>> +++ b/include/linux/landlock.h
>> @@ -12,6 +12,9 @@
>> #define _LINUX_LANDLOCK_H
>> #ifdef CONFIG_SECURITY_LANDLOCK
>>
>> +#include <linux/bpf.h> /* _LANDLOCK_SUBTYPE_EVENT_LAST */
>> +#include <linux/types.h> /* atomic_t */
>> +
>> /*
>> * This is not intended for the UAPI headers. Each userland software should use
>> * a static minimal version for the required features as explained in the
>> @@ -19,5 +22,38 @@
>> */
>> #define LANDLOCK_VERSION 1
>>
>> +struct landlock_rule {
>> + atomic_t usage;
>
> This should be refcount_t. (And I should convert seccomp to use
> refcount_t too!) :)
OK
>
>> + struct landlock_rule *prev;
>> + struct bpf_prog *prog;
>> +};
>> +
>> +/**
>> + * struct landlock_events - Landlock event rules enforced on a thread
>> + *
>> + * This is used for low performance impact when forking a process. Instead of
>> + * copying the full array and incrementing the usage of each entries, only
>> + * create a pointer to &struct landlock_events and increments its usage. When
>> + * appending a new rule, if &struct landlock_events is shared with other tasks,
>> + * then duplicate it and append the rule to this new &struct landlock_events.
>> + *
>> + * @usage: reference count to manage the object lifetime. When a thread need to
>> + * add Landlock rules and if @usage is greater than 1, then the thread
>> + * must duplicate &struct landlock_events to not change the children's
>> + * rules as well.
>> + * @rules: array of non-NULL &struct landlock_rule pointers
>> + */
>> +struct landlock_events {
>> + atomic_t usage;
>> + struct landlock_rule *rules[_LANDLOCK_SUBTYPE_EVENT_LAST];
>> +};
>> +
>> +void put_landlock_events(struct landlock_events *events);
>> +
>> +#ifdef CONFIG_SECCOMP_FILTER
>
> Isn't CONFIG_SECCOMP_FILTER already required for landlock?
Yes it is, but Landlock could only/also be used through cgroups in the
future. :)
>
>> +int landlock_seccomp_append_prog(unsigned int flags,
>> + const char __user *user_bpf_fd);
>> +#endif /* CONFIG_SECCOMP_FILTER */
>> +
>> #endif /* CONFIG_SECURITY_LANDLOCK */
>> #endif /* _LINUX_LANDLOCK_H */
>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>> index e25aee2cdfc0..9a38de3c0e72 100644
>> --- a/include/linux/seccomp.h
>> +++ b/include/linux/seccomp.h
>> @@ -10,6 +10,10 @@
>> #include <linux/thread_info.h>
>> #include <asm/seccomp.h>
>>
>> +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_SECURITY_LANDLOCK)
>> +struct landlock_events;
>> +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_SECURITY_LANDLOCK */
>
> Testing LANDLOCK should be sufficient, since it requires ..._FILTER.
>
>> +
>> struct seccomp_filter;
>> /**
>> * struct seccomp - the state of a seccomp'ed process
>> @@ -18,6 +22,7 @@ struct seccomp_filter;
>> * system calls available to a process.
>> * @filter: must always point to a valid seccomp-filter or NULL as it is
>> * accessed without locking during system call entry.
>> + * @landlock_events: contains an array of Landlock rules.
>> *
>> * @filter must only be accessed from the context of current as there
>> * is no read locking.
>> @@ -25,6 +30,9 @@ struct seccomp_filter;
>> struct seccomp {
>> int mode;
>> struct seccomp_filter *filter;
>> +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_SECURITY_LANDLOCK)
>> + struct landlock_events *landlock_events;
>> +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_SECURITY_LANDLOCK */
>
> Same.
>
>> };
>>
>> #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a43ff1e..74891cf60ca6 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -13,6 +13,7 @@
>> /* Valid operations for seccomp syscall. */
>> #define SECCOMP_SET_MODE_STRICT 0
>> #define SECCOMP_SET_MODE_FILTER 1
>> +#define SECCOMP_APPEND_LANDLOCK_RULE 2
>>
>> /* Valid flags for SECCOMP_SET_MODE_FILTER */
>> #define SECCOMP_FILTER_FLAG_TSYNC 1
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index a27d8e67ce33..14c09486c565 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -47,6 +47,7 @@
>> #include <linux/security.h>
>> #include <linux/hugetlb.h>
>> #include <linux/seccomp.h>
>> +#include <linux/landlock.h>
>> #include <linux/swap.h>
>> #include <linux/syscalls.h>
>> #include <linux/jiffies.h>
>> @@ -528,7 +529,10 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
>> * the usage counts on the error path calling free_task.
>> */
>> tsk->seccomp.filter = NULL;
>> -#endif
>> +#ifdef CONFIG_SECURITY_LANDLOCK
>> + tsk->seccomp.landlock_events = NULL;
>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>> +#endif /* CONFIG_SECCOMP */
>>
>> setup_thread_stack(tsk, orig);
>> clear_user_return_notifier(tsk);
>> @@ -1405,7 +1409,13 @@ static void copy_seccomp(struct task_struct *p)
>>
>> /* Ref-count the new filter user, and assign it. */
>> get_seccomp_filter(current);
>> - p->seccomp = current->seccomp;
>> + p->seccomp.mode = current->seccomp.mode;
>> + p->seccomp.filter = current->seccomp.filter;
>> +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_SECURITY_LANDLOCK)
>> + p->seccomp.landlock_events = current->seccomp.landlock_events;
>> + if (p->seccomp.landlock_events)
>> + atomic_inc(&p->seccomp.landlock_events->usage);
>> +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_SECURITY_LANDLOCK */
>
> Hrm. So, this needs config cleanup as above. Also, I'm going to need
> some help understanding the usage tracking on landlock_events (which
> should use a get/put rather than open-coding the _inc). I don't see
> why individual assignments are needed here. The only thing that
> matters is the usage bump. I would have expected no changes at all in
> this code, actually. The filter and the events share the same usage
> don't they?
Right, I can move the struct landlock_event into the struct
seccomp_filter. This should make the code cleaner.
>
>> /*
>> * Explicitly enable no_new_privs here in case it got set
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 326f79e32127..d122829e6da1 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -34,6 +34,7 @@
>> #include <linux/security.h>
>> #include <linux/tracehook.h>
>> #include <linux/uaccess.h>
>> +#include <linux/landlock.h>
>>
>> /**
>> * struct seccomp_filter - container for seccomp BPF programs
>> @@ -494,6 +495,9 @@ static void put_seccomp_filter(struct seccomp_filter *filter)
>> void put_seccomp(struct task_struct *tsk)
>> {
>> put_seccomp_filter(tsk->seccomp.filter);
>> +#ifdef CONFIG_SECURITY_LANDLOCK
>> + put_landlock_events(tsk->seccomp.landlock_events);
>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>
> put_landlock_events() should be defined in a header file to be a
> static inline no-op if ..._LANDLOCK isn't defined. That way we can
> avoid all the #ifdef stuff here. Similarly, I think all of these
> should take just task_struct so that there isn't an exposed structure
> name which lets you avoid the #ifdef too.
Right
>
>> }
>>
>> static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
>> @@ -813,6 +817,10 @@ static long do_seccomp(unsigned int op, unsigned int flags,
>> return seccomp_set_mode_strict();
>> case SECCOMP_SET_MODE_FILTER:
>> return seccomp_set_mode_filter(flags, uargs);
>> +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_SECURITY_LANDLOCK)
>> + case SECCOMP_APPEND_LANDLOCK_RULE:
>> + return landlock_seccomp_append_prog(flags, uargs);
>> +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_SECURITY_LANDLOCK */
>> default:
>> return -EINVAL;
>> }
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index c0db504a6335..da8ba8b5183e 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -2,4 +2,4 @@ ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function
>>
>> obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>
>> -landlock-y := init.o hooks.o hooks_fs.o
>> +landlock-y := init.o providers.o hooks.o hooks_fs.o
>> diff --git a/security/landlock/hooks.c b/security/landlock/hooks.c
>> index eaee8162ff70..4fa7d0b38d41 100644
>> --- a/security/landlock/hooks.c
>> +++ b/security/landlock/hooks.c
>> @@ -95,6 +95,38 @@ bool landlock_is_valid_access(int off, int size, enum bpf_access_type type,
>> return true;
>> }
>>
>> +/**
>> + * landlock_event_deny - run Landlock rules tied to an event
>> + *
>> + * @event_idx: event index in the rules array
>> + * @ctx: non-NULL eBPF context
>> + * @events: Landlock events pointer
>> + *
>> + * Return true if at least one rule deny the event.
>> + */
>> +static bool landlock_event_deny(u32 event_idx, const struct landlock_context *ctx,
>> + struct landlock_events *events)
>> +{
>> + struct landlock_rule *rule;
>> +
>> + if (!events)
>> + return false;
>> +
>> + for (rule = events->rules[event_idx]; rule; rule = rule->prev) {
>> + u32 ret;
>> +
>> + if (WARN_ON(!rule->prog))
>> + continue;
>> + rcu_read_lock();
>> + ret = BPF_PROG_RUN(rule->prog, (void *)ctx);
>> + rcu_read_unlock();
>> + /* deny access if a program returns a value different than 0 */
>> + if (ret)
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> int landlock_decide(enum landlock_subtype_event event,
>> __u64 ctx_values[CTX_ARG_NB], u32 cmd, const char *hook)
>> {
>> @@ -111,5 +143,10 @@ int landlock_decide(enum landlock_subtype_event event,
>> .arg2 = ctx_values[1],
>> };
>>
>> +#ifdef CONFIG_SECCOMP_FILTER
>> + deny = landlock_event_deny(event_idx, &ctx,
>> + current->seccomp.landlock_events);
>> +#endif /* CONFIG_SECCOMP_FILTER */
>
> Isn't ..._FILTER required?
Same as above, this is required for now but could change in the near
future. :)
>
>> +
>> return deny ? -EPERM : 0;
>> }
>> diff --git a/security/landlock/hooks.h b/security/landlock/hooks.h
>> index 2e180f6ed86b..dd0486a4c284 100644
>> --- a/security/landlock/hooks.h
>> +++ b/security/landlock/hooks.h
>> @@ -12,6 +12,7 @@
>> #include <linux/bpf.h> /* enum bpf_access_type */
>> #include <linux/lsm_hooks.h>
>> #include <linux/sched.h> /* struct task_struct */
>> +#include <linux/seccomp.h>
>>
>> /* separators */
>> #define SEP_COMMA() ,
>> @@ -163,7 +164,11 @@ WRAP_TYPE_RAW_C;
>>
>> static inline bool landlocked(const struct task_struct *task)
>> {
>> +#ifdef CONFIG_SECCOMP_FILTER
>> + return !!(task->seccomp.landlock_events);
>> +#else
>> return false;
>> +#endif /* CONFIG_SECCOMP_FILTER */
>> }
>>
>> __init void landlock_register_hooks(struct security_hook_list *hooks, int count);
>> diff --git a/security/landlock/init.c b/security/landlock/init.c
>> index 1c2750e12dfa..ef8a3da69860 100644
>> --- a/security/landlock/init.c
>> +++ b/security/landlock/init.c
>> @@ -135,7 +135,8 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>>
>> void __init landlock_add_hooks(void)
>> {
>> - pr_info("landlock: Version %u", LANDLOCK_VERSION);
>> + pr_info("landlock: Version %u, ready to sandbox with %s\n",
>> + LANDLOCK_VERSION, "seccomp");
>> landlock_add_hooks_fs();
>> security_add_hooks(NULL, 0, "landlock");
>> bpf_register_prog_type(&bpf_landlock_type);
>> diff --git a/security/landlock/providers.c b/security/landlock/providers.c
>> new file mode 100644
>> index 000000000000..6d867a39c947
>> --- /dev/null
>> +++ b/security/landlock/providers.c
>> @@ -0,0 +1,232 @@
>> +/*
>> + * Landlock LSM - seccomp provider
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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 <asm/page.h> /* PAGE_SIZE */
>> +#include <linux/atomic.h> /* atomic_*(), smp_store_release() */
>> +#include <linux/bpf.h> /* bpf_prog_put() */
>> +#include <linux/filter.h> /* struct bpf_prog */
>> +#include <linux/kernel.h> /* round_up() */
>> +#include <linux/landlock.h>
>> +#include <linux/sched.h> /* current_cred(), task_no_new_privs() */
>> +#include <linux/security.h> /* security_capable_noaudit() */
>> +#include <linux/slab.h> /* alloc(), kfree() */
>> +#include <linux/types.h> /* atomic_t */
>> +#include <linux/uaccess.h> /* copy_from_user() */
>> +
>> +#include "common.h"
>> +
>> +static void put_landlock_rule(struct landlock_rule *rule)
>> +{
>> + struct landlock_rule *orig = rule;
>> +
>> + /* clean up single-reference branches iteratively */
>> + while (orig && atomic_dec_and_test(&orig->usage)) {
>> + struct landlock_rule *freeme = orig;
>> +
>> + bpf_prog_put(orig->prog);
>> + orig = orig->prev;
>> + kfree(freeme);
>> + }
>> +}
>> +
>> +void put_landlock_events(struct landlock_events *events)
>> +{
>> + if (events && atomic_dec_and_test(&events->usage)) {
>> + size_t i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(events->rules); i++)
>> + /* XXX: Do we need to use lockless_dereference() here? */
>> + put_landlock_rule(events->rules[i]);
>> + kfree(events);
>> + }
>> +}
>> +
>> +static struct landlock_events *new_landlock_events(void)
>> +{
>> + struct landlock_events *ret;
>> +
>> + /* array filled with NULL values */
>> + ret = kzalloc(sizeof(*ret), GFP_KERNEL);
>> + if (!ret)
>> + return ERR_PTR(-ENOMEM);
>> + atomic_set(&ret->usage, 1);
>> + return ret;
>> +}
>> +
>> +static void add_landlock_rule(struct landlock_events *events,
>> + struct landlock_rule *rule)
>> +{
>> + /* subtype.landlock_rule.event > 0 for loaded programs */
>> + u32 event_idx = get_index(rule->prog->subtype.landlock_rule.event);
>> +
>> + rule->prev = events->rules[event_idx];
>> + WARN_ON(atomic_read(&rule->usage));
>> + atomic_set(&rule->usage, 1);
>> + /* do not increment the previous rule usage */
>> + smp_store_release(&events->rules[event_idx], rule);
>> +}
>> +
>> +/* limit Landlock events to 256KB */
>> +#define LANDLOCK_EVENTS_MAX_PAGES (1 << 6)
>> +
>> +/**
>> + * landlock_append_prog - attach a Landlock rule to @current_events
>> + *
>> + * @current_events: landlock_events pointer, must be locked (if needed) to
>> + * prevent a concurrent put/free. This pointer must not be
>> + * freed after the call.
>> + * @prog: non-NULL Landlock rule to append to @current_events. @prog will be
>> + * owned by landlock_append_prog() and freed if an error happened.
>> + *
>> + * Return @current_events or a new pointer when OK. Return a pointer error
>> + * otherwise.
>> + */
>> +static struct landlock_events *landlock_append_prog(
>> + struct landlock_events *current_events, struct bpf_prog *prog)
>> +{
>> + struct landlock_events *new_events = current_events;
>> + unsigned long pages;
>> + struct landlock_rule *rule;
>> + u32 event_idx;
>> +
>> + if (prog->type != BPF_PROG_TYPE_LANDLOCK) {
>> + new_events = ERR_PTR(-EINVAL);
>> + goto put_prog;
>> + }
>> +
>> + /* validate memory size allocation */
>> + pages = prog->pages;
>> + if (current_events) {
>> + size_t i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(current_events->rules); i++) {
>> + struct landlock_rule *walker_r;
>> +
>> + for (walker_r = current_events->rules[i]; walker_r;
>> + walker_r = walker_r->prev)
>> + pages += walker_r->prog->pages;
>> + }
>> + /* count a struct landlock_events if we need to allocate one */
>> + if (atomic_read(¤t_events->usage) != 1)
>> + pages += round_up(sizeof(*current_events), PAGE_SIZE) /
>> + PAGE_SIZE;
>> + }
>> + if (pages > LANDLOCK_EVENTS_MAX_PAGES) {
>> + new_events = ERR_PTR(-E2BIG);
>> + goto put_prog;
>> + }
>> +
>> + rule = kzalloc(sizeof(*rule), GFP_KERNEL);
>> + if (!rule) {
>> + new_events = ERR_PTR(-ENOMEM);
>> + goto put_prog;
>> + }
>> + rule->prog = prog;
>> +
>> + /* subtype.landlock_rule.event > 0 for loaded programs */
>> + event_idx = get_index(rule->prog->subtype.landlock_rule.event);
>> +
>> + if (!new_events) {
>> + /*
>> + * If there is no Landlock events used by the current task,
>> + * then create a new one.
>> + */
>> + new_events = new_landlock_events();
>> + if (IS_ERR(new_events))
>> + goto put_rule;
>
> Shouldn't bpf_prog_put() get called in the face of a rule failure too?
> Why separate exit paths?
You're right but put_landlock_rule() call bpf_prog_put() by itself.
>
>> + } else if (atomic_read(¤t_events->usage) > 1) {
>> + /*
>> + * If the current task is not the sole user of its Landlock
>> + * events, then duplicate them.
>> + */
>> + size_t i;
>> +
>> + new_events = new_landlock_events();
>> + if (IS_ERR(new_events))
>> + goto put_rule;
>> + for (i = 0; i < ARRAY_SIZE(new_events->rules); i++) {
>> + new_events->rules[i] =
>> + lockless_dereference(current_events->rules[i]);
>> + if (new_events->rules[i])
>> + atomic_inc(&new_events->rules[i]->usage);
>
> I was going to ask: isn't the top-level usage counter sufficient to
> track rule lifetime? But I think I see how things are tracked now:
> each task_struct points to an array of rule list pointers. These
> tables are duplicated when additions are made (which means each table
> needs to be refcounted for the processes using it), and when a new
> table is created, all the refcounters on the rules are bumped (to
> track each table that references the rule), and when a new rule is
> added, it's just prepended to the list for the new table to point at.
That's right.
>
> Does this mean that rules are processed in reverse?
Yes, the rules are processed from the newest to the oldest, as
seccomp-bpf does with filters.
>
>> + }
>> +
>> + /*
>> + * Landlock events from the current task will not be freed here
>> + * because the usage is strictly greater than 1. It is only
>> + * prevented to be freed by another subject thanks to the
>> + * caller of landlock_append_prog() which should be locked if
>> + * needed.
>> + */
>> + put_landlock_events(current_events);
>> + }
>> + add_landlock_rule(new_events, rule);
>> + return new_events;
>> +
>> +put_prog:
>> + bpf_prog_put(prog);
>> + return new_events;
>> +
>> +put_rule:
>> + put_landlock_rule(rule);
>> + return new_events;
>> +}
>> +
>> +/**
>> + * landlock_seccomp_append_prog - attach a Landlock rule to the current process
>> + *
>> + * current->seccomp.landlock_events is lazily allocated. When a process fork,
>> + * only a pointer is copied. When a new event is added by a process, if there
>> + * is other references to this process' landlock_events, then a new allocation
>> + * is made to contain an array pointing to Landlock rule lists. This design
>> + * enable low-performance impact and is memory efficient while keeping the
>> + * property of append-only rules.
>> + *
>> + * @flags: not used for now, but could be used for TSYNC
>> + * @user_bpf_fd: file descriptor pointing to a loaded Landlock rule
>> + */
>> +#ifdef CONFIG_SECCOMP_FILTER
>> +int landlock_seccomp_append_prog(unsigned int flags,
>> + const char __user *user_bpf_fd)
>> +{
>> + struct landlock_events *new_events;
>> + struct bpf_prog *prog;
>> + int bpf_fd;
>> +
>> + /* force no_new_privs to limit privilege escalation */
>> + if (!task_no_new_privs(current))
>> + return -EPERM;
>> + /* will be removed in the future to allow unprivileged tasks */
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> + /* enable to check if Landlock is supported with early EFAULT */
>> + if (!user_bpf_fd)
>> + return -EFAULT;
>> + if (flags)
>> + return -EINVAL;
>> + if (copy_from_user(&bpf_fd, user_bpf_fd, sizeof(bpf_fd)))
>> + return -EFAULT;
>
> I think this can just be get_user()?
Yes, I didn't know about that.
>
>> + prog = bpf_prog_get(bpf_fd);
>> + if (IS_ERR(prog))
>> + return PTR_ERR(prog);
>> +
>> + /*
>> + * We don't need to lock anything for the current process hierarchy,
>> + * everything is guarded by the atomic counters.
>> + */
>> + new_events = landlock_append_prog(current->seccomp.landlock_events,
>> + prog);
>> + /* @prog is managed/freed by landlock_append_prog() */
>
> Does kmemcheck notice this "leak"? (i.e. is further annotation needed?)
I didn't enable kmemcheck, I will take a look at it.
>
>> + if (IS_ERR(new_events))
>> + return PTR_ERR(new_events);
>> + current->seccomp.landlock_events = new_events;
>> + return 0;
>> +}
>> +#endif /* CONFIG_SECCOMP_FILTER */
>> --
>> 2.11.0
>>
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes
From: Samuel Ortiz @ 2017-04-18 23:24 UTC (permalink / raw)
To: Johan Hovold; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20170418100916.GB2823@localhost>
Hi Johan,
On Tue, Apr 18, 2017 at 12:09:16PM +0200, Johan Hovold wrote:
> On Thu, Mar 30, 2017 at 12:15:34PM +0200, Johan Hovold wrote:
> > This started out with the observation that the nfcmrvl_uart driver
> > unconditionally dereferenced the tty class device despite the fact that
> > not every tty has an associated struct device (Unix98 ptys). Some
> > further changes were needed in the common nfcmrvl code to fully address
> > this, some of which also incidentally fixed a few related bugs (e.g.
> > resource leaks in error paths).
> >
> > While fixing this I stumbled over a regression in NFC core that lead to
> > broken registration error paths and misnamed workqueues.
> >
> > Note that this has only been tested by configuring the n_hci line
> > discipline for different ttys without any actual NFC hardware connected.
>
> > Johan Hovold (8):
> > NFC: fix broken device allocation
> > NFC: nfcmrvl_uart: add missing tty-device sanity check
> > NFC: nfcmrvl: do not use device-managed resources
> > NFC: nfcmrvl: use nfc-device for firmware download
> > NFC: nfcmrvl: fix firmware-management initialisation
> > NFC: nfcmrvl_uart: fix device-node leak during probe
> > NFC: nfcmrvl_usb: use interface as phy device
> > NFC: nfcmrvl: allow gpio 0 for reset signalling
>
> Any chance of getting these into 4.12, Samuel?
I have yours and Mark Greer patchset pending. I'll push them to nfc-next
and see if I can send another pull request to Dave by the end of this
week.
Cheers,
Samuel.
^ permalink raw reply
* Re: [PATCH net-next v6 00/11] Landlock LSM: Toward unprivileged sandboxing
From: Kees Cook @ 2017-04-18 23:26 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <20170328234650.19695-1-mic@digikod.net>
On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
> This sixth series add some changes to the previous one [1], including a simpler
> rule inheritance hierarchy (similar to seccomp-bpf), a ptrace scope protection,
> some file renaming (better feature identification per file), a future-proof
> eBPF subtype and miscellaneous cosmetic fixes.
Sorry for the delay in review! I finally had a chunk of time I could
devote to this. I really like how it's heading. I still wonder about
its overlap with seccomp (it's really only using the syscall now...),
but that's just a detail. Getting the abstraction away from direct LSM
hooks looks good.
> There is as yet no way to allow a process to access only a subset of the
> filesystem where the subset is specified via a path or a file descriptor. This
> feature is intentionally left out so as to minimize the amount of code of this
> patch series but will come in a following series. However, it is possible to
> check the file type, as done in the following example.
I understand why you've taken a progressive approach here, but I think
there are two fundamental areas where people will use Landlock: path
evaluation and network address evaluation. I think it's worth
expanding this series to include those two confinement examples, since
that can help people understand what the "general" case will look
like.
As I mentioned in one of the patch review emails, I think there needs
to be a clearer explanation of how usage counting works vs the
"events" (which I think of as "rule tables" not events -- maybe it
needs a new name?) and "rule" lists. I think I understand it, but I
spent a lot of time trying to get there. More comments would help.
Finally, another thing I'm curious about is how to deal with the
thread-sync issue. Seccomp uses its TSYNC thing, and I'd expect we'd
want something similar for landlock... but that looks really hairy as
far as locking goes. Perhaps it's already solved by using the same
locking seccomp uses, in which case I'm less inclined to kick landlock
out of seccomp.c. :)
Looks like it's coming along nicely! Thanks for continuing to work on this!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: question about size of sk_buff and skb_shared_info
From: Code Soldier1 @ 2017-04-18 23:26 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1492542021.10587.137.camel@edumazet-glaptop3.roam.corp.google.com>
Eric,
This alignment flag is passed to the cache constructor and the
allocation is indeed cache aligned. However, since the allocated size
is not a multiple of the alignment, wont memory be wasted ?. We can
get 40 extra bytes without any side effects since they are on the same
cache line ?
kmem_cache_create() code does an ALIGN() to round up the size.
kasan_cache_create(cachep, &size, &flags);
size = ALIGN(size, cachep->align);
This is the size used in calculate_slab_order() to calculate num. I am
assuming that in the non debug case gfp_order will be 0.
Perhaps I am missing something.
On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
>> Hi Folks,
>>
>> I am sure there is a reason for the current sizes of these structures,
>> However the reason is not obvious to me. So please help me understand.
>>
>> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
>> is that. I expected it to be a multiple of 32/64 as they are the most
>> common cache lines. Since the alignment calculation will align the
>> structure with the hw cache line, it seems like we might be wasting
>> space ?
>>
>> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
>>
>> Thanks,
>>
>
> The alignment is there.
> Look at skb_init() code, using SLAB_HWCACHE_ALIGN
>
>
>
>
>
--
CS1
^ permalink raw reply
* Re: [PATCH net-next v6 08/11] bpf: Add a Landlock sandbox example
From: Mickaël Salaün @ 2017-04-18 23:35 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <CAGXu5jKThQ7n=t=eu4i4dx9u-NUmAPS-m8oNNEaCe3jH+MZfBw@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 14194 bytes --]
On 19/04/2017 01:06, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> Add a basic sandbox tool to create a process isolated from some part of
>> the system. This sandbox create a read-only environment. It is only
>> allowed to write to a character device such as a TTY:
>>
>> # :> X
>> # echo $?
>> 0
>> # ./samples/bpf/landlock1 /bin/sh -i
>> Launching a new sandboxed process.
>> # :> Y
>> cannot create Y: Operation not permitted
>>
>> Changes since v5:
>> * cosmetic fixes
>> * rebase
>>
>> Changes since v4:
>> * write Landlock rule in C and compiled it with LLVM
>> * remove cgroup handling
>> * remove path handling: only handle a read-only environment
>> * remove errno return codes
>>
>> Changes since v3:
>> * remove seccomp and origin field: completely free from seccomp programs
>> * handle more FS-related hooks
>> * handle inode hooks and directory traversal
>> * add faked but consistent view thanks to ENOENT
>> * add /lib64 in the example
>> * fix spelling
>> * rename some types and definitions (e.g. SECCOMP_ADD_LANDLOCK_RULE)
>>
>> Changes since v2:
>> * use BPF_PROG_ATTACH for cgroup handling
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: James Morris <james.l.morris@oracle.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> ---
>> samples/bpf/Makefile | 4 ++
>> samples/bpf/bpf_load.c | 31 +++++++++++--
>> samples/bpf/landlock1_kern.c | 46 +++++++++++++++++++
>> samples/bpf/landlock1_user.c | 102 +++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 179 insertions(+), 4 deletions(-)
>> create mode 100644 samples/bpf/landlock1_kern.c
>> create mode 100644 samples/bpf/landlock1_user.c
>>
>> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
>> index d42b495b0992..4743674a3fa3 100644
>> --- a/samples/bpf/Makefile
>> +++ b/samples/bpf/Makefile
>> @@ -36,6 +36,7 @@ hostprogs-y += lwt_len_hist
>> hostprogs-y += xdp_tx_iptunnel
>> hostprogs-y += test_map_in_map
>> hostprogs-y += per_socket_stats_example
>> +hostprogs-y += landlock1
>>
>> # Libbpf dependencies
>> LIBBPF := ../../tools/lib/bpf/bpf.o
>> @@ -76,6 +77,7 @@ lwt_len_hist-objs := bpf_load.o $(LIBBPF) lwt_len_hist_user.o
>> xdp_tx_iptunnel-objs := bpf_load.o $(LIBBPF) xdp_tx_iptunnel_user.o
>> test_map_in_map-objs := bpf_load.o $(LIBBPF) test_map_in_map_user.o
>> per_socket_stats_example-objs := $(LIBBPF) cookie_uid_helper_example.o
>> +landlock1-objs := bpf_load.o $(LIBBPF) landlock1_user.o
>>
>> # Tell kbuild to always build the programs
>> always := $(hostprogs-y)
>> @@ -111,6 +113,7 @@ always += lwt_len_hist_kern.o
>> always += xdp_tx_iptunnel_kern.o
>> always += test_map_in_map_kern.o
>> always += cookie_uid_helper_example.o
>> +always += landlock1_kern.o
>>
>> HOSTCFLAGS += -I$(objtree)/usr/include
>> HOSTCFLAGS += -I$(srctree)/tools/lib/
>> @@ -146,6 +149,7 @@ HOSTLOADLIBES_tc_l2_redirect += -l elf
>> HOSTLOADLIBES_lwt_len_hist += -l elf
>> HOSTLOADLIBES_xdp_tx_iptunnel += -lelf
>> HOSTLOADLIBES_test_map_in_map += -lelf
>> +HOSTLOADLIBES_landlock1 += -lelf
>>
>> # Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
>> # make samples/bpf/ LLC=~/git/llvm/build/bin/llc CLANG=~/git/llvm/build/bin/clang
>> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
>> index 4a3460d7c01f..3713e5e2e998 100644
>> --- a/samples/bpf/bpf_load.c
>> +++ b/samples/bpf/bpf_load.c
>> @@ -29,6 +29,8 @@
>>
>> static char license[128];
>> static int kern_version;
>> +static union bpf_prog_subtype subtype = {};
>> +static bool has_subtype;
>> static bool processed_sec[128];
>> char bpf_log_buf[BPF_LOG_BUF_SIZE];
>> int map_fd[MAX_MAPS];
>> @@ -68,6 +70,7 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>> bool is_perf_event = strncmp(event, "perf_event", 10) == 0;
>> bool is_cgroup_skb = strncmp(event, "cgroup/skb", 10) == 0;
>> bool is_cgroup_sk = strncmp(event, "cgroup/sock", 11) == 0;
>> + bool is_landlock = strncmp(event, "landlock", 8) == 0;
>> size_t insns_cnt = size / sizeof(struct bpf_insn);
>> enum bpf_prog_type prog_type;
>> char buf[256];
>> @@ -94,6 +97,13 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>> prog_type = BPF_PROG_TYPE_CGROUP_SKB;
>> } else if (is_cgroup_sk) {
>> prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
>> + } else if (is_landlock) {
>> + prog_type = BPF_PROG_TYPE_LANDLOCK;
>> + if (!has_subtype) {
>> + printf("No subtype\n");
>> + return -1;
>> + }
>> + st = &subtype;
>> } else {
>> printf("Unknown event '%s'\n", event);
>> return -1;
>> @@ -108,7 +118,8 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>>
>> prog_fd[prog_cnt++] = fd;
>>
>> - if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk)
>> + if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk ||
>> + is_landlock)
>> return 0;
>>
>> if (is_socket) {
>> @@ -294,6 +305,7 @@ int load_bpf_file(char *path)
>> kern_version = 0;
>> memset(license, 0, sizeof(license));
>> memset(processed_sec, 0, sizeof(processed_sec));
>> + has_subtype = false;
>>
>> if (elf_version(EV_CURRENT) == EV_NONE)
>> return 1;
>> @@ -339,6 +351,16 @@ int load_bpf_file(char *path)
>> processed_sec[i] = true;
>> if (load_maps(data->d_buf, data->d_size))
>> return 1;
>> + } else if (strcmp(shname, "subtype") == 0) {
>> + processed_sec[i] = true;
>> + if (data->d_size != sizeof(union bpf_prog_subtype)) {
>> + printf("invalid size of subtype section %zd\n",
>> + data->d_size);
>> + return 1;
>> + }
>> + memcpy(&subtype, data->d_buf,
>> + sizeof(union bpf_prog_subtype));
>> + has_subtype = true;
>> } else if (shdr.sh_type == SHT_SYMTAB) {
>> symbols = data;
>> }
>> @@ -376,14 +398,14 @@ int load_bpf_file(char *path)
>> memcmp(shname_prog, "xdp", 3) == 0 ||
>> memcmp(shname_prog, "perf_event", 10) == 0 ||
>> memcmp(shname_prog, "socket", 6) == 0 ||
>> - memcmp(shname_prog, "cgroup/", 7) == 0)
>> + memcmp(shname_prog, "cgroup/", 7) == 0 ||
>> + memcmp(shname_prog, "landlock", 8) == 0)
>> load_and_attach(shname_prog, insns, data_prog->d_size);
>> }
>> }
>>
>> /* load programs that don't use maps */
>> for (i = 1; i < ehdr.e_shnum; i++) {
>> -
>> if (processed_sec[i])
>> continue;
>>
>> @@ -396,7 +418,8 @@ int load_bpf_file(char *path)
>> memcmp(shname, "xdp", 3) == 0 ||
>> memcmp(shname, "perf_event", 10) == 0 ||
>> memcmp(shname, "socket", 6) == 0 ||
>> - memcmp(shname, "cgroup/", 7) == 0)
>> + memcmp(shname, "cgroup/", 7) == 0 ||
>> + memcmp(shname, "landlock", 8) == 0)
>> load_and_attach(shname, data->d_buf, data->d_size);
>> }
>>
>> diff --git a/samples/bpf/landlock1_kern.c b/samples/bpf/landlock1_kern.c
>> new file mode 100644
>> index 000000000000..b8a9b0ca84c9
>> --- /dev/null
>> +++ b/samples/bpf/landlock1_kern.c
>> @@ -0,0 +1,46 @@
>> +/*
>> + * Landlock rule - partial read-only filesystem
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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.
>> + */
>> +
>> +#define KBUILD_MODNAME "foo"
>> +#include <uapi/linux/bpf.h>
>> +#include <uapi/linux/stat.h> /* S_ISCHR() */
>> +#include "bpf_helpers.h"
>> +
>> +SEC("landlock1")
>> +static int landlock_fs_prog1(struct landlock_context *ctx)
>
> Since this is in samples, I think this needs a lot more comments to
> describe each pieces, how it fits together, etc. This is where people
> are going to come to learn how to use landlock, so making it as clear
> as possible is important. This is especially true for each step of the
> rule logic. (e.g. some will wonder why is S_ISCHR excluded, etc.)
Right, I already extended a bit this example with a S_ISFIFO() and some
comments. There is also the Documentation/.../user.rst which should help
understand how it works.
>
>> +{
>> + char fmt_error[] = "landlock1: error: get_mode:%lld\n";
>> + char fmt_name[] = "landlock1: syscall:%d\n";
>> + long long ret;
>> +
>> + if (!(ctx->arg2 & LANDLOCK_ACTION_FS_WRITE))
>> + return 0;
>> + ret = bpf_handle_fs_get_mode((void *)ctx->arg1);
>> + if (ret < 0) {
>> + bpf_trace_printk(fmt_error, sizeof(fmt_error), ret);
>> + return 1;
>> + }
>> + if (S_ISCHR(ret))
>> + return 0;
>> + bpf_trace_printk(fmt_name, sizeof(fmt_name), ctx->syscall_nr);
>> + return 1;
>> +}
>> +
>> +SEC("subtype")
>> +static union bpf_prog_subtype _subtype = {
>
> Can this be const?
Yes
>
>> + .landlock_rule = {
>> + .version = 1,
>> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
>> + .ability = LANDLOCK_SUBTYPE_ABILITY_DEBUG,
>> + }
>> +};
>> +
>> +SEC("license")
>> +static const char _license[] = "GPL";
>> diff --git a/samples/bpf/landlock1_user.c b/samples/bpf/landlock1_user.c
>> new file mode 100644
>> index 000000000000..6f79eb0ee6db
>> --- /dev/null
>> +++ b/samples/bpf/landlock1_user.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Landlock sandbox - partial read-only filesystem
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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 "bpf_load.h"
>> +#include "libbpf.h"
>> +
>> +#define _GNU_SOURCE
>> +#include <errno.h>
>> +#include <fcntl.h> /* open() */
>> +#include <linux/bpf.h>
>> +#include <linux/filter.h>
>> +#include <linux/prctl.h>
>> +#include <linux/seccomp.h>
>> +#include <stddef.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/prctl.h>
>> +#include <sys/syscall.h>
>> +#include <unistd.h>
>> +
>> +#ifndef seccomp
>> +static int seccomp(unsigned int op, unsigned int flags, void *args)
>> +{
>> + errno = 0;
>> + return syscall(__NR_seccomp, op, flags, args);
>> +}
>> +#endif
>> +
>> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>> +#define MAX_ERRNO 4095
>
> Is MAX_ERRNO actually needed?
Not anymore.
>
>> +
>> +
>> +struct landlock_rule {
>> + enum landlock_subtype_event event;
>> + struct bpf_insn *bpf;
>> + size_t size;
>> +};
>> +
>> +static int apply_sandbox(int prog_fd)
>> +{
>> + int ret = 0;
>> +
>> + /* set up the test sandbox */
>> + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
>> + perror("prctl(no_new_priv)");
>> + return 1;
>> + }
>> + if (seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &prog_fd)) {
>> + perror("seccomp(set_hook)");
>> + ret = 1;
>> + }
>> + close(prog_fd);
>> +
>> + return ret;
>> +}
>> +
>> +int main(int argc, char * const argv[], char * const *envp)
>> +{
>> + char filename[256];
>> + char *cmd_path;
>> + char * const *cmd_argv;
>> +
>> + if (argc < 2) {
>> + fprintf(stderr, "usage: %s <cmd> [args]...\n\n", argv[0]);
>> + fprintf(stderr, "Launch a command in a read-only environment "
>> + "(except for character devices).\n");
>> + fprintf(stderr, "Display debug with: "
>> + "cat /sys/kernel/debug/tracing/trace_pipe &\n");
>> + return 1;
>> + }
>> +
>> + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
>> + if (load_bpf_file(filename)) {
>> + printf("%s", bpf_log_buf);
>> + return 1;
>> + }
>> + if (!prog_fd[0]) {
>> + if (errno) {
>> + printf("load_bpf_file: %s\n", strerror(errno));
>> + } else {
>> + printf("load_bpf_file: Error\n");
>> + }
>> + return 1;
>> + }
>> +
>> + if (apply_sandbox(prog_fd[0]))
>> + return 1;
>> + cmd_path = argv[1];
>> + cmd_argv = argv + 1;
>> + fprintf(stderr, "Launching a new sandboxed process.\n");
>> + execve(cmd_path, cmd_argv, envp);
>> + perror("execve");
>> + return 1;
>> +}
>
> I like this example. It's a very powerful rule to for a program to
> enforce on itself. :)
>
> -Kees
>
Thanks!
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem
From: Kees Cook @ 2017-04-18 23:39 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net>
On Tue, Apr 18, 2017 at 3:44 PM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 19/04/2017 00:17, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
>>> event: LANDLOCK_SUBTYPE_EVENT_FS.
>>>
>>> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
>>> struct file, struct path...). Multiple LSM hooks can trigger the same
>>> Landlock event.
>>>
>>> Landlock handle nine coarse-grained actions: read, write, execute, new,
>>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
>>> access control in a way that can be extended in the future.
>>>
>>> The Landlock LSM hook registration is done after other LSM to only run
>>> actions from user-space, via eBPF programs, if the access was granted by
>>> major (privileged) LSMs.
>>>
>>> Changes since v5:
>>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch]
>>> * add more documentation
>>> * cosmetic fixes
>>>
>>> Changes since v4:
>>> * add LSM hook abstraction called Landlock event
>>> * use the compiler type checking to verify hooks use by an event
>>> * handle all filesystem related LSM hooks (e.g. file_permission,
>>> mmap_file, sb_mount...)
>>> * register BPF programs for Landlock just after LSM hooks registration
>>> * move hooks registration after other LSMs
>>> * add failsafes to check if a hook is not used by the kernel
>>> * allow partial raw value access form the context (needed for programs
>>> generated by LLVM)
>>>
>>> Changes since v3:
>>> * split commit
>>> * add hooks dealing with struct inode and struct path pointers:
>>> inode_permission and inode_getattr
>>> * add abstraction over eBPF helper arguments thanks to wrapping structs
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Cc: James Morris <james.l.morris@oracle.com>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>> ---
>>> include/linux/lsm_hooks.h | 5 +
>>> security/landlock/Makefile | 4 +-
>>> security/landlock/hooks.c | 115 +++++++++
>>> security/landlock/hooks.h | 177 ++++++++++++++
>>> security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>>> security/landlock/hooks_fs.h | 19 ++
>>> security/landlock/init.c | 13 +
>>> security/security.c | 7 +-
>>> 8 files changed, 901 insertions(+), 2 deletions(-)
>>> create mode 100644 security/landlock/hooks.c
>>> create mode 100644 security/landlock/hooks.h
>>> create mode 100644 security/landlock/hooks_fs.c
>>> create mode 100644 security/landlock/hooks_fs.h
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index e29d4c62a3c8..884289166a0e 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
>>> #else
>>> static inline void loadpin_add_hooks(void) { };
>>> #endif
>>> +#ifdef CONFIG_SECURITY_LANDLOCK
>>> +extern void __init landlock_add_hooks(void);
>>> +#else
>>> +static inline void __init landlock_add_hooks(void) { }
>>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>>>
>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>> index 7205f9a7a2ee..c0db504a6335 100644
>>> --- a/security/landlock/Makefile
>>> +++ b/security/landlock/Makefile
>>> @@ -1,3 +1,5 @@
>>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function
>>
>> Why is this needed? If it can't be avoided, a comment should exist
>> here explaining why.
>
> This is useful to catch defined but unused hooks: error out if a
> HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct
> security_hook_list landlock_hooks.
Gotcha. Please convert into a comment for the next revision. :)
>
>>
>>> [...]
>>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>>> .ops = &bpf_landlock_ops,
>>> .type = BPF_PROG_TYPE_LANDLOCK,
>>> };
>>> +
>>> +void __init landlock_add_hooks(void)
>>> +{
>>> + pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>> + landlock_add_hooks_fs();
>>> + security_add_hooks(NULL, 0, "landlock");
>>> + bpf_register_prog_type(&bpf_landlock_type);
>>
>> I'm confused by the separation of hook registration here. The call to
>> security_add_hooks is with count=0 is especially weird. Why isn't this
>> just a single call with security_add_hooks(landlock_hooks,
>> ARRAY_SIZE(landlock_hooks), "landlock")?
>
> Yes, this is ugly with the new security_add_hooks() with three arguments
> but I wanted to split the hooks definition in multiple files.
>
> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
> is not exported. Unfortunately, calling multiple security_add_hooks()
> with the same LSM name would register multiple names for the same LSM…
> Is it OK if I modify this function to not add duplicated entries?
I'm not sure which would be more sane: a single table (constructed
from header-defined function declarations), or changes to this core
area.
>>> diff --git a/security/security.c b/security/security.c
>>> index d0e07f269b2d..a3e9f4625991 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -64,10 +64,15 @@ int __init security_init(void)
>>> loadpin_add_hooks();
>>>
>>> /*
>>> - * Load all the remaining security modules.
>>> + * Load all remaining privileged security modules.
>>> */
>>> do_security_initcalls();
>>>
>>> + /*
>>> + * Load potentially-unprivileged security modules at the end.
>>> + */
>>> + landlock_add_hooks();
>>
>> Oh, is this to make it last in the list? Is there a reason it has to be last?
>
> Right, this is the intend. I'm not sure it is the only way to register
> hooks, though.
>
> For an unprivileged access-control, we don't want to give the ability to
> any process to do some checks, through an eBPF program, on kernel
> objects (e.g. files) if they should not be accessible (because of a
> following LSM hook check).
Ah right, yes. I thought some of that was already going to be handled
by getting handles to things, but yeah, best to run last. (If you can
expand the comment above the call to landlock_add_hooks() that would
be great.)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem
From: Kees Cook @ 2017-04-18 23:40 UTC (permalink / raw)
To: Casey Schaufler
Cc: Mickaël Salaün, LKML, Alexei Starovoitov,
Andy Lutomirski, Arnaldo Carvalho de Melo, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf
In-Reply-To: <c122415d-c2be-d013-efb4-833ba87d144d@schaufler-ca.com>
On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
>> On 19/04/2017 00:17, Kees Cook wrote:
>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> +void __init landlock_add_hooks(void)
>>>> +{
>>>> + pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>> + landlock_add_hooks_fs();
>>>> + security_add_hooks(NULL, 0, "landlock");
>>>> + bpf_register_prog_type(&bpf_landlock_type);
>>> I'm confused by the separation of hook registration here. The call to
>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>> just a single call with security_add_hooks(landlock_hooks,
>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>> Yes, this is ugly with the new security_add_hooks() with three arguments
>> but I wanted to split the hooks definition in multiple files.
>
> Why? I'll buy a good argument, but there are dangers in
> allowing multiple calls to security_add_hooks().
>
>>
>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>> is not exported. Unfortunately, calling multiple security_add_hooks()
>> with the same LSM name would register multiple names for the same LSM…
>> Is it OK if I modify this function to not add duplicated entries?
>
> It may seem absurd, but it's conceivable that a module might
> have two hooks it wants called. My example is a module that
> counts the number of times SELinux denies a process access to
> things (which needs to be called before and after SELinux in
> order to detect denials) and takes "appropriate action" if
> too many denials occur. It would be weird, wonky and hackish,
> but that never stopped anybody before.
If ends up being sane and clear, I'm fine with allowing multiple calls.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH 1/5] netlink: extended ACK reporting
From: Joe Stringer @ 2017-04-18 23:46 UTC (permalink / raw)
To: Johannes Berg
Cc: ovs dev, Jiri Pirko, netdev,
linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jiri Benc,
Jamal Hadi Salim, David Ahern, Nicolas Dichtel, Pablo Neira Ayuso
In-Reply-To: <1492508494.2472.11.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
On 18 April 2017 at 02:41, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
> On Thu, 2017-04-13 at 14:44 -0700, Joe Stringer wrote
> (something that never made it to the list, due to HTML formatting)
>>
>> I think that OVS was doing some more elaborate validation than most
>> users, so over time we picked up a bunch of extra parsing code that
>> layers on top of nla_parse(). I took a look at trying to broaden this
>> and make it useful to other users a while ago, but when I posted
>> there wasn't much interest from others on it so I just moved on.
>> Maybe it's about time to pick that back up.
>
> Ah, ok. I didn't realize it was actually on top of nla_parse(). Some of
> this does seem rather useful though, and having more expressive policy
> would seem very useful too - I'd love to be able to express nesting
> better, for example.
Ah, correction - nla_parse() is used in some parts but not all of it.
More expressive policy sounds useful for OVS cases too.
^ permalink raw reply
* Re: [PATCH net-next v6 06/11] seccomp,landlock: Handle Landlock events per process hierarchy
From: Kees Cook @ 2017-04-18 23:48 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <a3c71579-4238-0967-b61d-522859f740aa-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
On Tue, Apr 18, 2017 at 4:24 PM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
> On 19/04/2017 00:53, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> +#ifdef CONFIG_SECCOMP_FILTER
>>
>> Isn't CONFIG_SECCOMP_FILTER already required for landlock?
>
> Yes it is, but Landlock could only/also be used through cgroups in the
> future. :)
Hm, okay. I still feel like the configs could be more sensible. :)
>>> @@ -1405,7 +1409,13 @@ static void copy_seccomp(struct task_struct *p)
>>>
>>> /* Ref-count the new filter user, and assign it. */
>>> get_seccomp_filter(current);
>>> - p->seccomp = current->seccomp;
>>> + p->seccomp.mode = current->seccomp.mode;
>>> + p->seccomp.filter = current->seccomp.filter;
>>> +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_SECURITY_LANDLOCK)
>>> + p->seccomp.landlock_events = current->seccomp.landlock_events;
>>> + if (p->seccomp.landlock_events)
>>> + atomic_inc(&p->seccomp.landlock_events->usage);
>>> +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_SECURITY_LANDLOCK */
>>
>> Hrm. So, this needs config cleanup as above. Also, I'm going to need
>> some help understanding the usage tracking on landlock_events (which
>> should use a get/put rather than open-coding the _inc). I don't see
>> why individual assignments are needed here. The only thing that
>> matters is the usage bump. I would have expected no changes at all in
>> this code, actually. The filter and the events share the same usage
>> don't they?
>
> Right, I can move the struct landlock_event into the struct
> seccomp_filter. This should make the code cleaner.
No, that wasn't my point. I meant that since landlock_events is
already in ->seccomp, it's already copied by p->seccomp =
current->seccomp. The only thing you need is a
get_seccomp_landlock(current) call before the copy:
get_seccomp_filter(current);
get_seccomp_landlock(current);
p->seccomp = current->seccomp;
done! :)
And get_seccomp_landlock() can do a check for landlock_events existing, etc etc.
>>> + if (!new_events) {
>>> + /*
>>> + * If there is no Landlock events used by the current task,
>>> + * then create a new one.
>>> + */
>>> + new_events = new_landlock_events();
>>> + if (IS_ERR(new_events))
>>> + goto put_rule;
>>
>> Shouldn't bpf_prog_put() get called in the face of a rule failure too?
>> Why separate exit paths?
>
> You're right but put_landlock_rule() call bpf_prog_put() by itself.
Ah! Missed that, thanks!
>>> + } else if (atomic_read(¤t_events->usage) > 1) {
>>> + /*
>>> + * If the current task is not the sole user of its Landlock
>>> + * events, then duplicate them.
>>> + */
>>> + size_t i;
>>> +
>>> + new_events = new_landlock_events();
>>> + if (IS_ERR(new_events))
>>> + goto put_rule;
>>> + for (i = 0; i < ARRAY_SIZE(new_events->rules); i++) {
>>> + new_events->rules[i] =
>>> + lockless_dereference(current_events->rules[i]);
>>> + if (new_events->rules[i])
>>> + atomic_inc(&new_events->rules[i]->usage);
>>
>> I was going to ask: isn't the top-level usage counter sufficient to
>> track rule lifetime? But I think I see how things are tracked now:
>> each task_struct points to an array of rule list pointers. These
>> tables are duplicated when additions are made (which means each table
>> needs to be refcounted for the processes using it), and when a new
>> table is created, all the refcounters on the rules are bumped (to
>> track each table that references the rule), and when a new rule is
>> added, it's just prepended to the list for the new table to point at.
>
> That's right.
Okay, excellent. This should end up in a comment somewhere so when I
forget I can go read it again. ;)
>> Does this mean that rules are processed in reverse?
>
> Yes, the rules are processed from the newest to the oldest, as
> seccomp-bpf does with filters.
Cool. If not already mentioned, that should end up in the docs somewhere.
>>> + if (copy_from_user(&bpf_fd, user_bpf_fd, sizeof(bpf_fd)))
>>> + return -EFAULT;
>>
>> I think this can just be get_user()?
>
> Yes, I didn't know about that.
No worries. It's nice for small things. :)
>>> + prog = bpf_prog_get(bpf_fd);
>>> + if (IS_ERR(prog))
>>> + return PTR_ERR(prog);
>>> +
>>> + /*
>>> + * We don't need to lock anything for the current process hierarchy,
>>> + * everything is guarded by the atomic counters.
>>> + */
>>> + new_events = landlock_append_prog(current->seccomp.landlock_events,
>>> + prog);
>>> + /* @prog is managed/freed by landlock_append_prog() */
>>
>> Does kmemcheck notice this "leak"? (i.e. is further annotation needed?)
>
> I didn't enable kmemcheck, I will take a look at it.
Yeah, I'd turn on at least these while you're testing:
CONFIG_PROVE_LOCKING=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_DEBUG_KMEMLEAK=y
I'm sure people will suggest more, too. :)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net-next v6 10/11] bpf,landlock: Add tests for Landlock
From: Mickaël Salaün @ 2017-04-18 23:53 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <CAGXu5j+-sFD5z2daPjvM0Q8vM=g9ZtwJ7-ZakqoUcjsiAD=+1A@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 34260 bytes --]
On 19/04/2017 01:16, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> Test basic context access, ptrace protection and filesystem event with
>> multiple cases.
>>
>> Changes since v5:
>> * add subtype test
>> * add ptrace tests
>> * split and rename files
>> * cleanup and rebase
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: James Morris <james.l.morris@oracle.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> Cc: Shuah Khan <shuah@kernel.org>
>> Cc: Will Drewry <wad@chromium.org>
>> ---
>> tools/testing/selftests/Makefile | 1 +
>> tools/testing/selftests/bpf/test_verifier.c | 64 +++++
>> tools/testing/selftests/landlock/.gitignore | 4 +
>> tools/testing/selftests/landlock/Makefile | 47 ++++
>> tools/testing/selftests/landlock/rules/Makefile | 52 ++++
>> tools/testing/selftests/landlock/rules/README.rst | 1 +
>> .../testing/selftests/landlock/rules/bpf_helpers.h | 1 +
>> .../testing/selftests/landlock/rules/fs_no_open.c | 31 +++
>> .../selftests/landlock/rules/fs_read_only.c | 31 +++
>> tools/testing/selftests/landlock/test.h | 35 +++
>> tools/testing/selftests/landlock/test_base.c | 31 +++
>> tools/testing/selftests/landlock/test_fs.c | 305 +++++++++++++++++++++
>> tools/testing/selftests/landlock/test_ptrace.c | 161 +++++++++++
>> 13 files changed, 764 insertions(+)
>> create mode 100644 tools/testing/selftests/landlock/.gitignore
>> create mode 100644 tools/testing/selftests/landlock/Makefile
>> create mode 100644 tools/testing/selftests/landlock/rules/Makefile
>> create mode 120000 tools/testing/selftests/landlock/rules/README.rst
>> create mode 120000 tools/testing/selftests/landlock/rules/bpf_helpers.h
>> create mode 100644 tools/testing/selftests/landlock/rules/fs_no_open.c
>> create mode 100644 tools/testing/selftests/landlock/rules/fs_read_only.c
>> create mode 100644 tools/testing/selftests/landlock/test.h
>> create mode 100644 tools/testing/selftests/landlock/test_base.c
>> create mode 100644 tools/testing/selftests/landlock/test_fs.c
>> create mode 100644 tools/testing/selftests/landlock/test_ptrace.c
>>
>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>> index d8593f1251ec..b584ad456428 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -12,6 +12,7 @@ TARGETS += gpio
>> TARGETS += intel_pstate
>> TARGETS += ipc
>> TARGETS += kcmp
>> +TARGETS += landlock
>> TARGETS += lib
>> TARGETS += membarrier
>> TARGETS += memfd
>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>> index daa87dd7c80e..77255b14871e 100644
>> --- a/tools/testing/selftests/bpf/test_verifier.c
>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>> @@ -4536,6 +4536,70 @@ static struct bpf_test tests[] = {
>> .result = REJECT,
>> .has_prog_subtype = true,
>> },
>> + {
>> + "missing subtype",
>> + .insns = {
>> + BPF_MOV32_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + },
>> + .errstr = "",
>> + .result = REJECT,
>> + .prog_type = BPF_PROG_TYPE_LANDLOCK,
>> + },
>> + {
>> + "landlock/fs: always accept",
>> + .insns = {
>> + BPF_MOV32_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + },
>> + .result = ACCEPT,
>> + .prog_type = BPF_PROG_TYPE_LANDLOCK,
>> + .has_prog_subtype = true,
>> + .prog_subtype = {
>> + .landlock_rule = {
>> + .version = 1,
>> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
>> + }
>> + },
>> + },
>> + {
>> + "landlock/fs: read context",
>> + .insns = {
>> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, status)),
>> + /* test operations on raw values */
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
>> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, arch)),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
>> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, syscall_nr)),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
>> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, syscall_cmd)),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
>> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, event)),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, arg1)),
>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6,
>> + offsetof(struct landlock_context, arg2)),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1),
>> + BPF_MOV32_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + },
>> + .result = ACCEPT,
>> + .prog_type = BPF_PROG_TYPE_LANDLOCK,
>> + .has_prog_subtype = true,
>> + .prog_subtype = {
>> + .landlock_rule = {
>> + .version = 1,
>> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
>> + }
>> + },
>> + },
>> };
>>
>> static int probe_filter_length(const struct bpf_insn *fp)
>> diff --git a/tools/testing/selftests/landlock/.gitignore b/tools/testing/selftests/landlock/.gitignore
>> new file mode 100644
>> index 000000000000..25b9cd834c3c
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/.gitignore
>> @@ -0,0 +1,4 @@
>> +/test_base
>> +/test_fs
>> +/test_ptrace
>> +/tmp_*
>> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
>> new file mode 100644
>> index 000000000000..9a52c82d64fa
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/Makefile
>> @@ -0,0 +1,47 @@
>> +LIBDIR := ../../../lib
>> +BPFOBJ := $(LIBDIR)/bpf/bpf.o
>> +LOADOBJ := ../../../../samples/bpf/bpf_load.o
>
> Is the selftest tarball creation tool okay with this? IIRC, it should
> be fine since it'll be a built object already, but it's a random
> thought I had while looking at this.
Hum, I'll check since it's the same for BPF tests.
>
>> +
>> +CFLAGS += -Wl,-no-as-needed -Wall -O2 -I../../../include/uapi -I$(LIBDIR)
>> +LDFLAGS += -lelf
>> +
>> +test_src = $(wildcard test_*.c)
>> +rule_src = $(wildcard rules/*.c)
>> +
>> +test_objs := $(test_src:.c=)
>> +rule_objs := $(rule_src:.c=.o)
>> +
>> +TEST_PROGS := $(test_objs)
>> +
>> +.PHONY: all clean clean_tmp force
>> +
>> +all: $(test_objs) $(rule_objs)
>> +
>> +# force a rebuild of BPFOBJ when its dependencies are updated
>> +force:
>> +
>> +$(BPFOBJ): force
>> + $(MAKE) -C $(dir $(BPFOBJ))
>> +
>> +$(LOADOBJ):
>> + $(MAKE) -C $(dir $(LOADOBJ))
>> +
>> +# minimize builds
>> +rules/modules.order: $(rule_src)
>> + $(MAKE) -C rules
>> + @touch $@
>> +
>> +$(rule_objs): rules/modules.order
>> + @
>> +
>> +$(test_objs): $(BPFOBJ) $(LOADOBJ)
>> +
>> +include ../lib.mk
>> +
>> +clean_tmp:
>> + $(RM) -r tmp_*
>> +
>> +clean: clean_tmp
>> + $(MAKE) -C rules clean
>> + $(RM) $(test_objs)
>> +
>> diff --git a/tools/testing/selftests/landlock/rules/Makefile b/tools/testing/selftests/landlock/rules/Makefile
>> new file mode 100644
>> index 000000000000..8d6ff960ff7c
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/rules/Makefile
>> @@ -0,0 +1,52 @@
>> +# kbuild trick to avoid linker error. Can be omitted if a module is built.
>> +obj- := dummy.o
>> +
>> +# Tell kbuild to always build the programs
>> +always := fs_read_only.o
>> +always += fs_no_open.o
>> +
>> +EXTRA_CFLAGS = -Wall -Wextra
>> +
>> +# Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
>> +# make samples/bpf/ LLC=~/git/llvm/build/bin/llc CLANG=~/git/llvm/build/bin/clang
>> +LLC ?= llc
>> +CLANG ?= clang
>> +
>> +# Verify LLVM compiler tools are available and bpf target is supported by llc
>> +.PHONY: all clean verify_cmds verify_target_bpf $(CLANG) $(LLC)
>> +
>> +# Trick to allow make to be run from this directory
>> +all:
>> + $(MAKE) -C ../../../../../ $(CURDIR)/
>> +
>> +clean:
>> + $(MAKE) -C ../../../../../ M=$(CURDIR) clean
>
> Is this really needed? Others don't have it, I think.
This is copied from the BPF tests and yes it's needed.
>
>> +verify_cmds: $(CLANG) $(LLC)
>> + @for TOOL in $^ ; do \
>> + if ! (which -- "$${TOOL}" > /dev/null 2>&1); then \
>> + echo "*** ERROR: Cannot find LLVM tool $${TOOL}" ;\
>> + exit 1; \
>> + else true; fi; \
>> + done
>> +
>> +verify_target_bpf: verify_cmds
>> + @if ! (${LLC} -march=bpf -mattr=help > /dev/null 2>&1); then \
>> + echo "*** ERROR: LLVM (${LLC}) does not support 'bpf' target" ;\
>> + echo " NOTICE: LLVM version >= 3.7.1 required" ;\
>> + exit 2; \
>> + else true; fi
>> +
>> +%_kern.c: verify_target_bpf
>> +
>> +# asm/sysreg.h - inline assembly used by it is incompatible with llvm.
>> +# But, there is no easy way to fix it, so just exclude it since it is
>> +# useless for BPF samples.
>> +$(obj)/%.o: $(src)/%.c
>> + $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
>> + -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value -Wno-pointer-sign \
>> + -Wno-compare-distinct-pointer-types \
>> + -Wno-gnu-variable-sized-type-not-at-end \
>> + -Wno-tautological-compare \
>> + -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf -filetype=obj -o $@
>
> Is clang required for the samples and the selftests? That needs to be
> avoided... there needs to be a way to show people how to build a
> landlock rule without requiring clang.
I can rewrite this tests without requiring clang but it is already
required for BPF tests…
>
>> +
>> diff --git a/tools/testing/selftests/landlock/rules/README.rst b/tools/testing/selftests/landlock/rules/README.rst
>> new file mode 120000
>> index 000000000000..605f48aa6f72
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/rules/README.rst
>> @@ -0,0 +1 @@
>> +../../../../../samples/bpf/README.rst
>> \ No newline at end of file
>> diff --git a/tools/testing/selftests/landlock/rules/bpf_helpers.h b/tools/testing/selftests/landlock/rules/bpf_helpers.h
>> new file mode 120000
>> index 000000000000..0aa1a521b39a
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/rules/bpf_helpers.h
>> @@ -0,0 +1 @@
>> +../../../../../samples/bpf/bpf_helpers.h
>> \ No newline at end of file
>> diff --git a/tools/testing/selftests/landlock/rules/fs_no_open.c b/tools/testing/selftests/landlock/rules/fs_no_open.c
>> new file mode 100644
>> index 000000000000..c6ea305e58a7
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/rules/fs_no_open.c
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Landlock rule - no-open filesystem
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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 <uapi/linux/bpf.h>
>> +#include "bpf_helpers.h"
>> +
>> +SEC("landlock1")
>> +static int landlock_fs_prog1(struct landlock_context *ctx)
>> +{
>> + if (!(ctx->arg2 & LANDLOCK_ACTION_FS_GET))
>> + return 0;
>> + return 1;
>> +}
>> +
>> +SEC("subtype")
>> +static union bpf_prog_subtype _subtype = {
>> + .landlock_rule = {
>> + .version = 1,
>> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
>> + }
>> +};
>> +
>> +SEC("license")
>> +static const char _license[] = "GPL";
>> diff --git a/tools/testing/selftests/landlock/rules/fs_read_only.c b/tools/testing/selftests/landlock/rules/fs_read_only.c
>> new file mode 100644
>> index 000000000000..212dda7c0c27
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/rules/fs_read_only.c
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Landlock rule - read-only filesystem
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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 <uapi/linux/bpf.h>
>> +#include "bpf_helpers.h"
>> +
>> +SEC("landlock1")
>> +static int landlock_fs_prog1(struct landlock_context *ctx)
>> +{
>> + if (!(ctx->arg2 & LANDLOCK_ACTION_FS_WRITE))
>> + return 0;
>> + return 1;
>> +}
>> +
>> +SEC("subtype")
>> +static union bpf_prog_subtype _subtype = {
>> + .landlock_rule = {
>> + .version = 1,
>> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
>> + }
>> +};
>> +
>> +SEC("license")
>> +static const char _license[] = "GPL";
>> diff --git a/tools/testing/selftests/landlock/test.h b/tools/testing/selftests/landlock/test.h
>> new file mode 100644
>> index 000000000000..7a194815391b
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/test.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * Landlock helpers
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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 <errno.h>
>> +#include <sys/prctl.h>
>> +#include <sys/syscall.h>
>> +
>> +#include "../seccomp/test_harness.h"
>> +#include "../../../../samples/bpf/bpf_load.h"
>> +
>> +#ifndef SECCOMP_APPEND_LANDLOCK_RULE
>> +#define SECCOMP_APPEND_LANDLOCK_RULE 2
>> +#endif
>> +
>> +#ifndef seccomp
>> +static int seccomp(unsigned int op, unsigned int flags, void *args)
>> +{
>> + errno = 0;
>> + return syscall(__NR_seccomp, op, flags, args);
>> +}
>> +#endif
>> +
>> +#define ASSERT_STEP(cond) \
>> + { \
>> + step--; \
>> + if (!(cond)) \
>> + _exit(step); \
>> + }
>
> Can you explain this in more detail? I'm assuming there is a problem
> with writing to the TH_LOG_STREAM fd or something?
It's a trick to use the test framework without requiring to be allowed
to write to an FD (i.e. log stream), but only to exit a code. I use this
to test a Landlock rule which forbid access to any FS objects (including
open FD). This could be used for seccomp too.
>
>> diff --git a/tools/testing/selftests/landlock/test_base.c b/tools/testing/selftests/landlock/test_base.c
>> new file mode 100644
>> index 000000000000..bdf056edee03
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/test_base.c
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Landlock tests - base
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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.
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <errno.h>
>> +
>> +#include "test.h"
>> +
>> +TEST(seccomp_landlock)
>> +{
>> + int ret;
>> +
>> + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0);
>> + ASSERT_EQ(0, ret) {
>> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
>> + }
>> + ret = seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, NULL);
>> + EXPECT_EQ(-1, ret);
>> + EXPECT_EQ(EFAULT, errno) {
>> + TH_LOG("Kernel does not support CONFIG_SECURITY_LANDLOCK");
>> + }
>> +}
>> +
>> +TEST_HARNESS_MAIN
>> diff --git a/tools/testing/selftests/landlock/test_fs.c b/tools/testing/selftests/landlock/test_fs.c
>> new file mode 100644
>> index 000000000000..e69eda433716
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/test_fs.c
>> @@ -0,0 +1,305 @@
>> +/*
>> + * Landlock tests - filesystem
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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.
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <errno.h>
>> +#include <linux/bpf.h>
>> +#include <linux/filter.h>
>> +#include <linux/seccomp.h>
>> +#include <stddef.h>
>> +#include <string.h>
>> +#include <sys/prctl.h>
>> +#include <sys/syscall.h>
>> +
>> +#include <fcntl.h> /* open() */
>> +#include <sys/mount.h>
>> +#include <sys/stat.h> /* mkdir() */
>> +#include <sys/mman.h> /* mmap() */
>> +
>> +#include "test.h"
>> +
>> +#define TMP_PREFIX "tmp_"
>> +
>> +struct layout1 {
>> + int file_ro;
>> + int file_rw;
>> + int file_wo;
>> +};
>> +
>> +static void setup_layout1(struct __test_metadata *_metadata,
>> + struct layout1 *l1)
>> +{
>> + int fd;
>> + char buf[] = "fs_read_only";
>> +
>> + l1->file_ro = -1;
>> + l1->file_rw = -1;
>> + l1->file_wo = -1;
>> +
>> + fd = open(TMP_PREFIX "file_created",
>> + O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600);
>> + ASSERT_GE(fd, 0);
>> + ASSERT_EQ(sizeof(buf), write(fd, buf, sizeof(buf)));
>> + ASSERT_EQ(0, close(fd));
>> +
>> + fd = mkdir(TMP_PREFIX "dir_created", 0600);
>> + ASSERT_GE(fd, 0);
>> + ASSERT_EQ(0, close(fd));
>> +
>> + l1->file_ro = open(TMP_PREFIX "file_ro",
>> + O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600);
>> + ASSERT_LE(0, l1->file_ro);
>> + ASSERT_EQ(sizeof(buf), write(l1->file_ro, buf, sizeof(buf)));
>> + ASSERT_EQ(0, close(l1->file_ro));
>> + l1->file_ro = open(TMP_PREFIX "file_ro",
>> + O_RDONLY | O_CLOEXEC, 0600);
>> + ASSERT_LE(0, l1->file_ro);
>> +
>> + l1->file_rw = open(TMP_PREFIX "file_rw",
>> + O_CREAT | O_EXCL | O_RDWR | O_CLOEXEC, 0600);
>> + ASSERT_LE(0, l1->file_rw);
>> + ASSERT_EQ(sizeof(buf), write(l1->file_rw, buf, sizeof(buf)));
>> + ASSERT_EQ(0, lseek(l1->file_rw, 0, SEEK_SET));
>> +
>> + l1->file_wo = open(TMP_PREFIX "file_wo",
>> + O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600);
>> + ASSERT_LE(0, l1->file_wo);
>> + ASSERT_EQ(sizeof(buf), write(l1->file_wo, buf, sizeof(buf)));
>> + ASSERT_EQ(0, lseek(l1->file_wo, 0, SEEK_SET));
>> +}
>> +
>> +static void cleanup_layout1(void)
>> +{
>> + unlink(TMP_PREFIX "file_created");
>> + unlink(TMP_PREFIX "file_ro");
>> + unlink(TMP_PREFIX "file_rw");
>> + unlink(TMP_PREFIX "file_wo");
>> + unlink(TMP_PREFIX "should_not_exist");
>> + rmdir(TMP_PREFIX "dir_created");
>> +}
>> +
>> +FIXTURE(fs_read_only) {
>> + struct layout1 l1;
>> + int prog;
>> +};
>> +
>> +FIXTURE_SETUP(fs_read_only)
>> +{
>> + cleanup_layout1();
>> + setup_layout1(_metadata, &self->l1);
>> +
>> + ASSERT_EQ(0, load_bpf_file("rules/fs_read_only.o")) {
>> + TH_LOG("%s", bpf_log_buf);
>> + }
>> + self->prog = prog_fd[0];
>> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0)) {
>> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
>> + }
>> +}
>> +
>> +FIXTURE_TEARDOWN(fs_read_only)
>> +{
>> + EXPECT_EQ(0, close(self->prog));
>> + /* cleanup_layout1() would be denied here */
>> +}
>> +
>> +TEST_F(fs_read_only, load_prog) {}
>> +
>> +TEST_F(fs_read_only, read_only_file)
>> +{
>> + int fd;
>> + int step = 0;
>> + char buf_write[] = "should not be written";
>> + char buf_read[2];
>> +
>> + ASSERT_EQ(-1, write(self->l1.file_ro, buf_write, sizeof(buf_write)));
>> + ASSERT_EQ(EBADF, errno);
>> +
>> + ASSERT_EQ(-1, read(self->l1.file_wo, buf_read, sizeof(buf_read)));
>> + ASSERT_EQ(EBADF, errno);
>> +
>> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
>> + TH_LOG("Failed to apply rule fs_read_only: %s",
>> + strerror(errno));
>> + }
>> +
>> + fd = open(".", O_TMPFILE | O_EXCL | O_RDWR | O_CLOEXEC, 0600);
>> + ASSERT_STEP(fd == -1);
>> + ASSERT_STEP(errno != EOPNOTSUPP)
>> + ASSERT_STEP(errno == EPERM);
>> +
>> + fd = open(TMP_PREFIX "file_created",
>> + O_RDONLY | O_CLOEXEC);
>> + ASSERT_STEP(fd >= 0);
>> + ASSERT_STEP(!close(fd));
>> +
>> + fd = open(TMP_PREFIX "file_created",
>> + O_RDWR | O_CLOEXEC);
>> + ASSERT_STEP(fd == -1);
>> + ASSERT_STEP(errno == EPERM);
>> +
>> + fd = open(TMP_PREFIX "file_created",
>> + O_WRONLY | O_CLOEXEC);
>> + ASSERT_STEP(fd == -1);
>> + ASSERT_STEP(errno == EPERM);
>> +
>> + fd = open(TMP_PREFIX "should_not_exist",
>> + O_CREAT | O_EXCL | O_CLOEXEC, 0600);
>> + ASSERT_STEP(fd == -1);
>> + ASSERT_STEP(errno == EPERM);
>> +
>> + ASSERT_STEP(-1 ==
>> + write(self->l1.file_ro, buf_write, sizeof(buf_write)));
>> + ASSERT_STEP(errno == EBADF);
>> + ASSERT_STEP(sizeof(buf_read) ==
>> + read(self->l1.file_ro, buf_read, sizeof(buf_read)));
>> +
>> + ASSERT_STEP(-1 ==
>> + write(self->l1.file_rw, buf_write, sizeof(buf_write)));
>> + ASSERT_STEP(errno == EPERM);
>> + ASSERT_STEP(sizeof(buf_read) ==
>> + read(self->l1.file_rw, buf_read, sizeof(buf_read)));
>> +
>> + ASSERT_STEP(-1 == write(self->l1.file_wo, buf_write, sizeof(buf_write)));
>> + ASSERT_STEP(errno == EPERM);
>> + ASSERT_STEP(-1 == read(self->l1.file_wo, buf_read, sizeof(buf_read)));
>> + ASSERT_STEP(errno == EBADF);
>> +
>> + ASSERT_STEP(-1 == unlink(TMP_PREFIX "file_created"));
>> + ASSERT_STEP(errno == EPERM);
>> + ASSERT_STEP(-1 == rmdir(TMP_PREFIX "dir_created"));
>> + ASSERT_STEP(errno == EPERM);
>> +
>> + ASSERT_STEP(0 == close(self->l1.file_ro));
>> + ASSERT_STEP(0 == close(self->l1.file_rw));
>> + ASSERT_STEP(0 == close(self->l1.file_wo));
>> +}
>> +
>> +TEST_F(fs_read_only, read_only_mount)
>> +{
>> + int step = 0;
>> +
>> + ASSERT_EQ(0, mount(".", TMP_PREFIX "dir_created",
>> + NULL, MS_BIND, NULL));
>> + ASSERT_EQ(0, umount2(TMP_PREFIX "dir_created", MNT_FORCE));
>> +
>> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
>> + TH_LOG("Failed to apply rule fs_read_only: %s",
>> + strerror(errno));
>> + }
>> +
>> + ASSERT_STEP(-1 == mount(".", TMP_PREFIX "dir_created",
>> + NULL, MS_BIND, NULL));
>> + ASSERT_STEP(errno == EPERM);
>> + ASSERT_STEP(-1 == umount("/"));
>> + ASSERT_STEP(errno == EPERM);
>> +}
>> +
>> +TEST_F(fs_read_only, read_only_mem)
>> +{
>> + int step = 0;
>> + void *addr;
>> +
>> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE,
>> + MAP_SHARED, self->l1.file_rw, 0);
>> + ASSERT_NE(NULL, addr);
>> + ASSERT_EQ(0, munmap(addr, 1));
>> +
>> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
>> + TH_LOG("Failed to apply rule fs_read_only: %s",
>> + strerror(errno));
>> + }
>> +
>> + addr = mmap(NULL, 1, PROT_READ, MAP_SHARED,
>> + self->l1.file_rw, 0);
>> + ASSERT_STEP(addr != NULL);
>> + ASSERT_STEP(-1 == mprotect(addr, 1, PROT_WRITE));
>> + ASSERT_STEP(errno == EPERM);
>> + ASSERT_STEP(0 == munmap(addr, 1));
>> +
>> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE, MAP_SHARED,
>> + self->l1.file_rw, 0);
>> + ASSERT_STEP(addr != NULL);
>> + ASSERT_STEP(errno == EPERM);
>> +
>> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE, MAP_PRIVATE,
>> + self->l1.file_rw, 0);
>> + ASSERT_STEP(addr != NULL);
>> + ASSERT_STEP(0 == munmap(addr, 1));
>> +}
>> +
>> +FIXTURE(fs_no_open) {
>> + struct layout1 l1;
>> + int prog;
>> +};
>> +
>> +FIXTURE_SETUP(fs_no_open)
>> +{
>> + cleanup_layout1();
>> + setup_layout1(_metadata, &self->l1);
>> +
>> + ASSERT_EQ(0, load_bpf_file("rules/fs_no_open.o")) {
>> + TH_LOG("%s", bpf_log_buf);
>> + }
>> + self->prog = prog_fd[0];
>> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0)) {
>> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
>> + }
>> +}
>> +
>> +FIXTURE_TEARDOWN(fs_no_open)
>> +{
>> + EXPECT_EQ(0, close(self->prog));
>> + cleanup_layout1();
>> +}
>> +
>> +static void landlocked_deny_open(struct __test_metadata *_metadata,
>> + struct layout1 *l1)
>> +{
>> + int fd;
>> + void *addr;
>> +
>> + fd = open(".", O_DIRECTORY | O_CLOEXEC);
>> + ASSERT_EQ(-1, fd);
>> + ASSERT_EQ(EPERM, errno);
>> +
>> + addr = mmap(NULL, 1, PROT_READ | PROT_WRITE,
>> + MAP_SHARED, l1->file_rw, 0);
>> + ASSERT_NE(NULL, addr);
>> + ASSERT_EQ(0, munmap(addr, 1));
>> +}
>> +
>> +TEST_F(fs_no_open, deny_open_for_hierarchy) {
>> + int fd;
>> + int status;
>> + pid_t child;
>> +
>> + fd = open(".", O_DIRECTORY | O_CLOEXEC);
>> + ASSERT_LE(0, fd);
>> + ASSERT_EQ(0, close(fd));
>> +
>> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &self->prog)) {
>> + TH_LOG("Failed to apply rule fs_no_open: %s", strerror(errno));
>> + }
>> +
>> + landlocked_deny_open(_metadata, &self->l1);
>> +
>> + child = fork();
>> + ASSERT_LE(0, child);
>> + if (!child) {
>> + landlocked_deny_open(_metadata, &self->l1);
>> + _exit(1);
>> + }
>> + ASSERT_EQ(child, waitpid(child, &status, 0));
>> + ASSERT_TRUE(WIFEXITED(status));
>> + _exit(WEXITSTATUS(status));
>> +}
>> +
>> +TEST_HARNESS_MAIN
>> diff --git a/tools/testing/selftests/landlock/test_ptrace.c b/tools/testing/selftests/landlock/test_ptrace.c
>> new file mode 100644
>> index 000000000000..0c940a7fd3d0
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/test_ptrace.c
>> @@ -0,0 +1,161 @@
>> +/*
>> + * Landlock tests - ptrace
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * 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.
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <signal.h> /* raise */
>> +#include <sys/ptrace.h>
>> +#include <sys/types.h> /* waitpid */
>> +#include <sys/wait.h> /* waitpid */
>> +#include <unistd.h> /* fork, pipe */
>> +
>> +#include "test.h"
>> +
>> +static void apply_null_sandbox(struct __test_metadata *_metadata)
>> +{
>> + const struct bpf_insn prog_accept[] = {
>> + BPF_MOV32_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + };
>> + const union bpf_prog_subtype subtype = {
>> + .landlock_rule = {
>> + .version = 1,
>> + .event = LANDLOCK_SUBTYPE_EVENT_FS,
>> + }
>> + };
>> + int prog;
>> + char log[256] = "";
>> +
>> + prog = bpf_load_program(BPF_PROG_TYPE_LANDLOCK,
>> + (const struct bpf_insn *)&prog_accept,
>> + sizeof(prog_accept) / sizeof(struct bpf_insn), "GPL",
>> + 0, log, sizeof(log), &subtype);
>> + ASSERT_NE(-1, prog) {
>> + TH_LOG("Failed to load minimal rule: %s\n%s",
>> + strerror(errno), log);
>> + }
>> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0)) {
>> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS");
>> + }
>> + ASSERT_EQ(0, seccomp(SECCOMP_APPEND_LANDLOCK_RULE, 0, &prog)) {
>> + TH_LOG("Failed to apply minimal rule: %s", strerror(errno));
>> + }
>> + EXPECT_EQ(0, close(prog));
>> +}
>> +
>> +/* PTRACE_TRACEME and PTRACE_ATTACH without Landlock rules effect */
>> +static void check_ptrace(struct __test_metadata *_metadata,
>> + int sandbox_both, int sandbox_parent, int sandbox_child,
>> + int expect_ptrace)
>> +{
>> + pid_t child;
>> + int status;
>> + int pipefd[2];
>> +
>> + ASSERT_EQ(0, pipe(pipefd));
>> + if (sandbox_both)
>> + apply_null_sandbox(_metadata);
>> +
>> + child = fork();
>> + ASSERT_LE(0, child);
>> + if (child == 0) {
>> + char buf;
>> +
>> + EXPECT_EQ(0, close(pipefd[1]));
>> + if (sandbox_child)
>> + apply_null_sandbox(_metadata);
>> +
>> + /* test traceme */
>> + ASSERT_EQ(expect_ptrace, ptrace(PTRACE_TRACEME));
>> + if (expect_ptrace) {
>> + ASSERT_EQ(EPERM, errno);
>> + } else {
>> + ASSERT_EQ(0, raise(SIGSTOP));
>> + }
>> +
>> + /* sync */
>> + ASSERT_EQ(1, read(pipefd[0], &buf, 1)) {
>> + TH_LOG("Failed to read() sync from parent");
>> + }
>> + ASSERT_EQ('.', buf);
>> + _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
>> + }
>> +
>> + EXPECT_EQ(0, close(pipefd[0]));
>> + if (sandbox_parent)
>> + apply_null_sandbox(_metadata);
>> +
>> + /* test traceme */
>> + if (!expect_ptrace) {
>> + ASSERT_EQ(child, waitpid(child, &status, 0));
>> + ASSERT_EQ(1, WIFSTOPPED(status));
>> + ASSERT_EQ(0, ptrace(PTRACE_DETACH, child, NULL, 0));
>> + }
>> + /* test attach */
>> + ASSERT_EQ(expect_ptrace, ptrace(PTRACE_ATTACH, child, NULL, 0));
>> + if (expect_ptrace) {
>> + ASSERT_EQ(EPERM, errno);
>> + } else {
>> + ASSERT_EQ(child, waitpid(child, &status, 0));
>> + ASSERT_EQ(1, WIFSTOPPED(status));
>> + ASSERT_EQ(0, ptrace(PTRACE_CONT, child, NULL, 0));
>> + }
>> +
>> + /* sync */
>> + ASSERT_EQ(1, write(pipefd[1], ".", 1)) {
>> + TH_LOG("Failed to write() sync to child");
>> + }
>> + ASSERT_EQ(child, waitpid(child, &status, 0));
>> + if (WIFSIGNALED(status) || WEXITSTATUS(status))
>> + _metadata->passed = 0;
>> +}
>> +
>> +TEST(ptrace_allow_without_sandbox)
>> +{
>> + /* no sandbox */
>> + check_ptrace(_metadata, 0, 0, 0, 0);
>> +}
>> +
>> +TEST(ptrace_allow_with_one_sandbox)
>> +{
>> + /* child sandbox */
>> + check_ptrace(_metadata, 0, 0, 1, 0);
>> +}
>> +
>> +TEST(ptrace_allow_with_nested_sandbox)
>> +{
>> + /* inherited and child sandbox */
>> + check_ptrace(_metadata, 1, 0, 1, 0);
>> +}
>> +
>> +TEST(ptrace_deny_with_parent_sandbox)
>> +{
>> + /* parent sandbox */
>> + check_ptrace(_metadata, 0, 1, 0, -1);
>> +}
>> +
>> +TEST(ptrace_deny_with_nested_and_parent_sandbox)
>> +{
>> + /* inherited and parent sandbox */
>> + check_ptrace(_metadata, 1, 1, 0, -1);
>> +}
>> +
>> +TEST(ptrace_deny_with_forked_sandbox)
>> +{
>> + /* inherited, parent and child sandbox */
>> + check_ptrace(_metadata, 1, 1, 1, -1);
>> +}
>> +
>> +TEST(ptrace_deny_with_sibling_sandbox)
>> +{
>> + /* parent and child sandbox */
>> + check_ptrace(_metadata, 0, 1, 1, -1);
>> +}
>> +
>> +TEST_HARNESS_MAIN
>> --
>> 2.11.0
>>
>
> Awesome. I love to see all these tests, with both positive and
> negative checks. Nice!
>
> -Kees
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [Patch net-next v3] net_sched: move the empty tp check from ->destroy() to ->delete()
From: Daniel Borkmann @ 2017-04-18 23:55 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, John Fastabend, Jamal Hadi Salim,
lucasb
In-Reply-To: <CAM_iQpV14UyknX8DywDxQGA4xRNspqWzag+t49wW9-N2W=cgmg@mail.gmail.com>
On 04/18/2017 10:55 PM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 10:01 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> Hi Cong,
>>
>> sorry for the late reply. Generally the patch looks good to me, just
>> a few comments inline:
>>
>> On 04/17/2017 08:30 PM, Cong Wang wrote:
>>>
>>> Roi reported we could have a race condition where in ->classify() path
>>> we dereference tp->root and meanwhile a parallel ->destroy() makes it
>>> a NULL.
>>
>> Correct.
>>
>>> This is possible because ->destroy() could be called when deleting
>>> a filter to check if we are the last one in tp, this tp is still
>>> linked and visible at that time.
>>>
>>> Daniel fixed this in commit d936377414fa
>>> ("net, sched: respect rcu grace period on cls destruction"), but
>>> the root cause of this problem is the semantic of ->destroy(), it
>>> does two things (for non-force case):
>>>
>>> 1) check if tp is empty
>>> 2) if tp is empty we could really destroy it
>>>
>>> and its caller, if cares, needs to check its return value to see if
>>> it is really destroyed. Therefore we can't unlink tp unless we know
>>> it is empty.
>>>
>>> As suggested by Daniel, we could actually move the test logic to
>>> ->delete()
>>> so that we can safely unlink tp after ->delete() tells us the last one is
>>> just deleted and before ->destroy().
>>>
>>> What's more, even we unlink it before ->destroy(), it could still have
>>> readers since we don't wait for a grace period here, we should not modify
>>> tp->root in ->destroy() either.
>>
>> Here seems to be a bit of a mixup in this analysis, imo. The issue
>> that Roi reported back then was exactly the one that d936377414fa ("net,
>> sched: respect rcu grace period on cls destruction") fixed, which
>> affected flower and other classifiers:
>>
>> Roi reported a crash in flower where tp->root was NULL in ->classify()
>> callbacks. Reason is that in ->destroy() tp->root is set to NULL via
>> RCU_INIT_POINTER(). It's problematic for some of the classifiers,
>> because
>> this doesn't respect RCU grace period for them, and as a result, still
>> outstanding readers from tc_classify() will try to blindly dereference
>> a NULL tp->root.
>>
>> The ->delete() callback was never used by Roi back then, he said that
>> he just removed the ingress qdisc in his test, which implicitly purges
>> all cls attached to it via tcf_destroy_chain(). So the above description
>> with regards to the "root cause" of Roi's reported issue is not correct.
>
> Hmm, thanks for clarifying this, I will remove this part, together with the
> Reported-by of Roi.
>
>> The issue that this patch fixes is an _independent_ race that we found
>> while auditing the code when looking into Roi's report back then. It
>> fixes commit 1e052be69d04 ("net_sched: destroy proto tp when all filters
>> are gone"), which added the RCU_INIT_POINTER() after the tcf_destroy() in
>> RTM_DELTFILTER case. That part of the description looks good, where you
>> describe that "[...] we need to move the test logic to ->delete(), so
>> that we can safely unlink tp after ->delete() tells us the last one is
>> just deleted and before ->destroy()."
>
> OK.
>
>> Please also add Fixes tag, so it can be better tracked for backports.
>>
>> Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are
>> gone")
>
> Actually I intentionally remove the Fixes tag because I don't think we
> need to backport it to stable as no one reports a _real_ crash so far,
> right? Or you saw a real one?
>
> (Not to mention its size does not fit for -stable either.)
I don't think anyone reported a crash on this. I think net-next is
fine, but still the Fixes tag helps keeping track of bug fixes (we
usually do this for net-next too when applicable, it certainly doesn't
hurt and helps identifying follow-ups to a certain commit), f.e. for
others backporting this to their kernels (outside of the scope of
upstream stable).
>> The above three RCU_INIT_POINTER(tp->root, NULL) are independent
>> of the fix and actually do no harm right now. I described that in
>> d936377414fa ("net, sched: respect rcu grace period on cls destruction")
>> as well, meaning that they each handle tp->root being NULL in ->classify()
>> path (for historic reasons), so this is handled gracefully, readers use
>> rcu_dereference_bh(tp->root) and test for this being NULL.
>>
>> But I agree that this could be cleaned up along with the check in the
>> ->classify() callbacks for these three (not sure if really worth it,
>> though). However, such cleanup should be a separate patch and not
>> included in this fix.
>
> Agreed. I will make it a separated patch and send them together.
Okay, sounds good thanks!
^ permalink raw reply
* Re: question about size of sk_buff and skb_shared_info
From: Eric Dumazet @ 2017-04-18 23:55 UTC (permalink / raw)
To: Code Soldier1; +Cc: netdev
In-Reply-To: <CABGNecz7oRk8PZS_YzG2QzpMXi78rQX4zPpt_28ZgJYYZS-ytg@mail.gmail.com>
Please do not top post on netdev
On Tue, 2017-04-18 at 16:26 -0700, Code Soldier1 wrote:
> Eric,
>
> This alignment flag is passed to the cache constructor and the
> allocation is indeed cache aligned. However, since the allocated size
> is not a multiple of the alignment, wont memory be wasted ?. We can
> get 40 extra bytes without any side effects since they are on the same
> cache line ?
We _want_ to align skb to cache lines.
Fact that few bytes might be wasted is unfortunate, but negligible.
BTW, if you do kmalloc(1025), kmalloc() wastes 1023 bytes.
>
> kmem_cache_create() code does an ALIGN() to round up the size.
>
> kasan_cache_create(cachep, &size, &flags);
>
> size = ALIGN(size, cachep->align);
>
> This is the size used in calculate_slab_order() to calculate num. I am
> assuming that in the non debug case gfp_order will be 0.
>
> Perhaps I am missing something.
>
>
> On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
> >> Hi Folks,
> >>
> >> I am sure there is a reason for the current sizes of these structures,
> >> However the reason is not obvious to me. So please help me understand.
> >>
> >> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
> >> is that. I expected it to be a multiple of 32/64 as they are the most
> >> common cache lines. Since the alignment calculation will align the
> >> structure with the hw cache line, it seems like we might be wasting
> >> space ?
> >>
> >> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
> >>
> >> Thanks,
> >>
> >
> > The alignment is there.
> > Look at skb_init() code, using SLAB_HWCACHE_ALIGN
> >
> >
> >
> >
> >
>
>
>
^ permalink raw reply
* Re: [PATCH net-next v6 10/11] bpf,landlock: Add tests for Landlock
From: Kees Cook @ 2017-04-18 23:59 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <f5291c43-100f-a06c-6c64-c718a02822df@digikod.net>
On Tue, Apr 18, 2017 at 4:53 PM, Mickaël Salaün <mic@digikod.net> wrote:
> On 19/04/2017 01:16, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/landlock/Makefile
>>> @@ -0,0 +1,47 @@
>>> +LIBDIR := ../../../lib
>>> +BPFOBJ := $(LIBDIR)/bpf/bpf.o
>>> +LOADOBJ := ../../../../samples/bpf/bpf_load.o
>>
>> Is the selftest tarball creation tool okay with this? IIRC, it should
>> be fine since it'll be a built object already, but it's a random
>> thought I had while looking at this.
>
> Hum, I'll check since it's the same for BPF tests.
Okay, cool.
>>> +# asm/sysreg.h - inline assembly used by it is incompatible with llvm.
>>> +# But, there is no easy way to fix it, so just exclude it since it is
>>> +# useless for BPF samples.
>>> +$(obj)/%.o: $(src)/%.c
>>> + $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
>>> + -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value -Wno-pointer-sign \
>>> + -Wno-compare-distinct-pointer-types \
>>> + -Wno-gnu-variable-sized-type-not-at-end \
>>> + -Wno-tautological-compare \
>>> + -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf -filetype=obj -o $@
>>
>> Is clang required for the samples and the selftests? That needs to be
>> avoided... there needs to be a way to show people how to build a
>> landlock rule without requiring clang.
>
> I can rewrite this tests without requiring clang but it is already
> required for BPF tests…
So, I guess it's not a big deal for selftests (but it'd be nice, even
for BPF), but I think at least the samples/ should have examples on
how to do it "by hand", etc. Not everyone will build stuff with clang,
and it'd be good to make landlock as available as possible.
>>> +#define ASSERT_STEP(cond) \
>>> + { \
>>> + step--; \
>>> + if (!(cond)) \
>>> + _exit(step); \
>>> + }
>>
>> Can you explain this in more detail? I'm assuming there is a problem
>> with writing to the TH_LOG_STREAM fd or something?
>
> It's a trick to use the test framework without requiring to be allowed
> to write to an FD (i.e. log stream), but only to exit a code. I use this
> to test a Landlock rule which forbid access to any FS objects (including
> open FD). This could be used for seccomp too.
Okay. For seccomp, we just allow the fd. :P I'm not opposed to it; it
just makes some debugging harder without text details, etc.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net-next v6 09/11] seccomp: Enhance test_harness with an assert step mechanism
From: Kees Cook @ 2017-04-19 0:02 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <20170328234650.19695-10-mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
> This is useful to return an information about the error without being
> able to write to TH_LOG_STREAM.
>
> Helpers from test_harness.h may be useful outside of the seccomp
> directory.
>
> Signed-off-by: Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
> Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> Cc: Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Shuah Khan <shuah-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> tools/testing/selftests/seccomp/test_harness.h | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/seccomp/test_harness.h b/tools/testing/selftests/seccomp/test_harness.h
> index a786c69c7584..77e407663e06 100644
> --- a/tools/testing/selftests/seccomp/test_harness.h
> +++ b/tools/testing/selftests/seccomp/test_harness.h
> @@ -397,7 +397,7 @@ struct __test_metadata {
> const char *name;
> void (*fn)(struct __test_metadata *);
> int termsig;
> - int passed;
> + __s8 passed;
Why the reduction here? int is signed too?
> int trigger; /* extra handler after the evaluation */
> struct __test_metadata *prev, *next;
> };
> @@ -476,6 +476,12 @@ void __run_test(struct __test_metadata *t)
> "instead of by signal (code: %d)\n",
> t->name,
> WEXITSTATUS(status));
> + } else if (t->passed < 0) {
> + fprintf(TH_LOG_STREAM,
> + "%s: Failed at step #%d\n",
> + t->name,
> + t->passed * -1);
> + t->passed = 0;
> }
Instead of creating an overloaded mechanism here, perhaps have an
option reporting mechanism that can be enabled. Like adding to
__test_metadata "bool no_stream; int test_number;" and adding
test_number++ to each ASSERT/EXCEPT call, and doing something like:
if (t->no_stream) {
fprintf(TH_LOG_STREAM,
"%s: Failed at step #%d\n",
t->name,
t->test_number);
}
It'd be a cleaner approach, maybe?
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net-next v6 00/11] Landlock LSM: Toward unprivileged sandboxing
From: Mickaël Salaün @ 2017-04-19 0:12 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <CAGXu5jLpM13xmFf2_bcWH=db7LEavfpZswvPbAM3+66dBPtMTg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3031 bytes --]
On 19/04/2017 01:26, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> This sixth series add some changes to the previous one [1], including a simpler
>> rule inheritance hierarchy (similar to seccomp-bpf), a ptrace scope protection,
>> some file renaming (better feature identification per file), a future-proof
>> eBPF subtype and miscellaneous cosmetic fixes.
>
> Sorry for the delay in review! I finally had a chunk of time I could
> devote to this. I really like how it's heading. I still wonder about
> its overlap with seccomp (it's really only using the syscall now...),
> but that's just a detail. Getting the abstraction away from direct LSM
> hooks looks good.
>
>> There is as yet no way to allow a process to access only a subset of the
>> filesystem where the subset is specified via a path or a file descriptor. This
>> feature is intentionally left out so as to minimize the amount of code of this
>> patch series but will come in a following series. However, it is possible to
>> check the file type, as done in the following example.
>
> I understand why you've taken a progressive approach here, but I think
> there are two fundamental areas where people will use Landlock: path
> evaluation and network address evaluation. I think it's worth
> expanding this series to include those two confinement examples, since
> that can help people understand what the "general" case will look
> like.
I agree that it would be more useful to add a path/FS evaluation,
however we agreed at LPC that it would be another patch series:
https://lkml.kernel.org/r/5828776A.1010104@digikod.net
It brings more complexity and a new kind of BPF map, which should be
reviewed in a separate series.
>
> As I mentioned in one of the patch review emails, I think there needs
> to be a clearer explanation of how usage counting works vs the
> "events" (which I think of as "rule tables" not events -- maybe it
> needs a new name?) and "rule" lists. I think I understand it, but I
> spent a lot of time trying to get there. More comments would help.
I though about different names and the previous one was PDP (for Policy
Decision Point) which is used in the literature, but "event" seems
easier to understand and close enough with "hook". Rule tables doesn't
seems to express what is the meaning to register an event/hook.
I'll add more comments to explain how it works.
>
> Finally, another thing I'm curious about is how to deal with the
> thread-sync issue. Seccomp uses its TSYNC thing, and I'd expect we'd
> want something similar for landlock... but that looks really hairy as
> far as locking goes. Perhaps it's already solved by using the same
> locking seccomp uses, in which case I'm less inclined to kick landlock
> out of seccomp.c. :)
I didn't work on it but it should be really similar to seccomp-bpf.
>
> Looks like it's coming along nicely! Thanks for continuing to work on this!
>
> -Kees
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: question about size of sk_buff and skb_shared_info
From: Code Soldier1 @ 2017-04-19 0:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1492559721.10587.150.camel@edumazet-glaptop3.roam.corp.google.com>
I am not suggesting that we do not do cache alignment. That is
required for performance. I have gone through this exercise because I
need to add a field to sk_buff and I want to do that without causing
any adverse effects.
Now that we have discovered that there are 40 bytes that can be used
without any adverse effect, may I increase skb->cb by 8 bytes ?
If not then may I increase skb_shared_info -- However that would have
to be by 64bytes.
On Tue, Apr 18, 2017 at 4:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Please do not top post on netdev
>
> On Tue, 2017-04-18 at 16:26 -0700, Code Soldier1 wrote:
>> Eric,
>>
>> This alignment flag is passed to the cache constructor and the
>> allocation is indeed cache aligned. However, since the allocated size
>> is not a multiple of the alignment, wont memory be wasted ?. We can
>> get 40 extra bytes without any side effects since they are on the same
>> cache line ?
>
> We _want_ to align skb to cache lines.
>
> Fact that few bytes might be wasted is unfortunate, but negligible.
>
> BTW, if you do kmalloc(1025), kmalloc() wastes 1023 bytes.
>
>
>>
>> kmem_cache_create() code does an ALIGN() to round up the size.
>>
>> kasan_cache_create(cachep, &size, &flags);
>>
>> size = ALIGN(size, cachep->align);
>>
>> This is the size used in calculate_slab_order() to calculate num. I am
>> assuming that in the non debug case gfp_order will be 0.
>>
>> Perhaps I am missing something.
>>
>>
>> On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
>> >> Hi Folks,
>> >>
>> >> I am sure there is a reason for the current sizes of these structures,
>> >> However the reason is not obvious to me. So please help me understand.
>> >>
>> >> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
>> >> is that. I expected it to be a multiple of 32/64 as they are the most
>> >> common cache lines. Since the alignment calculation will align the
>> >> structure with the hw cache line, it seems like we might be wasting
>> >> space ?
>> >>
>> >> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
>> >>
>> >> Thanks,
>> >>
>> >
>> > The alignment is there.
>> > Look at skb_init() code, using SLAB_HWCACHE_ALIGN
>> >
>> >
>> >
>> >
>> >
>>
>>
>>
>
>
--
CS1
^ permalink raw reply
* [PATCH net-next]smsc911x: Adding support for Micochip LAN9250 Ethernet controller
From: David.Cai @ 2017-04-19 0:29 UTC (permalink / raw)
To: netdev, davem; +Cc: UNGLinuxDriver, steve.glendinning, Ravi.Hegde, Woojung.Huh
Attached is a patch which adding support for Microchip LAN9250 Ethernet controller.
Thanks,
=====================
David Cai
Principle Software Engineer
Microchip Technology Inc.
=====================
Signed-off-by: David Cai <david.cai@microchip.com>
---
drivers/net/ethernet/smsc/smsc911x.c | 32 +++++++++++++++++++++++++++-----
drivers/net/ethernet/smsc/smsc911x.h | 3 +++
2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index fa5ca09..22b1951 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -25,7 +25,7 @@
* LAN9215, LAN9216, LAN9217, LAN9218
* LAN9210, LAN9211
* LAN9220, LAN9221
- * LAN89218
+ * LAN89218,LAN9250
*
*/
@@ -104,6 +104,9 @@ struct smsc911x_data {
/* used to decide which workarounds apply */
unsigned int generation;
+ /* used to decide which sub generation product work arounds to apply */
+ unsigned int sub_generation;
+
/* device configuration (copied from platform_data during probe) */
struct smsc911x_platform_config config;
@@ -1450,6 +1453,8 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
unsigned int timeout;
unsigned int temp;
int ret;
+ unsigned int reset_offset = HW_CFG;
+ unsigned int reset_mask = HW_CFG_SRST_;
/*
* Make sure to power-up the PHY chip before doing a reset, otherwise
@@ -1476,15 +1481,23 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
}
}
+ if (pdata->sub_generation) {
+ /* special reset for LAN9250 */
+ reset_offset = RESET_CTL;
+ reset_mask = RESET_CTL_DIGITAL_RST_;
+ }
+
/* Reset the LAN911x */
- smsc911x_reg_write(pdata, HW_CFG, HW_CFG_SRST_);
+ smsc911x_reg_write(pdata, reset_offset, reset_mask);
+
+ /* verify reset bit is cleared */
timeout = 10;
do {
udelay(10);
- temp = smsc911x_reg_read(pdata, HW_CFG);
- } while ((--timeout) && (temp & HW_CFG_SRST_));
+ temp = smsc911x_reg_read(pdata, reset_offset);
+ } while ((--timeout) && (temp & reset_mask));
- if (unlikely(temp & HW_CFG_SRST_)) {
+ if (unlikely(temp & reset_mask)) {
SMSC_WARN(pdata, drv, "Failed to complete reset");
return -EIO;
}
@@ -2251,6 +2264,9 @@ static int smsc911x_init(struct net_device *dev)
/* Default generation to zero (all workarounds apply) */
pdata->generation = 0;
+ /* Default sub_generation to zero */
+ pdata->sub_generation = 0;
+
pdata->idrev = smsc911x_reg_read(pdata, ID_REV);
switch (pdata->idrev & 0xFFFF0000) {
case 0x01180000:
@@ -2278,6 +2294,12 @@ static int smsc911x_init(struct net_device *dev)
pdata->generation = 4;
break;
+ case 0x92500000:
+ /* LAN9250 */
+ pdata->generation = 4;
+ pdata->sub_generation = 1;
+ break;
+
default:
SMSC_WARN(pdata, probe, "LAN911x not identified, idrev: 0x%08X",
pdata->idrev);
diff --git a/drivers/net/ethernet/smsc/smsc911x.h b/drivers/net/ethernet/smsc/smsc911x.h
index 54d6489..6b2d479 100644
--- a/drivers/net/ethernet/smsc/smsc911x.h
+++ b/drivers/net/ethernet/smsc/smsc911x.h
@@ -303,6 +303,9 @@
#define E2P_DATA_EEPROM_DATA_ 0x000000FF
#define LAN_REGISTER_EXTENT 0x00000100
+#define RESET_CTL 0x1F8
+#define RESET_CTL_DIGITAL_RST_ 0x00000001
+
/*
* MAC Control and Status Register (Indirect Address)
* Offset (through the MAC_CSR CMD and DATA port)
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox