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

Groups > linux.kernel > #1219143 > unrolled thread

c/r of seccomp filters via underlying eBPF

Back to article view | Back to linux.kernel

Contents

  c/r of seccomp filters via underlying eBPF Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 18:10 +0200
    [PATCH 5/6] seccomp: add a way to attach a filter via eBPF fd Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 18:10 +0200
      Re: [PATCH 5/6] seccomp: add a way to attach a filter via eBPF fd Kees Cook <keescook@chromium.org> - 2015-09-04 22:50 +0200
        Re: [PATCH 5/6] seccomp: add a way to attach a filter via eBPF fd "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2015-09-05 09:20 +0200
      Re: [PATCH 5/6] seccomp: add a way to attach a filter via eBPF fd Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-04 22:50 +0200
    [PATCH 4/6] seccomp: add a way to access filters via bpf fds Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 18:10 +0200
      Re: [PATCH 4/6] seccomp: add a way to access filters via bpf fds Kees Cook <keescook@chromium.org> - 2015-09-04 22:30 +0200
        Re: [PATCH 4/6] seccomp: add a way to access filters via bpf fds Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-04 22:40 +0200
          Re: [PATCH 4/6] seccomp: add a way to access filters via bpf fds Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 23:00 +0200
    [PATCH 6/6] ebpf: allow BPF_REG_X in src_reg conditional jumps Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 18:10 +0200
      Re: [PATCH 6/6] ebpf: allow BPF_REG_X in src_reg conditional jumps Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-04 23:10 +0200
        Re: [PATCH 6/6] ebpf: allow BPF_REG_X in src_reg conditional jumps Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-05 00:50 +0200
          Re: [PATCH 6/6] ebpf: allow BPF_REG_X in src_reg conditional jumps Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-05 06:20 +0200
    [PATCH 1/6] ebpf: add a seccomp program type Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 18:10 +0200
      Re: [PATCH 1/6] ebpf: add a seccomp program type Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-04 22:20 +0200
        Re: [PATCH 1/6] ebpf: add a seccomp program type Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 23:10 +0200
      Re: [PATCH 1/6] ebpf: add a seccomp program type Kees Cook <keescook@chromium.org> - 2015-09-04 22:40 +0200
        Re: [PATCH 1/6] ebpf: add a seccomp program type Kees Cook <keescook@chromium.org> - 2015-09-04 23:10 +0200
        Re: [PATCH 1/6] ebpf: add a seccomp program type Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 23:10 +0200
      Re: [PATCH 1/6] ebpf: add a seccomp program type Andy Lutomirski <luto@amacapital.net> - 2015-09-05 00:00 +0200
    [PATCH 3/6] ebpf: add a way to dump an eBPF program Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 18:10 +0200
      Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Kees Cook <keescook@chromium.org> - 2015-09-04 22:20 +0200
        Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 22:50 +0200
          Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-04 23:00 +0200
            Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 23:10 +0200
          Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Kees Cook <keescook@chromium.org> - 2015-09-04 23:00 +0200
          Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Andy Lutomirski <luto@amacapital.net> - 2015-09-04 23:50 +0200
            Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-05 00:30 +0200
              Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Andy Lutomirski <luto@amacapital.net> - 2015-09-05 01:10 +0200
                Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-05 02:30 +0200
              Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Kees Cook <keescook@chromium.org> - 2015-09-05 01:30 +0200
                Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Andy Lutomirski <luto@amacapital.net> - 2015-09-05 02:10 +0200
      Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-09-04 22:30 +0200
        Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program Tycho Andersen <tycho.andersen@canonical.com> - 2015-09-04 22:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1219148 — [PATCH 3/6] ebpf: add a way to dump an eBPF program

This commit adds a way to dump eBPF programs. The initial implementation
doesn't support maps, and therefore only allows dumping seccomp ebpf
programs which themselves don't currently support maps.

