netdev - Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog

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] [day] [month] [year] [list]

Date:   Wed, 12 Dec 2018 10:31:26 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Martin Lau <kafai@...com>
Cc:     Song Liu <songliubraving@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in
 bpf_prog_info

On 12/12/2018 05:38 AM, Martin Lau wrote:
> On Wed, Dec 12, 2018 at 03:22:38AM +0100, Daniel Borkmann wrote:
>> On 12/11/2018 09:18 AM, Song Liu wrote:
>>> Changes from v1:
>>> 1. Fix error path as Martin suggested.
>>>
>>> This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
>>> reliable way for user space to get tags of all sub programs. Before this
>>> patch, user space need to find sub program tags via kallsyms.
>>>
>>> This feature will be used in BPF introspection, where user space queries
>>> information about BPF programs via sys_bpf.
>>>
>>> Signed-off-by: Song Liu <songliubraving@...com>
>>> ---
>>>  include/uapi/linux/bpf.h |  2 ++
>>>  kernel/bpf/syscall.c     | 27 +++++++++++++++++++++++++++
>>>  2 files changed, 29 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 1bee1135866a..368d185aa32f 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2703,6 +2703,8 @@ struct bpf_prog_info {
>>>  	__u32 jited_line_info_cnt;
>>>  	__u32 line_info_rec_size;
>>>  	__u32 jited_line_info_rec_size;
>>> +	__u32 nr_prog_tags;
>>> +	__aligned_u64 prog_tags;
>>>  } __attribute__((aligned(8)));
>>>
>>>  struct bpf_map_info {
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 19c88cff7880..49cb59177db9 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -2322,6 +2322,33 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
>>>  		}
>>>  	}
>>>
>>> +	ulen = info.nr_prog_tags;
>>> +	info.nr_prog_tags = prog->aux->func_cnt ? : 1;
>>> +	if (ulen) {
>>> +		if (bpf_dump_raw_ok()) {
>>
>> Hm, why is bpf_dump_raw_ok() needed here? tag is not exposing a kernel
>> address. prog tag is in general also visible from fdinfo from unpriv.
>>
>> Just looking at the recently merged func_info / line_info I'm not sure
>> this is needed there either ... Martin, is there a specific reason this
>> was added there as well?
> It is mostly to follow info.jited_func_lens which also tests
> bpf_dump_raw_ok().  We thought the check iss there even it is
> not exposing the kernel address because jited_func_lens is not
> useful in general without jited_ksyms.

Yes, agree, makes sense to me there.

> Same go for func_info when dumping jited insn.  func_info used to only
> support jited insn dump.

Ok, sounds reasonable for the JIT case. (In the xlated one we just zero
the addresses through bpf_insn_prepare_dump() so the dump is available
also on !bpf_dump_raw_ok().)

> Considering func_info was later made to support xlated insn also,
> I think it makes sense to remove the bpf_dump_raw_ok() check
> for func_info, line_info and prog_tags and ask the

Yeah lets remove it from there.

> userspace to decide if it has all needed details before dumping
> info for the xlated insn and jited insn?

E.g. in case of !bpf_dump_raw_ok(), user space should still be able to
do the C / line-info annotation for the xlated insns. If it finds that
for JIT it cannot dump anything, we could print an error to the user in
bpftool telling that if the information is wanted nevertheless then the
kptr_restrict setting needs to be adapted.

I think the sub-prog_tags and func_info could still be useful e.g. in
bpftool case for dumping all kernel symbols related to a specific
prog-id, for example, or if we add a new mode in bpftool to dump all
tags from a user provided object file, so they can be correlated with
the one active in the kernel.

> The bpf_dump_raw_ok() check for jited_line_info will stay
> because jited_line_info contains kernel address.

+1

Thanks,
Daniel