Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1716037 > unrolled thread
| Started by | Mickaël Salaün <mic@digikod.net> |
|---|---|
| First post | 2017-08-21 02:20 +0200 |
| Last post | 2017-08-24 03:30 +0200 |
| Articles | 3 on this page of 23 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH net-next v7 00/10] Landlock LSM: Toward unprivileged sandboxing Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
[PATCH net-next v7 04/10] bpf: Define handle_fs and add a new helper bpf_handle_fs_get_mode() Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
[PATCH net-next v7 07/10] landlock: Add ptrace restrictions Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
[PATCH net-next v7 01/10] selftest: Enhance kselftest_harness.h with a step mechanism Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
Re: [PATCH net-next v7 01/10] selftest: Enhance kselftest_harness.h with a step mechanism Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-24 04:40 +0200
Re: [PATCH net-next v7 01/10] selftest: Enhance kselftest_harness.h with a step mechanism Mickaël Salaün <mic@digikod.net> - 2017-08-25 10:10 +0200
[PATCH net-next v7 09/10] bpf,landlock: Add tests for Landlock Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
[PATCH net-next v7 08/10] bpf: Add a Landlock sandbox example Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
Re: [PATCH net-next v7 08/10] bpf: Add a Landlock sandbox example Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-24 05:00 +0200
Re: [PATCH net-next v7 08/10] bpf: Add a Landlock sandbox example Mickaël Salaün <mic@digikod.net> - 2017-08-25 10:20 +0200
[PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem Mickaël Salaün <mic@digikod.net> - 2017-08-23 00:10 +0200
Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-24 05:00 +0200
Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem Mickaël Salaün <mic@digikod.net> - 2017-08-25 10:20 +0200
[PATCH net-next v7 03/10] bpf,landlock: Define an eBPF program type for a Landlock rule Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
Re: [PATCH net-next v7 03/10] bpf,landlock: Define an eBPF program type for a Landlock rule Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-24 04:40 +0200
Re: [PATCH net-next v7 03/10] bpf,landlock: Define an eBPF program type for a Landlock rule Mickaël Salaün <mic@digikod.net> - 2017-08-25 10:10 +0200
[PATCH net-next v7 06/10] seccomp,landlock: Handle Landlock events per process hierarchy Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
[PATCH net-next v7 10/10] landlock: Add user and kernel documentation for Landlock Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
[PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Mickaël Salaün <mic@digikod.net> - 2017-08-21 02:20 +0200
Re: [PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-23 04:50 +0200
Re: [PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Mickaël Salaün <mic@digikod.net> - 2017-08-23 09:50 +0200
Re: [PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-08-24 03:30 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-08-23 04:50 +0200 |
| Subject | Re: [PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier |
| Message-ID | <uhtTY-34H-7@gated-at.bofh.it> |
| In reply to | #1716047 |
On Mon, Aug 21, 2017 at 02:09:25AM +0200, Mickaël Salaün wrote:
> The goal of the program subtype is to be able to have different static
> fine-grained verifications for a unique program type.
>
> The struct bpf_verifier_ops gets a new optional function:
> is_valid_subtype(). This new verifier is called at the beginning of the
> eBPF program verification to check if the (optional) program subtype is
> valid.
>
> For now, only Landlock eBPF programs are using a program subtype (see
> next commit) but this could be used by other program types in the future.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Link: https://lkml.kernel.org/r/20160827205559.GA43880@ast-mbp.thefacebook.com
> ---
>
> Changes since v6:
> * rename Landlock version to ABI to better reflect its purpose
> * fix unsigned integer checks
> * fix pointer cast
> * constify pointers
> * rebase
>
> Changes since v5:
> * use a prog_subtype pointer and make it future-proof
> * add subtype test
> * constify bpf_load_program()'s subtype argument
> * cleanup subtype initialization
> * rebase
>
> Changes since v4:
> * replace the "status" field with "version" (more generic)
> * replace the "access" field with "ability" (less confusing)
>
> Changes since v3:
> * remove the "origin" field
> * add an "option" field
> * cleanup comments
> ---
> include/linux/bpf.h | 7 ++-
> include/linux/filter.h | 2 +
> include/uapi/linux/bpf.h | 11 +++++
> kernel/bpf/syscall.c | 22 ++++++++-
> kernel/bpf/verifier.c | 17 +++++--
> kernel/trace/bpf_trace.c | 15 ++++--
> net/core/filter.c | 71 ++++++++++++++++++-----------
> samples/bpf/bpf_load.c | 3 +-
> samples/bpf/cookie_uid_helper_example.c | 2 +-
> samples/bpf/fds_example.c | 2 +-
> samples/bpf/sock_example.c | 3 +-
> samples/bpf/test_cgrp2_attach.c | 2 +-
> samples/bpf/test_cgrp2_attach2.c | 2 +-
> samples/bpf/test_cgrp2_sock.c | 2 +-
> tools/include/uapi/linux/bpf.h | 11 +++++
> tools/lib/bpf/bpf.c | 10 +++-
> tools/lib/bpf/bpf.h | 5 +-
> tools/lib/bpf/libbpf.c | 4 +-
> tools/perf/tests/bpf.c | 2 +-
> tools/testing/selftests/bpf/test_align.c | 2 +-
> tools/testing/selftests/bpf/test_tag.c | 2 +-
> tools/testing/selftests/bpf/test_verifier.c | 17 ++++++-
> 22 files changed, 158 insertions(+), 56 deletions(-)
...
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 7015116331af..0c3fadbb5a58 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -464,6 +464,8 @@ struct bpf_prog {
> u32 len; /* Number of filter blocks */
> u32 jited_len; /* Size of jited insns in bytes */
> u8 tag[BPF_TAG_SIZE];
> + u8 has_subtype;
> + union bpf_prog_subtype subtype; /* Fine-grained verifications */
these burn a hole in very performance sensitive structure.
Also there are bits rigth above. use them instead of u8 has_subtype?
or can these two fields be part of bpf_prog_aux ?
> struct bpf_prog_aux *aux; /* Auxiliary fields */
> struct sock_fprog_kern *orig_prog; /* Original BPF program */
> unsigned int (*bpf_func)(const void *ctx,
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 843818dff96d..8541ab85e432 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -177,6 +177,15 @@ enum bpf_attach_type {
> /* Specify numa node during map creation */
> #define BPF_F_NUMA_NODE (1U << 2)
>
> +union bpf_prog_subtype {
> + struct {
> + __u32 abi; /* minimal ABI version, cf. user doc */
the concept of abi (version) sounds a bit weird to me.
Why bother with it at all?
Once the first set of patches lands the kernel as whole will have landlock feature
with a set of helpers, actions, event types.
Some future patches will extend the landlock feature step by step.
This abi concept assumes that anyone who adds new helper would need
to keep incrementing this 'abi'. What value does it give to user or to kernel?
The users will already know that landlock is present in kernel 4.14 or whatever
and the kernel 4.18 has more landlock features. Why bother with extra abi number?
> + __u32 event; /* enum landlock_subtype_event */
> + __aligned_u64 ability; /* LANDLOCK_SUBTYPE_ABILITY_* */
> + __aligned_u64 option; /* LANDLOCK_SUBTYPE_OPTION_* */
> + } landlock_rule;
> +} __attribute__((aligned(8)));
> +
> union bpf_attr {
> struct { /* anonymous struct used by BPF_MAP_CREATE command */
> __u32 map_type; /* one of enum bpf_map_type */
> @@ -212,6 +221,8 @@ union bpf_attr {
> __aligned_u64 log_buf; /* user supplied buffer */
> __u32 kern_version; /* checked when prog_type=kprobe */
> __u32 prog_flags;
> + __aligned_u64 prog_subtype; /* bpf_prog_subtype address */
> + __u32 prog_subtype_size;
> };
more general question: what is the status of security/ bits?
I'm assuming they still need to be reviewed and explicitly acked by James, right?
[toc] | [prev] | [next] | [standalone]
| From | Mickaël Salaün <mic@digikod.net> |
|---|---|
| Date | 2017-08-23 09:50 +0200 |
| Subject | Re: [PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier |
| Message-ID | <uhyAj-66m-31@gated-at.bofh.it> |
| In reply to | #1717994 |
[Multipart message — attachments visible in raw view] — view raw
On 23/08/2017 04:44, Alexei Starovoitov wrote:
> On Mon, Aug 21, 2017 at 02:09:25AM +0200, Mickaël Salaün wrote:
>> The goal of the program subtype is to be able to have different static
>> fine-grained verifications for a unique program type.
>>
>> The struct bpf_verifier_ops gets a new optional function:
>> is_valid_subtype(). This new verifier is called at the beginning of the
>> eBPF program verification to check if the (optional) program subtype is
>> valid.
>>
>> For now, only Landlock eBPF programs are using a program subtype (see
>> next commit) but this could be used by other program types in the future.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Link: https://lkml.kernel.org/r/20160827205559.GA43880@ast-mbp.thefacebook.com
>> ---
>>
>> Changes since v6:
>> * rename Landlock version to ABI to better reflect its purpose
>> * fix unsigned integer checks
>> * fix pointer cast
>> * constify pointers
>> * rebase
>>
>> Changes since v5:
>> * use a prog_subtype pointer and make it future-proof
>> * add subtype test
>> * constify bpf_load_program()'s subtype argument
>> * cleanup subtype initialization
>> * rebase
>>
>> Changes since v4:
>> * replace the "status" field with "version" (more generic)
>> * replace the "access" field with "ability" (less confusing)
>>
>> Changes since v3:
>> * remove the "origin" field
>> * add an "option" field
>> * cleanup comments
>> ---
>> include/linux/bpf.h | 7 ++-
>> include/linux/filter.h | 2 +
>> include/uapi/linux/bpf.h | 11 +++++
>> kernel/bpf/syscall.c | 22 ++++++++-
>> kernel/bpf/verifier.c | 17 +++++--
>> kernel/trace/bpf_trace.c | 15 ++++--
>> net/core/filter.c | 71 ++++++++++++++++++-----------
>> samples/bpf/bpf_load.c | 3 +-
>> samples/bpf/cookie_uid_helper_example.c | 2 +-
>> samples/bpf/fds_example.c | 2 +-
>> samples/bpf/sock_example.c | 3 +-
>> samples/bpf/test_cgrp2_attach.c | 2 +-
>> samples/bpf/test_cgrp2_attach2.c | 2 +-
>> samples/bpf/test_cgrp2_sock.c | 2 +-
>> tools/include/uapi/linux/bpf.h | 11 +++++
>> tools/lib/bpf/bpf.c | 10 +++-
>> tools/lib/bpf/bpf.h | 5 +-
>> tools/lib/bpf/libbpf.c | 4 +-
>> tools/perf/tests/bpf.c | 2 +-
>> tools/testing/selftests/bpf/test_align.c | 2 +-
>> tools/testing/selftests/bpf/test_tag.c | 2 +-
>> tools/testing/selftests/bpf/test_verifier.c | 17 ++++++-
>> 22 files changed, 158 insertions(+), 56 deletions(-)
>
> ...
>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index 7015116331af..0c3fadbb5a58 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -464,6 +464,8 @@ struct bpf_prog {
>> u32 len; /* Number of filter blocks */
>> u32 jited_len; /* Size of jited insns in bytes */
>> u8 tag[BPF_TAG_SIZE];
>> + u8 has_subtype;
>> + union bpf_prog_subtype subtype; /* Fine-grained verifications */
>
> these burn a hole in very performance sensitive structure.
> Also there are bits rigth above. use them instead of u8 has_subtype?
> or can these two fields be part of bpf_prog_aux ?
OK, I'll create one bit variable and a bpf_prog_subtype field in the
bpf_prog_aux struct then.
>
>> struct bpf_prog_aux *aux; /* Auxiliary fields */
>> struct sock_fprog_kern *orig_prog; /* Original BPF program */
>> unsigned int (*bpf_func)(const void *ctx,
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 843818dff96d..8541ab85e432 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -177,6 +177,15 @@ enum bpf_attach_type {
>> /* Specify numa node during map creation */
>> #define BPF_F_NUMA_NODE (1U << 2)
>>
>> +union bpf_prog_subtype {
>> + struct {
>> + __u32 abi; /* minimal ABI version, cf. user doc */
>
> the concept of abi (version) sounds a bit weird to me.
> Why bother with it at all?
> Once the first set of patches lands the kernel as whole will have landlock feature
> with a set of helpers, actions, event types.
> Some future patches will extend the landlock feature step by step.
> This abi concept assumes that anyone who adds new helper would need
> to keep incrementing this 'abi'. What value does it give to user or to kernel?
> The users will already know that landlock is present in kernel 4.14 or whatever
> and the kernel 4.18 has more landlock features. Why bother with extra abi number?
That's right for helpers and context fields, but we can't check the use
of one field's content. The status field is intended to be a bitfield
extendable in the future. For example, one use case is to set a flag to
inform the eBPF program that it was already called with the same context
and can skip most of its check (if not related to maps). Same goes for
the FS action bitfield, one may want to add more of them. Another
example may be the check for abilities. We may want to relax/remove the
capability require to set one of them. With an ABI version, the user can
easily check if the current kernel support that.
>
>> + __u32 event; /* enum landlock_subtype_event */
>> + __aligned_u64 ability; /* LANDLOCK_SUBTYPE_ABILITY_* */
>> + __aligned_u64 option; /* LANDLOCK_SUBTYPE_OPTION_* */
>> + } landlock_rule;
>> +} __attribute__((aligned(8)));
>> +
>> union bpf_attr {
>> struct { /* anonymous struct used by BPF_MAP_CREATE command */
>> __u32 map_type; /* one of enum bpf_map_type */
>> @@ -212,6 +221,8 @@ union bpf_attr {
>> __aligned_u64 log_buf; /* user supplied buffer */
>> __u32 kern_version; /* checked when prog_type=kprobe */
>> __u32 prog_flags;
>> + __aligned_u64 prog_subtype; /* bpf_prog_subtype address */
>> + __u32 prog_subtype_size;
>> };
>
> more general question: what is the status of security/ bits?
> I'm assuming they still need to be reviewed and explicitly acked by James, right?
Right, the review process is ongoing. :)
BTW, I'll be at Linux Security Summit (co-located with Plumbers) next
month. We'll be able to clarify some points there too.
Regards,
Mickaël
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-08-24 03:30 +0200 |
| Subject | Re: [PATCH net-next v7 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier |
| Message-ID | <uhP85-87D-1@gated-at.bofh.it> |
| In reply to | #1718112 |
On Wed, Aug 23, 2017 at 09:45:24AM +0200, Mickaël Salaün wrote:
> >>
> >> +union bpf_prog_subtype {
> >> + struct {
> >> + __u32 abi; /* minimal ABI version, cf. user doc */
> >
> > the concept of abi (version) sounds a bit weird to me.
> > Why bother with it at all?
> > Once the first set of patches lands the kernel as whole will have landlock feature
> > with a set of helpers, actions, event types.
> > Some future patches will extend the landlock feature step by step.
> > This abi concept assumes that anyone who adds new helper would need
> > to keep incrementing this 'abi'. What value does it give to user or to kernel?
> > The users will already know that landlock is present in kernel 4.14 or whatever
> > and the kernel 4.18 has more landlock features. Why bother with extra abi number?
>
> That's right for helpers and context fields, but we can't check the use
> of one field's content. The status field is intended to be a bitfield
> extendable in the future. For example, one use case is to set a flag to
> inform the eBPF program that it was already called with the same context
> and can skip most of its check (if not related to maps). Same goes for
'status' field ? I don't see it in the current patch set.
You mean something like scratch space in landlock_ctx that
program can write to? Sure, that's a good extension.
> the FS action bitfield, one may want to add more of them. Another
> example may be the check for abilities. We may want to relax/remove the
> capability require to set one of them. With an ABI version, the user can
> easily check if the current kernel support that.
sure. there will be future extensions. I still fail to see
why 'abi' field is needed.
Also consider that bpf core itself is being extended all the time as well.
The verifier gets smarter and smarter, so the programs deemed unsafe
a year ago now recognized properly by the verifier.
New instructions being added to the core and so on.
That means that newer landlock programs will not be acceptable by
older kernels. We cannot increment abi/version with every such change.
It's also possible that in the future we may catch a security bug
in the verifier that will start rejecting some corner case of the programs.
The only way to use landlock is to develop a set of programs/rules
for kernel version X and we together will guarantee that these programs
will work fine in the future kernels.
There is a good chance that the rules developed for kernel X+1 will _not_
be loadable on older kernel X even if you don't change anything on
landlock side (helpers, actions, events), so landlock abi/version will
stay the same but you won't get the effect you're looking to get from
this abi concept. Since landlock abi=1 in kernel X and abi=1 in kernel X+1
doesn't mean that landlock rules developed for X+1 will work on X.
Beyond bpf core there are other moving pieces. LSM may get new hooks,
seccomp side will be changed, etc. Incrementing landlock abi is not pracitcal.
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web