We export the GPL bit as well as a unique ID for the program so that
userspace can detect when two seccomp filters were inherited from each
other and clone the filter tree accordingly.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
CC: Kees Cook <keescook@chromium.org>
CC: Will Drewry <wad@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
CC: Alexei Starovoitov <ast@kernel.org>
CC: Daniel Borkmann <daniel@iogearbox.net>
---
 include/uapi/linux/bpf.h | 15 +++++++++++++++
 kernel/bpf/syscall.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 79b825a..c5d8dc2 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -107,6 +107,13 @@ enum bpf_cmd {
 	 * returns fd or negative error
 	 */
 	BPF_PROG_LOAD,
+
+	/* dump an existing bpf
+	 * err = bpf(BPF_PROG_DUMP, union bpf_attr *attr, u32 size)
+	 * Using attr->prog_fd, attr->dump_insn_cnt, attr->dump_insns
+	 * returns zero or negative error
+	 */
+	BPF_PROG_DUMP,
 };
 
 enum bpf_map_type {
@@ -160,6 +167,14 @@ union bpf_attr {
 		__aligned_u64	log_buf;	/* user supplied buffer */
 		__u32		kern_version;	/* checked when prog_type=kprobe */
 	};
+
+	struct { /* anonymous struct used by BPF_PROG_DUMP command */
+		__u32		prog_fd;
+		__u32		dump_insn_cnt;
+		__aligned_u64	dump_insns;	/* user supplied buffer */
+		__u8		gpl_compatible;
+		__u64		prog_id;	/* unique id for this prog */
+	};
 } __attribute__((aligned(8)));
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a1b14d1..ee580d0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -586,6 +586,47 @@ free_prog:
 	return err;
 }
 
+static int bpf_prog_dump(union bpf_attr *attr, union __user bpf_attr *uattr)
+{
+	int ufd = attr->prog_fd;
+	struct fd f = fdget(ufd);
+	struct bpf_prog *prog;
+	int ret = -EINVAL;
+
+	prog = get_prog(f);
+	if (IS_ERR(prog))
+		return PTR_ERR(prog);
+
+	/* For now, let's refuse to dump anything that isn't a seccomp program.
+	 * Other program types have support for maps, which our current dump
+	 * code doesn't support.
+	 */
+	if (prog->type != BPF_PROG_TYPE_SECCOMP)
+		goto out;
+
+	ret = -EFAULT;
+	if (put_user(prog->len, &uattr->dump_insn_cnt))
+		goto out;
+
+	if (put_user((u8) prog->gpl_compatible, &uattr->gpl_compatible))
+		goto out;
+
+	if (put_user((u64) prog, &uattr->prog_id))
+		goto out;
+
+	if (attr->dump_insns) {
+		u32 len = prog->len * sizeof(struct bpf_insn);
+
+		if (copy_to_user(u64_to_ptr(attr->dump_insns),
+				 prog->insns, len) != 0)
+			goto out;
+	}
+
+	ret = 0;
+out:
+	return ret;
+}
+
 SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
 {
 	union bpf_attr attr = {};
@@ -650,6 +691,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
 	case BPF_PROG_LOAD:
 		err = bpf_prog_load(&attr);
 		break;
+	case BPF_PROG_DUMP:
+		err = bpf_prog_dump(&attr, uattr);
+		break;
 	default:
 		err = -EINVAL;
 		break;
-- 
2.1.4

--
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]

#1219285 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
<tycho.andersen@canonical.com> wrote:
> This commit adds a way to dump eBPF programs. The initial implementation
> doesn't support maps, and therefore only allows dumping seccomp ebpf
> programs which themselves don't currently support maps.
>
> We export the GPL bit as well as a unique ID for the program so that

This unique ID appears to be the heap address for the prog. That's a
huge leak, and should not be done. We don't want to introduce new
kernel address leaks while we're trying to fix the remaining ones.
Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE could
be used, for example.

-Kees

