netdev - Re: [RFC PATCH bpf-next 04/16] bpf: allow loading of a dumper program

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 Apr 2020 15:36:09 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH bpf-next 04/16] bpf: allow loading of a dumper program

On Wed, Apr 8, 2020 at 4:25 PM Yonghong Song <yhs@...com> wrote:
>
> A dumper bpf program is a tracing program with attach type
> BPF_TRACE_DUMP. During bpf program load, the load attribute
>    attach_prog_fd
> carries the target directory fd. The program will be
> verified against btf_id of the target_proto.
>
> If the program is loaded successfully, the dump target, as
> represented as a relative path to /sys/kernel/bpfdump,
> will be remembered in prog->aux->dump_target, which will
> be used later to create dumpers.
>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---
>  include/linux/bpf.h            |  2 ++
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/dump.c              | 40 ++++++++++++++++++++++++++++++++++
>  kernel/bpf/syscall.c           |  8 ++++++-
>  kernel/bpf/verifier.c          | 15 +++++++++++++
>  tools/include/uapi/linux/bpf.h |  1 +
>  6 files changed, 66 insertions(+), 1 deletion(-)
>

[...]

>
> +int bpf_dump_set_target_info(u32 target_fd, struct bpf_prog *prog)
> +{
> +       struct bpfdump_target_info *tinfo;
> +       const char *target_proto;
> +       struct file *target_file;
> +       struct fd tfd;
> +       int err = 0, btf_id;
> +
> +       if (!btf_vmlinux)
> +               return -EINVAL;
> +
> +       tfd = fdget(target_fd);
> +       target_file = tfd.file;
> +       if (!target_file)
> +               return -EBADF;

fdput is missing (or rather err = -BADF; goto done; ?)


> +
> +       if (target_file->f_inode->i_op != &bpf_dir_iops) {
> +               err = -EINVAL;
> +               goto done;
> +       }
> +
> +       tinfo = target_file->f_inode->i_private;
> +       target_proto = tinfo->target_proto;
> +       btf_id = btf_find_by_name_kind(btf_vmlinux, target_proto,
> +                                      BTF_KIND_FUNC);
> +
> +       if (btf_id > 0) {
> +               prog->aux->dump_target = tinfo->target;
> +               prog->aux->attach_btf_id = btf_id;
> +       }
> +
> +       err = min(btf_id, 0);

this min trick looks too clever... why not more straightforward and composable:

if (btf_id < 0) {
    err = btf_id;
    goto done;
}

prog->aux->dump_target = tinfo->target;
prog->aux->attach_btf_id = btf_id;

?

> +done:
> +       fdput(tfd);
> +       return err;
> +}
> +
>  int bpf_dump_reg_target(const char *target,
>                         const char *target_proto,
>                         const struct seq_operations *seq_ops,
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 64783da34202..41005dee8957 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2060,7 +2060,12 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
>
>         prog->expected_attach_type = attr->expected_attach_type;
>         prog->aux->attach_btf_id = attr->attach_btf_id;
> -       if (attr->attach_prog_fd) {
> +       if (type == BPF_PROG_TYPE_TRACING &&
> +           attr->expected_attach_type == BPF_TRACE_DUMP) {
> +               err = bpf_dump_set_target_info(attr->attach_prog_fd, prog);

looking at bpf_attr, it's not clear why attach_prog_fd and
prog_ifindex were not combined into a single union field... this
probably got missed? But in this case I'd say let's create a

union {
    __u32 attach_prog_fd;
    __u32 attach_target_fd; (similar to terminology for BPF_PROG_ATTACH)
};

instead of reusing not-exactly-matching field names?

> +               if (err)
> +                       goto free_prog_nouncharge;
> +       } else if (attr->attach_prog_fd) {
>                 struct bpf_prog *tgt_prog;
>
>                 tgt_prog = bpf_prog_get(attr->attach_prog_fd);
> @@ -2145,6 +2150,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
>         err = bpf_prog_new_fd(prog);
>         if (err < 0)
>                 bpf_prog_put(prog);
> +
>         return err;
>

[...]