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 16:28:30 -0700
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.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 4/10/20 3:36 PM, Andrii Nakryiko wrote:
> 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; ?)

No need to do fdput if tfd.file is NULL.

> 
> 
>> +
>> +       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;
> 
> ?

this can be done.

> 
>> +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?

I thought about this, but thinking to avoid uapi change (although 
compatible). Maybe we should. Let me think about this.

> 
>> +               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;
>>
> 
> [...]
>