> userspace can detect when two seccomp filters were inherited from each
> other and clone the filter tree accordingly.
>
> Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Will Drewry <wad@chromium.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> CC: Alexei Starovoitov <ast@kernel.org>
> CC: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  include/uapi/linux/bpf.h | 15 +++++++++++++++
>  kernel/bpf/syscall.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 79b825a..c5d8dc2 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -107,6 +107,13 @@ enum bpf_cmd {
>          * returns fd or negative error
>          */
>         BPF_PROG_LOAD,
> +
> +       /* dump an existing bpf
> +        * err = bpf(BPF_PROG_DUMP, union bpf_attr *attr, u32 size)
> +        * Using attr->prog_fd, attr->dump_insn_cnt, attr->dump_insns
> +        * returns zero or negative error
> +        */
> +       BPF_PROG_DUMP,
>  };
>
>  enum bpf_map_type {
> @@ -160,6 +167,14 @@ union bpf_attr {
>                 __aligned_u64   log_buf;        /* user supplied buffer */
>                 __u32           kern_version;   /* checked when prog_type=kprobe */
>         };
> +
> +       struct { /* anonymous struct used by BPF_PROG_DUMP command */
> +               __u32           prog_fd;
> +               __u32           dump_insn_cnt;
> +               __aligned_u64   dump_insns;     /* user supplied buffer */
> +               __u8            gpl_compatible;
> +               __u64           prog_id;        /* unique id for this prog */
> +       };
>  } __attribute__((aligned(8)));
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a1b14d1..ee580d0 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -586,6 +586,47 @@ free_prog:
>         return err;
>  }
>
> +static int bpf_prog_dump(union bpf_attr *attr, union __user bpf_attr *uattr)
> +{
> +       int ufd = attr->prog_fd;
> +       struct fd f = fdget(ufd);
> +       struct bpf_prog *prog;
> +       int ret = -EINVAL;
> +
> +       prog = get_prog(f);
> +       if (IS_ERR(prog))
> +               return PTR_ERR(prog);
> +
> +       /* For now, let's refuse to dump anything that isn't a seccomp program.
> +        * Other program types have support for maps, which our current dump
> +        * code doesn't support.
> +        */
> +       if (prog->type != BPF_PROG_TYPE_SECCOMP)
> +               goto out;
> +
> +       ret = -EFAULT;
> +       if (put_user(prog->len, &uattr->dump_insn_cnt))
> +               goto out;
> +
> +       if (put_user((u8) prog->gpl_compatible, &uattr->gpl_compatible))
> +               goto out;
> +
> +       if (put_user((u64) prog, &uattr->prog_id))
> +               goto out;
> +
> +       if (attr->dump_insns) {
> +               u32 len = prog->len * sizeof(struct bpf_insn);
> +
> +               if (copy_to_user(u64_to_ptr(attr->dump_insns),
> +                                prog->insns, len) != 0)
> +                       goto out;
> +       }
> +
> +       ret = 0;
> +out:
> +       return ret;
> +}
> +
>  SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
>  {
>         union bpf_attr attr = {};
> @@ -650,6 +691,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
>         case BPF_PROG_LOAD:
>                 err = bpf_prog_load(&attr);
>                 break;
> +       case BPF_PROG_DUMP:
> +               err = bpf_prog_dump(&attr, uattr);
> +               break;
>         default:
>                 err = -EINVAL;
>                 break;
> --
> 2.1.4
>



-- 
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]

#1219310 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
> <tycho.andersen@canonical.com> wrote:
> > This commit adds a way to dump eBPF programs. The initial implementation
> > doesn't support maps, and therefore only allows dumping seccomp ebpf
> > programs which themselves don't currently support maps.
> >
> > We export the GPL bit as well as a unique ID for the program so that
> 
> This unique ID appears to be the heap address for the prog. That's a
> huge leak, and should not be done. We don't want to introduce new
> kernel address leaks while we're trying to fix the remaining ones.
> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
> could be used, for example.

No; we acquire the fd per process, so if a task installs a filter and
then forks N times, we'll grab N (+1) copies of the filter from N (+1)
different file descriptors. Ideally, we'd have some way to figure out
that these were all the same. Some sort of prog_id is one way,
although there may be others.

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]

#1219312 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 04, 2015 at 01:50:55PM -0700, Kees Cook wrote:
> On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
> <tycho.andersen@canonical.com> wrote:
> > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
> >> <tycho.andersen@canonical.com> wrote:
> >> > This commit adds a way to dump eBPF programs. The initial implementation
> >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
> >> > programs which themselves don't currently support maps.
> >> >
> >> > We export the GPL bit as well as a unique ID for the program so that
> >>
> >> This unique ID appears to be the heap address for the prog. That's a
> >> huge leak, and should not be done. We don't want to introduce new
> >> kernel address leaks while we're trying to fix the remaining ones.
> >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
> >> could be used, for example.
> >
> > No; we acquire the fd per process, so if a task installs a filter and
> > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
> > different file descriptors. Ideally, we'd have some way to figure out
> > that these were all the same. Some sort of prog_id is one way,
> > although there may be others.
> 
> If KCMP_FILE or a new KCMP_BPF isn't possible, then we'll probably
> have to add a unique id (counter) to all bpf programs as they're
> created.

I think tweaking KCMP_FILE for anon_inodes should do the trick
and should work at the end (if it's not working already).

--
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]

