netdev - Re: [PATCH bpf-next] libbpf: support CO-RE relocations for LD/LDX/ST/STX instructions

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:   Mon, 23 Dec 2019 07:48:21 +0000
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andriin@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>
CC:     "andrii.nakryiko@...il.com" <andrii.nakryiko@...il.com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next] libbpf: support CO-RE relocations for
 LD/LDX/ST/STX instructions



On 12/22/19 10:18 PM, Andrii Nakryiko wrote:
> Clang patch [0] enables emitting relocatable generic ALU/ALU64 instructions
> (i.e, shifts and arithmetic operations), as well as generic load/store
> instructions. The former ones are already supported by libbpf as is. This
> patch adds further support for load/store instructions. Relocatable field
> offset is encoded in BPF instruction's 16-bit offset section and are adjusted
> by libbpf based on target kernel BTF.
> 
> These Clang changes and corresponding libbpf changes allow for more succinct
> generated BPF code by encoding relocatable field reads as a single
> LD/ST/LDX/STX instruction. It also enables relocatable access to BPF context.
> Previously, if context struct (e.g., __sk_buff) was accessed with CO-RE
> relocations (e.g., due to preserve_access_index attribute), it would be
> rejected by BPF verifier due to modified context pointer dereference. With
> Clang patch, such context accesses are both relocatable and have a fixed
> offset from the point of view of BPF verifier.
> 
>    [0] https://reviews.llvm.org/D71790
> 
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---
>   tools/lib/bpf/libbpf.c | 32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 9576a90c5a1c..2dbc2204a02c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -18,6 +18,7 @@
>   #include <stdarg.h>
>   #include <libgen.h>
>   #include <inttypes.h>
> +#include <limits.h>
>   #include <string.h>
>   #include <unistd.h>
>   #include <endian.h>
> @@ -3810,11 +3811,13 @@ static int bpf_core_reloc_insn(struct bpf_program *prog,
>   	insn = &prog->insns[insn_idx];
>   	class = BPF_CLASS(insn->code);
>   
> -	if (class == BPF_ALU || class == BPF_ALU64) {
> +	switch (class) {
> +	case BPF_ALU:
> +	case BPF_ALU64:
>   		if (BPF_SRC(insn->code) != BPF_K)
>   			return -EINVAL;
>   		if (!failed && validate && insn->imm != orig_val) {
> -			pr_warn("prog '%s': unexpected insn #%d value: got %u, exp %u -> %u\n",
> +			pr_warn("prog '%s': unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n",
>   				bpf_program__title(prog, false), insn_idx,
>   				insn->imm, orig_val, new_val);
>   			return -EINVAL;
> @@ -3824,7 +3827,30 @@ static int bpf_core_reloc_insn(struct bpf_program *prog,
>   		pr_debug("prog '%s': patched insn #%d (ALU/ALU64)%s imm %u -> %u\n",
>   			 bpf_program__title(prog, false), insn_idx,
>   			 failed ? " w/ failed reloc" : "", orig_val, new_val);
> -	} else {
> +		break;
> +	case BPF_LD:

Maybe we should remove BPF_LD here? BPF_LD is used for ld_imm64, ld_abs 
and ld_ind, where the insn->off = 0 and not really used.

> +	case BPF_LDX:
> +	case BPF_ST:
> +	case BPF_STX: > +		if (!failed && validate && insn->off != orig_val) {
> +			pr_warn("prog '%s': unexpected insn #%d (LD/LDX/ST/STX) value: got %u, exp %u -> %u\n",
> +				bpf_program__title(prog, false), insn_idx,
> +				insn->off, orig_val, new_val);
> +			return -EINVAL;
> +		}
> +		if (new_val > SHRT_MAX) {
> +			pr_warn("prog '%s': insn #%d (LD/LDX/ST/STX) value too big: %u\n",
> +				bpf_program__title(prog, false), insn_idx,
> +				new_val);
> +			return -ERANGE;
> +		}
> +		orig_val = insn->off;
> +		insn->off = new_val;
> +		pr_debug("prog '%s': patched insn #%d (LD/LDX/ST/STX)%s off %u -> %u\n",
> +			 bpf_program__title(prog, false), insn_idx,
> +			 failed ? " w/ failed reloc" : "", orig_val, new_val);
> +		break;
> +	default:
>   		pr_warn("prog '%s': trying to relocate unrecognized insn #%d, code:%x, src:%x, dst:%x, off:%x, imm:%x\n",
>   			bpf_program__title(prog, false),
>   			insn_idx, insn->code, insn->src_reg, insn->dst_reg,
>