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.ander...@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.ander...@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 netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