#1219318 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 04, 2015 at 01:58:25PM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 04, 2015 at 01:50:55PM -0700, Kees Cook wrote:
> > On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
> > <tycho.andersen@canonical.com> wrote:
> > > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
> > >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
> > >> <tycho.andersen@canonical.com> wrote:
> > >> > This commit adds a way to dump eBPF programs. The initial implementation
> > >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
> > >> > programs which themselves don't currently support maps.
> > >> >
> > >> > We export the GPL bit as well as a unique ID for the program so that
> > >>
> > >> This unique ID appears to be the heap address for the prog. That's a
> > >> huge leak, and should not be done. We don't want to introduce new
> > >> kernel address leaks while we're trying to fix the remaining ones.
> > >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
> > >> could be used, for example.
> > >
> > > No; we acquire the fd per process, so if a task installs a filter and
> > > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
> > > different file descriptors. Ideally, we'd have some way to figure out
> > > that these were all the same. Some sort of prog_id is one way,
> > > although there may be others.
> > 
> > If KCMP_FILE or a new KCMP_BPF isn't possible, then we'll probably
> > have to add a unique id (counter) to all bpf programs as they're
> > created.
> 
> I think tweaking KCMP_FILE for anon_inodes should do the trick
> and should work at the end (if it's not working already).

Sounds good. I'll look into that for the next version, thanks.

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]

#1219314 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
<tycho.andersen@canonical.com> wrote:
> On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
>> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
>> <tycho.andersen@canonical.com> wrote:
>> > This commit adds a way to dump eBPF programs. The initial implementation
>> > doesn't support maps, and therefore only allows dumping seccomp ebpf
>> > programs which themselves don't currently support maps.
>> >
>> > We export the GPL bit as well as a unique ID for the program so that
>>
>> This unique ID appears to be the heap address for the prog. That's a
>> huge leak, and should not be done. We don't want to introduce new
>> kernel address leaks while we're trying to fix the remaining ones.
>> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
>> could be used, for example.
>
> No; we acquire the fd per process, so if a task installs a filter and
> then forks N times, we'll grab N (+1) copies of the filter from N (+1)
> different file descriptors. Ideally, we'd have some way to figure out
> that these were all the same. Some sort of prog_id is one way,
> although there may be others.

If KCMP_FILE or a new KCMP_BPF isn't possible, then we'll probably
have to add a unique id (counter) to all bpf programs as they're
created.

-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]

#1219335 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
<tycho.andersen@canonical.com> wrote:
> On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
>> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
>> <tycho.andersen@canonical.com> wrote:
>> > This commit adds a way to dump eBPF programs. The initial implementation
>> > doesn't support maps, and therefore only allows dumping seccomp ebpf
>> > programs which themselves don't currently support maps.
>> >
>> > We export the GPL bit as well as a unique ID for the program so that
>>
>> This unique ID appears to be the heap address for the prog. That's a
>> huge leak, and should not be done. We don't want to introduce new
>> kernel address leaks while we're trying to fix the remaining ones.
>> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
>> could be used, for example.
>
> No; we acquire the fd per process, so if a task installs a filter and
> then forks N times, we'll grab N (+1) copies of the filter from N (+1)
> different file descriptors. Ideally, we'd have some way to figure out
> that these were all the same. Some sort of prog_id is one way,
> although there may be others.

I disagree a bit.  I think we want the actual hierarchy to be a
well-defined thing, because I have plans to make the hierarchy
actually do something.  That means that we'll need to have a more
exact way to dump the hierarchy than "these two filters are identical"
or "these two filters are not identical".

--Andy
--
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]

#1219348 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 04, 2015 at 02:48:03PM -0700, Andy Lutomirski wrote:
> On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
> <tycho.andersen@canonical.com> wrote:
> > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
> >> <tycho.andersen@canonical.com> wrote:
> >> > This commit adds a way to dump eBPF programs. The initial implementation
> >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
> >> > programs which themselves don't currently support maps.
> >> >
> >> > We export the GPL bit as well as a unique ID for the program so that
> >>
> >> This unique ID appears to be the heap address for the prog. That's a
> >> huge leak, and should not be done. We don't want to introduce new
> >> kernel address leaks while we're trying to fix the remaining ones.
> >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
> >> could be used, for example.
> >
> > No; we acquire the fd per process, so if a task installs a filter and
> > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
> > different file descriptors. Ideally, we'd have some way to figure out
> > that these were all the same. Some sort of prog_id is one way,
> > although there may be others.
> 
> I disagree a bit.  I think we want the actual hierarchy to be a
> well-defined thing, because I have plans to make the hierarchy
> actually do something.  That means that we'll need to have a more
> exact way to dump the hierarchy than "these two filters are identical"
> or "these two filters are not identical".

Can you elaborate on what this would look like? I think with the
"these two filters are the same" primitive (the same in the sense that
they were inherited during a fork, not just that
memcmp(filter1->insns, filter2->insns) == 0) you can infer the entire
hierarchy, however clunky it may be to do so.

