Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221538 > unrolled thread

Re: [PATCH 1/6] ebpf: add a seccomp program type

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/6] ebpf: add a seccomp program type Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-09 18:00 +0200
    Re: [PATCH 1/6] ebpf: add a seccomp program type Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-09 18:10 +0200
      Re: [PATCH 1/6] ebpf: add a seccomp program type Daniel Borkmann <daniel@iogearbox.net> - 2015-09-09 18:10 +0200
        Re: [PATCH 1/6] ebpf: add a seccomp program type Kees Cook <keescook@chromium.org> - 2015-09-09 18:40 +0200
          Re: [PATCH 1/6] ebpf: add a seccomp program type Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-09 19:00 +0200
            Re: [PATCH 1/6] ebpf: add a seccomp program type Kees Cook <keescook@chromium.org> - 2015-09-09 19:30 +0200
              Re: [PATCH 1/6] ebpf: add a seccomp program type Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-09 19:40 +0200
    Re: [PATCH 1/6] ebpf: add a seccomp program type Daniel Borkmann <daniel@iogearbox.net> - 2015-09-09 18:10 +0200

#1221538 — Re: [PATCH 1/6] ebpf: add a seccomp program type

On Fri, Sep 04, 2015 at 02:08:37PM -0700, Kees Cook wrote:
> On Fri, Sep 4, 2015 at 2:06 PM, Tycho Andersen
> <tycho.andersen@canonical.com> wrote:
> > On Fri, Sep 04, 2015 at 01:34:12PM -0700, Kees Cook wrote:
> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
> >> <tycho.andersen@canonical.com> wrote:
> >> > +static const struct bpf_func_proto *
> >> > +seccomp_func_proto(enum bpf_func_id func_id)
> >> > +{
> >> > +       /* Right now seccomp eBPF loading doesn't support maps; seccomp filters
> >> > +        * are considered to be read-only after they're installed, so map fds
> >> > +        * probably need to be invalidated when a seccomp filter with maps is
> >> > +        * installed.
> >> > +        *
> >> > +        * The rest of these might be reasonable to call from seccomp, so we
> >> > +        * export them.
> >> > +        */
> >> > +       switch (func_id) {
> >> > +       case BPF_FUNC_ktime_get_ns:
> >> > +               return &bpf_ktime_get_ns_proto;
> >> > +       case BPF_FUNC_trace_printk:
> >> > +               return bpf_get_trace_printk_proto();
> >> > +       case BPF_FUNC_get_prandom_u32:
> >> > +               return &bpf_get_prandom_u32_proto;
> >> > +       case BPF_FUNC_get_smp_processor_id:
> >> > +               return &bpf_get_smp_processor_id_proto;
> >> > +       case BPF_FUNC_tail_call:
> >> > +               return &bpf_tail_call_proto;
> >> > +       case BPF_FUNC_get_current_pid_tgid:
> >> > +               return &bpf_get_current_pid_tgid_proto;
> >> > +       case BPF_FUNC_get_current_uid_gid:
> >> > +               return &bpf_get_current_uid_gid_proto;
> >> > +       case BPF_FUNC_get_current_comm:
> >> > +               return &bpf_get_current_comm_proto;
> >> > +       default:
> >> > +               return NULL;
> >> > +       }
> >> > +}
> >>
> >> While this list is probably fine, I don't want to mix the addition of
> >> eBPF functions to the seccomp ABI with the CRIU changes. No function
> >> calls are currently possible and it should stay that way.
> >
> > Ok, I can remove them.
> >
> >> I was expecting to see a validator, similar to the existing BPF
> >> validator that is called when creating seccomp filters currently. Can
> >> we add a similar validator for new BPF_PROG_TYPE_SECCOMP?
> >
> > That's effectively what this patch does; when the eBPF is loaded via
> > bpf(), you tell bpf() you want a BPF_PROG_TYPE_SECCOMP, and it invokes
> > this validation/translation code, i.e. it uses
> > seccomp_is_valid_access() to check and make sure access are aligned
> > and inside struct seccomp_data.
> 
> What about limiting the possible instructions?

I totally overlooked this. A quick glance through the eBPF verifier
makes me think that we can just add another function to struct
bpf_verifier_ops called valid_instruction, which shouldn't be too
hard. Perhaps a more interesting question is what to allow:

