netdev - Re: [PATCH bpf-next 1/2] bpf: Allow ld_imm64 instruction to point to kfunc.

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:   Thu, 16 Mar 2023 16:14:11 +0200
From:   Eduard Zingerman <eddyz87@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        davem@...emloft.net
Cc:     daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org,
        void@...ifault.com, davemarchevsky@...a.com, tj@...nel.org,
        memxor@...il.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
        kernel-team@...com
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow ld_imm64 instruction to point
 to kfunc.

On Wed, 2023-03-15 at 15:36 -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@...nel.org>
> 
> Allow ld_imm64 insn with BPF_PSEUDO_BTF_ID to hold the address of kfunc.
> PTR_MEM is already recognized for NULL-ness by is_branch_taken(),
> so unresolved kfuncs will be seen as zero.
> This allows BPF programs to detect at load time whether kfunc is present
> in the kernel with bpf_kfunc_exists() macro.
> 
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---
>  kernel/bpf/verifier.c       | 7 +++++--
>  tools/lib/bpf/bpf_helpers.h | 3 +++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 60793f793ca6..4e49d34d8cd6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15955,8 +15955,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>  		goto err_put;
>  	}
>  
> -	if (!btf_type_is_var(t)) {
> -		verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR.\n", id);
> +	if (!btf_type_is_var(t) && !btf_type_is_func(t)) {
> +		verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR or KIND_FUNC\n", id);
>  		err = -EINVAL;
>  		goto err_put;
>  	}
> @@ -15990,6 +15990,9 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>  		aux->btf_var.reg_type = PTR_TO_BTF_ID | MEM_PERCPU;
>  		aux->btf_var.btf = btf;
>  		aux->btf_var.btf_id = type;
> +	} else if (!btf_type_is_func(t)) {
> +		aux->btf_var.reg_type = PTR_TO_MEM | MEM_RDONLY;
> +		aux->btf_var.mem_size = 0;

This if statement has the following conditions in master:

	if (percpu) {
    	// ...
	} else if (!btf_type_is_struct(t)) {
    	// ...
	} else {
    	// ...
	}

Conditions `!btf_type_is_func()` and `!btf_type_is_struct()` are
not mutually exclusive, thus adding `if (!btf_type_is_func())`
would match certain conditions that were previously matched by struct
case, wouldn't it? E.g. if type is `BTF_KIND_INT`?

Although, I was not able to trigger it, as it seems that pahole only
encodes per-cpu vars in BTF.

>  	} else if (!btf_type_is_struct(t)) {
>  		const struct btf_type *ret;
>  		const char *tname;
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 7d12d3e620cc..43abe4c29409 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -177,6 +177,9 @@ enum libbpf_tristate {
>  #define __kptr_untrusted __attribute__((btf_type_tag("kptr_untrusted")))
>  #define __kptr __attribute__((btf_type_tag("kptr")))
>  
> +/* pass function pointer through asm otherwise compiler assumes that any function != 0 */
> +#define bpf_kfunc_exists(fn) ({ void *__p = fn; asm ("" : "+r"(__p)); __p; })
> +
>  #ifndef ___bpf_concat
>  #define ___bpf_concat(a, b) a ## b
>  #endif