Another issue is that KCMP_FILE won't work in this case, as it
effectively compares the struct file *, which will be different since
we need to call anon_inode_getfd() for each call of
ptrace(PTRACE_SECCOMP_GET_FILTER_FD). We could add a KCMP_BPF (or just
a KCMP_FILE_PRIVATE_DATA, since that's effectively what it would be).
Does that make sense? [added Cyrill]

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]

#1219368 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 4, 2015 at 3:28 PM, Tycho Andersen
<tycho.andersen@canonical.com> wrote:
> On Fri, Sep 04, 2015 at 02:48:03PM -0700, Andy Lutomirski wrote:
>> On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
>> <tycho.andersen@canonical.com> wrote:
>> > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
>> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
>> >> <tycho.andersen@canonical.com> wrote:
>> >> > This commit adds a way to dump eBPF programs. The initial implementation
>> >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
>> >> > programs which themselves don't currently support maps.
>> >> >
>> >> > We export the GPL bit as well as a unique ID for the program so that
>> >>
>> >> This unique ID appears to be the heap address for the prog. That's a
>> >> huge leak, and should not be done. We don't want to introduce new
>> >> kernel address leaks while we're trying to fix the remaining ones.
>> >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
>> >> could be used, for example.
>> >
>> > No; we acquire the fd per process, so if a task installs a filter and
>> > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
>> > different file descriptors. Ideally, we'd have some way to figure out
>> > that these were all the same. Some sort of prog_id is one way,
>> > although there may be others.
>>
>> I disagree a bit.  I think we want the actual hierarchy to be a
>> well-defined thing, because I have plans to make the hierarchy
>> actually do something.  That means that we'll need to have a more
>> exact way to dump the hierarchy than "these two filters are identical"
>> or "these two filters are not identical".
>
> Can you elaborate on what this would look like? I think with the
> "these two filters are the same" primitive (the same in the sense that
> they were inherited during a fork, not just that
> memcmp(filter1->insns, filter2->insns) == 0) you can infer the entire
> hierarchy, however clunky it may be to do so.
>
> Another issue is that KCMP_FILE won't work in this case, as it
> effectively compares the struct file *, which will be different since
> we need to call anon_inode_getfd() for each call of
> ptrace(PTRACE_SECCOMP_GET_FILTER_FD). We could add a KCMP_BPF (or just
> a KCMP_FILE_PRIVATE_DATA, since that's effectively what it would be).
> Does that make sense? [added Cyrill]
>

I don't really know what it would look like.  I think we want a way to
compare struct seccomp_filter pointers.

FWIW, I *hate* kcmp.  It might be worth trying to come up with a less
awful way to do this.  For example, what if we could generate a kcmpfd
such that each kcmpfd contains (internally) a random symmetric key?
We could have a function that would return kernel pointers encrypted
by that key.

Of course, then we need to make sure that no one ever tries to keep a
kcmpfd around long enough that CRIU needs to checkpoint it, because
that's impossible.

Grr.

--Andy
--
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]

