Re: [PATCH bpf-next 3/9] libbpf: improve relocation ambiguity detection

[Menglong Dong <menglong.dong@linux.dev>]

On 2020/8/19 06:39 Andrii Nakryiko <andriin@fb.com> write:
> Split the instruction patching logic into relocation value calculation and
> application of relocation to instruction. Using this, evaluate relocation
> against each matching candidate and validate that all candidates agree on
> relocated value. If not, report ambiguity and fail load.
> 
> This logic is necessary to avoid dangerous (however unlikely) accidental match
> against two incompatible candidate types. Without this change, libbpf will
> pick a random type as *the* candidate and apply potentially invalid
> relocation.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 170 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 124 insertions(+), 46 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2047e4ed0076..1ba458140f50 100644
> --- a/tools/lib/bpf/libbpf.c
[......]
> @@ -5005,16 +5063,31 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
>  		if (err == 0)
>  			continue;
>  
> +		err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, &cand_spec, &cand_res);
> +		if (err)
> +			return err;
> +
>  		if (j == 0) {
> +			targ_res = cand_res;
>  			targ_spec = cand_spec;
>  		} else if (cand_spec.bit_offset != targ_spec.bit_offset) {
> -			/* if there are many candidates, they should all
> -			 * resolve to the same bit offset
> +			/* if there are many field relo candidates, they
> +			 * should all resolve to the same bit offset
>  			 */
> -			pr_warn("prog '%s': relo #%d: offset ambiguity: %u != %u\n",
> +			pr_warn("prog '%s': relo #%d: field offset ambiguity: %u != %u\n",
>  				prog_name, relo_idx, cand_spec.bit_offset,
>  				targ_spec.bit_offset);
>  			return -EINVAL;
> +		} else if (cand_res.poison != targ_res.poison || cand_res.new_val != targ_res.new_val) {
> +			/* all candidates should result in the same relocation
> +			 * decision and value, otherwise it's dangerous to
> +			 * proceed due to ambiguity
> +			 */
> +			pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %u != %s %u\n",
> +				prog_name, relo_idx,
> +				cand_res.poison ? "failure" : "success", cand_res.new_val,
> +				targ_res.poison ? "failure" : "success", targ_res.new_val);
> +			return -EINVAL;
>  		}

Hi, Andrii. This approach is not friend to bpf_core_cast() if the struct
is not used in the vmlinux, but the kernel modules.

Take "struct nft_chain" for example. Following code will fail:
    struct nft_chain *chain = bpf_core_cast(ptr, struct nft_chain).

The bpf_core_cast() will record a BPF_CORE_TYPE_ID_TARGET relocation
for "struct nft_chain". The libbpf will find multi btf type of nft_chain
in the modules nf_tables, nft_reject, etc, and it will fail the verification
due to the "new_val", which is btf type id, not the same, even if all
the "struct nft_chain" are exactly the same in different kernel modules.

I think this is a common case. So how about we check the consistence of
struct nft_chain in all the candidate list, and use the first one if all of
them have exactly the same definition?

We can check all the members in the struct iteratively, and make
sure they are all the same.

Thanks!
Menglong Dong

>  
>  		cand_ids->data[j++] = cand_spec.spec[0].type_id;
> @@ -5042,13 +5115,18 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
>  	 * verifier. If it was an error, then verifier will complain and point
>  	 * to a specific instruction number in its log.
>  	 */
> -	if (j == 0)
> +	if (j == 0) {
>  		pr_debug("prog '%s': relo #%d: no matching targets found\n",
>  			 prog_name, relo_idx);
>  
> -	/* bpf_core_reloc_insn should know how to handle missing targ_spec */
> -	err = bpf_core_reloc_insn(prog, relo, relo_idx, &local_spec,
> -				  j ? &targ_spec : NULL);
> +		/* calculate single target relo result explicitly */
> +		err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, NULL, &targ_res);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* bpf_core_patch_insn() should know how to handle missing targ_spec */
> +	err = bpf_core_patch_insn(prog, relo, relo_idx, &targ_res);
>  	if (err) {
>  		pr_warn("prog '%s': relo #%d: failed to patch insn at offset %d: %d\n",
>  			prog_name, relo_idx, relo->insn_off, err);
> -- 
> 2.24.1
> 
>