netdev - Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

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: <20200124072054.2kr25erckbclkwgv@kafai-mbp.dhcp.thefacebook.com>
Date:   Fri, 24 Jan 2020 07:20:59 +0000
From:   Martin Lau <kafai@...com>
To:     Andrii Nakryiko <andriin@...com>
CC:     "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>,
        "andrii.nakryiko@...il.com" <andrii.nakryiko@...il.com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of
 failed CO-RE relocations

On Thu, Jan 23, 2020 at 09:38:37PM -0800, Andrii Nakryiko wrote:
> Previously, if libbpf failed to resolve CO-RE relocation for some
> instructions, it would either return error immediately, or, if
> .relaxed_core_relocs option was set, would replace relocatable offset/imm part
> of an instruction with a bogus value (-1). Neither approach is good, because
> there are many possible scenarios where relocation is expected to fail (e.g.,
> when some field knowingly can be missing on specific kernel versions). On the
> other hand, replacing offset with invalid one can hide programmer errors, if
> this relocation failue wasn't anticipated.
> 
> This patch deprecates .relaxed_core_relocs option and changes the approach to
> always replacing instruction, for which relocation failed, with invalid BPF
> helper call instruction. For cases where this is expected, BPF program should
> already ensure that that instruction is unreachable, in which case this
> invalid instruction is going to be silently ignored. But if instruction wasn't
> guarded, BPF program will be rejected at verification step with verifier log
> pointing precisely to the place in assembly where the problem is.
> 
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---
>  tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
>  tools/lib/bpf/libbpf.h |  6 ++-
>  2 files changed, 61 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ae34b681ae82..39f1b7633a7c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -345,7 +345,6 @@ struct bpf_object {
>  
>  	bool loaded;
>  	bool has_pseudo_calls;
> -	bool relaxed_core_relocs;
>  
>  	/*
>  	 * Information when doing elf related work. Only valid if fd
> @@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
>   */
>  static int bpf_core_reloc_insn(struct bpf_program *prog,
>  			       const struct bpf_field_reloc *relo,
> +			       int relo_idx,
>  			       const struct bpf_core_spec *local_spec,
>  			       const struct bpf_core_spec *targ_spec)
>  {
> -	bool failed = false, validate = true;
>  	__u32 orig_val, new_val;
>  	struct bpf_insn *insn;
> +	bool validate = true;
>  	int insn_idx, err;
>  	__u8 class;
>  
>  	if (relo->insn_off % sizeof(struct bpf_insn))
>  		return -EINVAL;
>  	insn_idx = relo->insn_off / sizeof(struct bpf_insn);
> +	insn = &prog->insns[insn_idx];
> +	class = BPF_CLASS(insn->code);
>  
>  	if (relo->kind == BPF_FIELD_EXISTS) {
>  		orig_val = 1; /* can't generate EXISTS relo w/o local field */
>  		new_val = targ_spec ? 1 : 0;
>  	} else if (!targ_spec) {
> -		failed = true;
> -		new_val = (__u32)-1;
> +		pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
> +			 bpf_program__title(prog, false), relo_idx, insn_idx);
> +		insn->code = BPF_JMP | BPF_CALL;
> +		insn->dst_reg = 0;
> +		insn->src_reg = 0;
> +		insn->off = 0;
> +		/* if this instruction is reachable (not a dead code),
> +		 * verifier will complain with the following message:
> +		 * invalid func unknown#195896080
> +		 */
> +		insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
Should this value become a binded contract in uapi/bpf.h so
that the verifier can print a more meaningful name than "unknown#195896080"?

> +		return 0;
>  	} else {
>  		err = bpf_core_calc_field_relo(prog, relo, local_spec,
>  					       &orig_val, &validate);