#1219388 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 04, 2015 at 04:08:53PM -0700, Andy Lutomirski wrote:
> On Fri, Sep 4, 2015 at 3:28 PM, Tycho Andersen
> <tycho.andersen@canonical.com> wrote:
> > On Fri, Sep 04, 2015 at 02:48:03PM -0700, Andy Lutomirski wrote:
> >> On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
> >> <tycho.andersen@canonical.com> wrote:
> >> > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
> >> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
> >> >> <tycho.andersen@canonical.com> wrote:
> >> >> > This commit adds a way to dump eBPF programs. The initial implementation
> >> >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
> >> >> > programs which themselves don't currently support maps.
> >> >> >
> >> >> > We export the GPL bit as well as a unique ID for the program so that
> >> >>
> >> >> This unique ID appears to be the heap address for the prog. That's a
> >> >> huge leak, and should not be done. We don't want to introduce new
> >> >> kernel address leaks while we're trying to fix the remaining ones.
> >> >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
> >> >> could be used, for example.
> >> >
> >> > No; we acquire the fd per process, so if a task installs a filter and
> >> > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
> >> > different file descriptors. Ideally, we'd have some way to figure out
> >> > that these were all the same. Some sort of prog_id is one way,
> >> > although there may be others.
> >>
> >> I disagree a bit.  I think we want the actual hierarchy to be a
> >> well-defined thing, because I have plans to make the hierarchy
> >> actually do something.  That means that we'll need to have a more
> >> exact way to dump the hierarchy than "these two filters are identical"
> >> or "these two filters are not identical".
> >
> > Can you elaborate on what this would look like? I think with the
> > "these two filters are the same" primitive (the same in the sense that
> > they were inherited during a fork, not just that
> > memcmp(filter1->insns, filter2->insns) == 0) you can infer the entire
> > hierarchy, however clunky it may be to do so.
> >
> > Another issue is that KCMP_FILE won't work in this case, as it
> > effectively compares the struct file *, which will be different since
> > we need to call anon_inode_getfd() for each call of
> > ptrace(PTRACE_SECCOMP_GET_FILTER_FD). We could add a KCMP_BPF (or just
> > a KCMP_FILE_PRIVATE_DATA, since that's effectively what it would be).
> > Does that make sense? [added Cyrill]
> >
> 
> I don't really know what it would look like.  I think we want a way to
> compare struct seccomp_filter pointers.

Not to complicate things further, but this brings up another
interesting issue. Right now, we require PT_SUSPEND_SECCOMP in order
to restore seccomp and do things afterwards (otherwise the seccomp
filters might kill whatever things the restore process is doing). If
we want the struct seccomp_filter pointers to be identical on restore,
that means we need to restore when we are real root, because bpf()
requires that we be real root. This means that we essentially need to
ptrace the entire restore, which we don't want to do.

In order to work around this, I was thinking we could change the
ancestry check slightly:

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 9c6bea6..efc3f36 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -239,7 +239,7 @@ static int is_ancestor(struct seccomp_filter *parent,
 	if (parent == NULL)
 		return 1;
 	for (; child; child = child->prev)
-		if (child == parent)
+		if (child->prog == parent->prog)
 			return 1;
 	return 0;
 }

so that we can do bpf() when we're real root, and just restore seccomp
at the very end. This would mean that the struct bpf_prog pointers are
shared, but the struct seccomp_filter pointers aren't.

Even assuming we have some sort of way to identify shared ancestry, we
still need something like the above in order to be able to restore it.
This sort of sucks; it would be ideal to to share struct
seccomp_filter *s too. We could do something like
seccomp(COPY_FROM_PARENT) or something, but given the struggles Kees
told me he had with getting SECCOMP_FILTER_FLAG_TSYNC right, I suspect
that won't fly.

> FWIW, I *hate* kcmp.  It might be worth trying to come up with a less
> awful way to do this.  For example, what if we could generate a kcmpfd
> such that each kcmpfd contains (internally) a random symmetric key?
> We could have a function that would return kernel pointers encrypted
> by that key.

We could do what Kees is proposing for struct bpf_prog and just keep a
globally unique id on struct seccomp_filter, and allow asking for that
via some seccomp(GET_ID, fd) over the same fd we're using to dump the
bpf prog. That doesn't solve our restore problem, though.

Tycho

> Of course, then we need to make sure that no one ever tries to keep a
> kcmpfd around long enough that CRIU needs to checkpoint it, because
> that's impossible.
> 
> Grr.
> 
> --Andy
--
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]

#1219376 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 4, 2015 at 3:28 PM, Tycho Andersen
<tycho.andersen@canonical.com> wrote:
> On Fri, Sep 04, 2015 at 02:48:03PM -0700, Andy Lutomirski wrote:
>> On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
>> <tycho.andersen@canonical.com> wrote:
>> > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
>> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
>> >> <tycho.andersen@canonical.com> wrote:
>> >> > This commit adds a way to dump eBPF programs. The initial implementation
>> >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
>> >> > programs which themselves don't currently support maps.
>> >> >
>> >> > We export the GPL bit as well as a unique ID for the program so that
>> >>
>> >> This unique ID appears to be the heap address for the prog. That's a
>> >> huge leak, and should not be done. We don't want to introduce new
>> >> kernel address leaks while we're trying to fix the remaining ones.
>> >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
>> >> could be used, for example.
>> >
>> > No; we acquire the fd per process, so if a task installs a filter and
>> > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
>> > different file descriptors. Ideally, we'd have some way to figure out
>> > that these were all the same. Some sort of prog_id is one way,
>> > although there may be others.
>>
>> I disagree a bit.  I think we want the actual hierarchy to be a
>> well-defined thing, because I have plans to make the hierarchy
>> actually do something.  That means that we'll need to have a more
>> exact way to dump the hierarchy than "these two filters are identical"
>> or "these two filters are not identical".
>
> Can you elaborate on what this would look like? I think with the
> "these two filters are the same" primitive (the same in the sense that
> they were inherited during a fork, not just that
> memcmp(filter1->insns, filter2->insns) == 0) you can infer the entire
> hierarchy, however clunky it may be to do so.
>
> Another issue is that KCMP_FILE won't work in this case, as it
> effectively compares the struct file *, which will be different since
> we need to call anon_inode_getfd() for each call of
> ptrace(PTRACE_SECCOMP_GET_FILTER_FD). We could add a KCMP_BPF (or just
> a KCMP_FILE_PRIVATE_DATA, since that's effectively what it would be).
> Does that make sense? [added Cyrill]

If KCMP_FILE_PRIVATE_DATA isn't desired, I think a global counter id
is the next best.

-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]