BPF_LD(X) and BPF_ST(X): it looks like all types of stores are
  allowed, and only BPF_MEM and BPF_IMM loads are allowed; I think
  these can stay the same. BPF_XADD is new in eBPF, and I don't think
  we need it for seccomp (yet), since we don't have any shared memory
  via maps.

BPF_ALU: It looks like we're also not allowing regular BPF_ALU
  instruction BPF_MOD; eBPF adds a few ones: BPF_MOV (register move),
  BPF_ARSH (sign extended right shift), and BPF_END (endianness
  conversion), wich I think should all be safe. In particular, we need
  to allow BPF_MOV at least, since that's how the converter implements
  BPF_MISC | BPF_TAX from classic.

BPF_ALU64: I think we can safely allow all these as above, since
  they're just the 64-bit versions.

BPF_JMP: eBPF adds BPF_JNE, BPF_JSGT, BPF_JSGE, BPF_CALL, and
  BPF_EXIT, which I think all should be safe (except maybe BPF_CALL
  since we're not allowing functions really). Again we have to allow
  one of the new eBPF codes, as the converter implements BPF_RET as
  BPF_JMP | BPF_EXIT.

Thoughts?

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221544

On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote:
> > >
> > > That's effectively what this patch does; when the eBPF is loaded via
> > > bpf(), you tell bpf() you want a BPF_PROG_TYPE_SECCOMP, and it invokes
> > > this validation/translation code, i.e. it uses
> > > seccomp_is_valid_access() to check and make sure access are aligned
> > > and inside struct seccomp_data.
> > 
> > What about limiting the possible instructions?
> 
> I totally overlooked this. A quick glance through the eBPF verifier
> makes me think that we can just add another function to struct
> bpf_verifier_ops called valid_instruction, which shouldn't be too
> hard. Perhaps a more interesting question is what to allow:
> 
> BPF_LD(X) and BPF_ST(X): it looks like all types of stores are
>   allowed, and only BPF_MEM and BPF_IMM loads are allowed; I think
>   these can stay the same. BPF_XADD is new in eBPF, and I don't think
>   we need it for seccomp (yet), since we don't have any shared memory
>   via maps.
> 
> BPF_ALU: It looks like we're also not allowing regular BPF_ALU
>   instruction BPF_MOD; eBPF adds a few ones: BPF_MOV (register move),
>   BPF_ARSH (sign extended right shift), and BPF_END (endianness
>   conversion), wich I think should all be safe. In particular, we need
>   to allow BPF_MOV at least, since that's how the converter implements
>   BPF_MISC | BPF_TAX from classic.
> 
> BPF_ALU64: I think we can safely allow all these as above, since
>   they're just the 64-bit versions.
> 
> BPF_JMP: eBPF adds BPF_JNE, BPF_JSGT, BPF_JSGE, BPF_CALL, and
>   BPF_EXIT, which I think all should be safe (except maybe BPF_CALL
>   since we're not allowing functions really). Again we have to allow
>   one of the new eBPF codes, as the converter implements BPF_RET as
>   BPF_JMP | BPF_EXIT.
> 
> Thoughts?

Please do not add any per-instruction hacks. None of them are
necessary. Classic had to do extra ugly checks in seccomp only
because verifier wasn't flexible enough.
If you don't want to see any BPF_CALL in seccomp, just have
empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP
and verifier will reject all calls.
Currently we have only two non-generic instrucitons
LD_ABS and LD_IND that are avaialable for sockets/TC only,
because these are legacy instructions and we had to make
exceptions for them.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221546

On 09/09/2015 06:07 PM, Alexei Starovoitov wrote:
> On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote:
[...]
>> Thoughts?
>
> Please do not add any per-instruction hacks. None of them are
> necessary. Classic had to do extra ugly checks in seccomp only
> because verifier wasn't flexible enough.
> If you don't want to see any BPF_CALL in seccomp, just have
> empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP
> and verifier will reject all calls.
> Currently we have only two non-generic instrucitons
> LD_ABS and LD_IND that are avaialable for sockets/TC only,
> because these are legacy instructions and we had to make
> exceptions for them.

Yep, +1.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221557

On Wed, Sep 9, 2015 at 9:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 09/09/2015 06:07 PM, Alexei Starovoitov wrote:
>>
>> On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote:
>
> [...]
>>>
>>> Thoughts?
>>
>>
>> Please do not add any per-instruction hacks. None of them are
>> necessary. Classic had to do extra ugly checks in seccomp only
>> because verifier wasn't flexible enough.
>> If you don't want to see any BPF_CALL in seccomp, just have
>> empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP
>> and verifier will reject all calls.
>> Currently we have only two non-generic instrucitons
>> LD_ABS and LD_IND that are avaialable for sockets/TC only,
>> because these are legacy instructions and we had to make
>> exceptions for them.
>
> Yep, +1.

Hrmpf. This adds to the cognitive load for accepting this patch
series. :P Now I have to convince myself that there is no additional
exposure to seccomp by using the entire set of eBPF instructions.
While I'm pretty sure it'll be fine, I really don't want to risk being
wrong and opening a hole here. I will spend some time looking at the
new eBPF instructions...

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221568

On Wed, Sep 09, 2015 at 09:37:51AM -0700, Kees Cook wrote:
> On Wed, Sep 9, 2015 at 9:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > On 09/09/2015 06:07 PM, Alexei Starovoitov wrote:
> >>
> >> On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote:
> >
> > [...]
> >>>
> >>> Thoughts?
> >>
> >>
> >> Please do not add any per-instruction hacks. None of them are
> >> necessary. Classic had to do extra ugly checks in seccomp only
> >> because verifier wasn't flexible enough.
> >> If you don't want to see any BPF_CALL in seccomp, just have
> >> empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP
> >> and verifier will reject all calls.
> >> Currently we have only two non-generic instrucitons
> >> LD_ABS and LD_IND that are avaialable for sockets/TC only,
> >> because these are legacy instructions and we had to make
> >> exceptions for them.
> >
> > Yep, +1.
> 
> Hrmpf. This adds to the cognitive load for accepting this patch
> series. :P Now I have to convince myself that there is no additional
> exposure to seccomp by using the entire set of eBPF instructions.
> While I'm pretty sure it'll be fine, I really don't want to risk being
> wrong and opening a hole here. I will spend some time looking at the
> new eBPF instructions...

note, as was discussed many times before, there is no pointer leak
prevention pass yet, so eBPF is root only.
Once the pass is complete it will prevent passing addresses to
functions, storing them in maps and returning from the program.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221582

On Wed, Sep 9, 2015 at 9:52 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Wed, Sep 09, 2015 at 09:37:51AM -0700, Kees Cook wrote:
>> On Wed, Sep 9, 2015 at 9:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> > On 09/09/2015 06:07 PM, Alexei Starovoitov wrote:
>> >>
>> >> On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote:
>> >
>> > [...]
>> >>>
>> >>> Thoughts?
>> >>
>> >>
>> >> Please do not add any per-instruction hacks. None of them are
>> >> necessary. Classic had to do extra ugly checks in seccomp only
>> >> because verifier wasn't flexible enough.
>> >> If you don't want to see any BPF_CALL in seccomp, just have
>> >> empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP
>> >> and verifier will reject all calls.
>> >> Currently we have only two non-generic instrucitons
>> >> LD_ABS and LD_IND that are avaialable for sockets/TC only,
>> >> because these are legacy instructions and we had to make
>> >> exceptions for them.
>> >
>> > Yep, +1.
>>
>> Hrmpf. This adds to the cognitive load for accepting this patch
>> series. :P Now I have to convince myself that there is no additional
>> exposure to seccomp by using the entire set of eBPF instructions.
>> While I'm pretty sure it'll be fine, I really don't want to risk being
>> wrong and opening a hole here. I will spend some time looking at the
>> new eBPF instructions...
>
> note, as was discussed many times before, there is no pointer leak
> prevention pass yet, so eBPF is root only.
> Once the pass is complete it will prevent passing addresses to
> functions, storing them in maps and returning from the program.

Tycho, are you building new eBPF filters as the root user and then
attaching them later? I was imagining you were going to need this
entirely as non-root.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221594

On Wed, Sep 09, 2015 at 10:27:08AM -0700, Kees Cook wrote:
> On Wed, Sep 9, 2015 at 9:52 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Wed, Sep 09, 2015 at 09:37:51AM -0700, Kees Cook wrote:
> >> On Wed, Sep 9, 2015 at 9:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> > On 09/09/2015 06:07 PM, Alexei Starovoitov wrote:
> >> >>
> >> >> On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote:
> >> >
> >> > [...]
> >> >>>
> >> >>> Thoughts?
> >> >>
> >> >>
> >> >> Please do not add any per-instruction hacks. None of them are
> >> >> necessary. Classic had to do extra ugly checks in seccomp only
> >> >> because verifier wasn't flexible enough.
> >> >> If you don't want to see any BPF_CALL in seccomp, just have
> >> >> empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP
> >> >> and verifier will reject all calls.
> >> >> Currently we have only two non-generic instrucitons
> >> >> LD_ABS and LD_IND that are avaialable for sockets/TC only,
> >> >> because these are legacy instructions and we had to make
> >> >> exceptions for them.
> >> >
> >> > Yep, +1.
> >>
> >> Hrmpf. This adds to the cognitive load for accepting this patch
> >> series. :P Now I have to convince myself that there is no additional
> >> exposure to seccomp by using the entire set of eBPF instructions.
> >> While I'm pretty sure it'll be fine, I really don't want to risk being
> >> wrong and opening a hole here. I will spend some time looking at the
> >> new eBPF instructions...
> >
> > note, as was discussed many times before, there is no pointer leak
> > prevention pass yet, so eBPF is root only.
> > Once the pass is complete it will prevent passing addresses to
> > functions, storing them in maps and returning from the program.
> 
> Tycho, are you building new eBPF filters as the root user and then
> attaching them later?

Yep, exactly.

> I was imagining you were going to need this entirely as non-root.

We can't exactly because of the real root restriction on bpf(). So we
just open the bpf fds when we're real root and keep them until the
very end of the restore when we finally install the seccomp filters.
Not ideal, of course, but I assume we can change how this works once
bpf() deemed safe for non-root.

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221547

On 09/09/2015 05:50 PM, Tycho Andersen wrote:
> On Fri, Sep 04, 2015 at 02:08:37PM -0700, Kees Cook wrote:
>> On Fri, Sep 4, 2015 at 2:06 PM, Tycho Andersen
[...]
>>>> I was expecting to see a validator, similar to the existing BPF
>>>> validator that is called when creating seccomp filters currently. Can
>>>> we add a similar validator for new BPF_PROG_TYPE_SECCOMP?
>>>
>>> That's effectively what this patch does; when the eBPF is loaded via
>>> bpf(), you tell bpf() you want a BPF_PROG_TYPE_SECCOMP, and it invokes
>>> this validation/translation code, i.e. it uses
>>> seccomp_is_valid_access() to check and make sure access are aligned
>>> and inside struct seccomp_data.
>>
>> What about limiting the possible instructions?
>
> I totally overlooked this. A quick glance through the eBPF verifier
> makes me think that we can just add another function to struct
> bpf_verifier_ops called valid_instruction, which shouldn't be too
> hard. Perhaps a more interesting question is what to allow:

It's possible, but keep in mind that when you disallow various
instructions from the base insns set, you won't be able to leverage
filter creation in the minimal C subset via clang/llvm anymore, so
usability would suffer from this side, even if you just use clang/llvm
to create the raw insns and later keep them in your application
directly.

And if you later on decide to allow maps, etc, hacking this together
by hand is a bit of a pain. ;)

[ Restricting helper functions and ctx access, etc via bpf_verifier_ops
   (as you can currently do) should not affect this. ]

> BPF_LD(X) and BPF_ST(X): it looks like all types of stores are
>    allowed, and only BPF_MEM and BPF_IMM loads are allowed; I think
>    these can stay the same. BPF_XADD is new in eBPF, and I don't think
>    we need it for seccomp (yet), since we don't have any shared memory
>    via maps.
>
> BPF_ALU: It looks like we're also not allowing regular BPF_ALU
>    instruction BPF_MOD; eBPF adds a few ones: BPF_MOV (register move),
>    BPF_ARSH (sign extended right shift), and BPF_END (endianness
>    conversion), wich I think should all be safe. In particular, we need
>    to allow BPF_MOV at least, since that's how the converter implements
>    BPF_MISC | BPF_TAX from classic.
>
> BPF_ALU64: I think we can safely allow all these as above, since
>    they're just the 64-bit versions.
>
> BPF_JMP: eBPF adds BPF_JNE, BPF_JSGT, BPF_JSGE, BPF_CALL, and
>    BPF_EXIT, which I think all should be safe (except maybe BPF_CALL
>    since we're not allowing functions really). Again we have to allow
>    one of the new eBPF codes, as the converter implements BPF_RET as
>    BPF_JMP | BPF_EXIT.
>
> Thoughts?
>
> Tycho

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web