netdev - Re: [PATCH bpf-next] libbpf: prevent overwriting of log_level in bpf_object__load

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]

Message-ID: <1f3cfd61-f8af-f67e-aa2e-c0286df72820@iogearbox.net>
Date:   Wed, 29 May 2019 16:03:36 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Quentin Monnet <quentin.monnet@...ronome.com>,
        Alexei Starovoitov <ast@...nel.org>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next] libbpf: prevent overwriting of log_level in
 bpf_object__load_progs()

On 05/29/2019 11:23 AM, Quentin Monnet wrote:
> There are two functions in libbpf that support passing a log_level
> parameter for the verifier for loading programs:
> bpf_object__load_xattr() and bpf_load_program_xattr(). Both accept an
> attribute object containing the log_level, and apply it to the programs
> to load.
> 
> It turns out that to effectively load the programs, the latter function
> eventually relies on the former. This was not taken into account when
> adding support for log_level in bpf_object__load_xattr(), and the
> log_level passed to bpf_load_program_xattr() later gets overwritten with
> a zero value, thus disabling verifier logs for the program in all cases:
> 
> bpf_load_program_xattr()             // prog.log_level = N;

I'm confused with your commit message. How can bpf_load_program_xattr()
make sense here, this is the one doing the bpf syscall. Do you mean to
say bpf_prog_load_xattr()? Because this one sets prog->log_level = attr->log_level
and calls bpf_object__load() which in turn does bpf_object__load_xattr()
with an attr that has attr->log_level of 0 such that bpf_object__load_progs()
then overrides it. Unless I'm not missing something, please fix up this
description properly and resubmit.

>  -> bpf_object__load()               // attr.log_level = 0;
>      -> bpf_object__load_xattr()     // <pass prog and attr>
>          -> bpf_object__load_progs() // prog.log_level = attr.log_level;
> 
> Fix this by OR-ing the log_level in bpf_object__load_progs(), instead of
> overwriting it.
> 
> Fixes: 60276f984998 ("libbpf: add bpf_object__load_xattr() API function to pass log_level")
> Reported-by: Alexei Starovoitov <ast@...nel.org>
> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
> ---
>  tools/lib/bpf/libbpf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ca4432f5b067..30cb08e2eb75 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2232,7 +2232,7 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
>  	for (i = 0; i < obj->nr_programs; i++) {
>  		if (bpf_program__is_function_storage(&obj->programs[i], obj))
>  			continue;
> -		obj->programs[i].log_level = log_level;
> +		obj->programs[i].log_level |= log_level;
>  		err = bpf_program__load(&obj->programs[i],
>  					obj->license,
>  					obj->kern_version);
>