#1219386 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 4, 2015 at 4:27 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Sep 4, 2015 at 3:28 PM, Tycho Andersen
> <tycho.andersen@canonical.com> wrote:
>> On Fri, Sep 04, 2015 at 02:48:03PM -0700, Andy Lutomirski wrote:
>>> On Fri, Sep 4, 2015 at 1:45 PM, Tycho Andersen
>>> <tycho.andersen@canonical.com> wrote:
>>> > On Fri, Sep 04, 2015 at 01:17:30PM -0700, Kees Cook wrote:
>>> >> On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
>>> >> <tycho.andersen@canonical.com> wrote:
>>> >> > This commit adds a way to dump eBPF programs. The initial implementation
>>> >> > doesn't support maps, and therefore only allows dumping seccomp ebpf
>>> >> > programs which themselves don't currently support maps.
>>> >> >
>>> >> > We export the GPL bit as well as a unique ID for the program so that
>>> >>
>>> >> This unique ID appears to be the heap address for the prog. That's a
>>> >> huge leak, and should not be done. We don't want to introduce new
>>> >> kernel address leaks while we're trying to fix the remaining ones.
>>> >> Shouldn't the "unique ID" be the fd itself? I imagine KCMP_FILE
>>> >> could be used, for example.
>>> >
>>> > No; we acquire the fd per process, so if a task installs a filter and
>>> > then forks N times, we'll grab N (+1) copies of the filter from N (+1)
>>> > different file descriptors. Ideally, we'd have some way to figure out
>>> > that these were all the same. Some sort of prog_id is one way,
>>> > although there may be others.
>>>
>>> I disagree a bit.  I think we want the actual hierarchy to be a
>>> well-defined thing, because I have plans to make the hierarchy
>>> actually do something.  That means that we'll need to have a more
>>> exact way to dump the hierarchy than "these two filters are identical"
>>> or "these two filters are not identical".
>>
>> Can you elaborate on what this would look like? I think with the
>> "these two filters are the same" primitive (the same in the sense that
>> they were inherited during a fork, not just that
>> memcmp(filter1->insns, filter2->insns) == 0) you can infer the entire
>> hierarchy, however clunky it may be to do so.
>>
>> Another issue is that KCMP_FILE won't work in this case, as it
>> effectively compares the struct file *, which will be different since
>> we need to call anon_inode_getfd() for each call of
>> ptrace(PTRACE_SECCOMP_GET_FILTER_FD). We could add a KCMP_BPF (or just
>> a KCMP_FILE_PRIVATE_DATA, since that's effectively what it would be).
>> Does that make sense? [added Cyrill]
>
> If KCMP_FILE_PRIVATE_DATA isn't desired, I think a global counter id
> is the next best.

The problem is that you can't checkpoint and restore it.  We could
have a counter relative to the parent filter, though.

--Andy

>
> -Kees
>
> --
> Kees Cook
> Chrome OS Security



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
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]

#1219291 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

On Fri, Sep 04, 2015 at 10:04:21AM -0600, Tycho Andersen wrote:
> This commit adds a way to dump eBPF programs. The initial implementation
> doesn't support maps, and therefore only allows dumping seccomp ebpf
> programs which themselves don't currently support maps.
> 
> 
> Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Will Drewry <wad@chromium.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> CC: Alexei Starovoitov <ast@kernel.org>
> CC: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  include/uapi/linux/bpf.h | 15 +++++++++++++++
>  kernel/bpf/syscall.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 79b825a..c5d8dc2 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -107,6 +107,13 @@ enum bpf_cmd {
>  	 * returns fd or negative error
>  	 */
>  	BPF_PROG_LOAD,
> +
> +	/* dump an existing bpf
> +	 * err = bpf(BPF_PROG_DUMP, union bpf_attr *attr, u32 size)
> +	 * Using attr->prog_fd, attr->dump_insn_cnt, attr->dump_insns
> +	 * returns zero or negative error
> +	 */
> +	BPF_PROG_DUMP,
>  };
>  
>  enum bpf_map_type {
> @@ -160,6 +167,14 @@ union bpf_attr {
>  		__aligned_u64	log_buf;	/* user supplied buffer */
>  		__u32		kern_version;	/* checked when prog_type=kprobe */
>  	};
> +
> +	struct { /* anonymous struct used by BPF_PROG_DUMP command */
> +		__u32		prog_fd;
> +		__u32		dump_insn_cnt;
> +		__aligned_u64	dump_insns;	/* user supplied buffer */
> +		__u8		gpl_compatible;
> +		__u64		prog_id;	/* unique id for this prog */
> +	};

my first reaction was to may be reuse existing struct used to load,
but I guess it's actually cleaner to have a new one like you did.
though prog_fd looks redundant and prog_id is ...

> +	if (put_user((u64) prog, &uattr->prog_id))
> +		goto out;

.. is definitely not secure.

> We export the GPL bit as well as a unique ID for the program so that
> userspace can detect when two seccomp filters were inherited from each
> other and clone the filter tree accordingly.

you mean that in-kernel prog pointer is the same?
I think user space can memcmp insns of programs instead?
Are you trying to solve the case when parent has an FD for bpf program
and child has another FD that points to the same program, and both
doing dump and need to coordinate?

--
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]

#1219306 — Re: [PATCH 3/6] ebpf: add a way to dump an eBPF program

Hi Alexei,

On Fri, Sep 04, 2015 at 01:27:05PM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 04, 2015 at 10:04:21AM -0600, Tycho Andersen wrote:
> > This commit adds a way to dump eBPF programs. The initial implementation
> > doesn't support maps, and therefore only allows dumping seccomp ebpf
> > programs which themselves don't currently support maps.
> > 
> > 
> > Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Will Drewry <wad@chromium.org>
> > CC: Oleg Nesterov <oleg@redhat.com>
> > CC: Andy Lutomirski <luto@amacapital.net>
> > CC: Pavel Emelyanov <xemul@parallels.com>
> > CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> > CC: Alexei Starovoitov <ast@kernel.org>
> > CC: Daniel Borkmann <daniel@iogearbox.net>
> > ---
> >  include/uapi/linux/bpf.h | 15 +++++++++++++++
> >  kernel/bpf/syscall.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 59 insertions(+)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 79b825a..c5d8dc2 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -107,6 +107,13 @@ enum bpf_cmd {
> >  	 * returns fd or negative error
> >  	 */
> >  	BPF_PROG_LOAD,
> > +
> > +	/* dump an existing bpf
> > +	 * err = bpf(BPF_PROG_DUMP, union bpf_attr *attr, u32 size)
> > +	 * Using attr->prog_fd, attr->dump_insn_cnt, attr->dump_insns
> > +	 * returns zero or negative error
> > +	 */
> > +	BPF_PROG_DUMP,
> >  };
> >  
> >  enum bpf_map_type {
> > @@ -160,6 +167,14 @@ union bpf_attr {
> >  		__aligned_u64	log_buf;	/* user supplied buffer */
> >  		__u32		kern_version;	/* checked when prog_type=kprobe */
> >  	};
> > +
> > +	struct { /* anonymous struct used by BPF_PROG_DUMP command */
> > +		__u32		prog_fd;
> > +		__u32		dump_insn_cnt;
> > +		__aligned_u64	dump_insns;	/* user supplied buffer */
> > +		__u8		gpl_compatible;
> > +		__u64		prog_id;	/* unique id for this prog */
> > +	};
> 
> my first reaction was to may be reuse existing struct used to load,
> but I guess it's actually cleaner to have a new one like you did.
> though prog_fd looks redundant and prog_id is ...

prog_fd is input here, the rest are outputs.

> > +	if (put_user((u64) prog, &uattr->prog_id))
> > +		goto out;
> 
> .. is definitely not secure.
> 
> > We export the GPL bit as well as a unique ID for the program so that
> > userspace can detect when two seccomp filters were inherited from each
> > other and clone the filter tree accordingly.
> 
> you mean that in-kernel prog pointer is the same?
> I think user space can memcmp insns of programs instead?
> Are you trying to solve the case when parent has an FD for bpf program
> and child has another FD that points to the same program, and both
> doing dump and need to coordinate?

Yes, exactly. If we just do a memcmp(), two users can install the same
filter and have a different inheritance model on checkpoint vs
restore. This means that a checkpoint/restore'd process may see
different behavior when using SECCOMP_FILTER_FLAG_TSYNC in the future.

I'm not entirely clear on how much of a problem this actually is, and
perhaps it is too small to be worth worry about, but if there was
another way to export some unique id, that would be dandy.

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]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web