Re: [PATCH bpf v3 4/7] bpf: Restrict CO-RE poisoning to relocatable instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Heap out-of-bounds read and write in libbpf when processing CO-RE relocations targeting a truncated ldimm64 instruction at the end of an ELF section.
--

commit cabdc155b3a9211d8b18a8961e8dcf7e4bc6ae61
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Restrict CO-RE poisoning to relocatable instructions

This patch restricts CO-RE poisoning to relocatable instructions. It handles
instruction poisoning after the instruction has passed the class and operand
checks, preventing unresolved relocations from altering the control flow by
poisoning instructions like exits or jumps.

> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> index 8ad2715721cfd..a884bc2bff198 100644
> --- a/tools/lib/bpf/relo_core.c
> +++ b/tools/lib/bpf/relo_core.c

[ ... ]

> @@ -1140,6 +1133,9 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
>  			return -EINVAL;
>  		}
>  
> +		if (res->poison)
> +			goto poison;
> +
>  		imm = (__u32)insn[0].imm | ((__u64)insn[1].imm << 32);

[Severity: High]
This isn't a bug introduced by this patch, but can this code cause an
out-of-bounds read?

Just before the new poison check in bpf_core_patch_insn(), the code validates
the BPF_LD instruction:

	case BPF_LD: {
		__u64 imm;

		if (!is_ldimm64_insn(insn) ||
		    insn[0].src_reg != 0 || insn[0].off != 0 ||
		    insn[1].code != 0 || insn[1].dst_reg != 0 ||
		    insn[1].src_reg != 0 || insn[1].off != 0) {

If an attacker crafts a malicious BPF ELF file where the final 8 bytes of an
ELF section form the first half of a BPF_LD instruction, prog->insns will be
allocated to end exactly at this half-instruction.

When processing a CO-RE relocation targeting this final instruction, would
evaluating insn[1] lead to a heap out-of-bounds read?

[ ... ]

> @@ -1164,6 +1160,16 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
>  	}
>  
>  	return 0;
> +
> +poison:
> +	/*
> +	 * Only relocatable instructions reach here. Poison both halves of
> +	 * ldimm64 so the verifier reports the bad relocation, not opcode 00.
> +	 */
> +	if (is_ldimm64_insn(insn))
> +		bpf_core_poison_insn(prog_name, relo_idx, insn_idx + 1, insn + 1);
> +	bpf_core_poison_insn(prog_name, relo_idx, insn_idx, insn);
> +	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but could this poison logic cause an
out-of-bounds write?

If a relocation targeting a truncated ldimm64 instruction is unresolvable and
marked for poisoning, execution reaches this poison block. Since
is_ldimm64_insn(insn) only checks the opcode of insn[0], it will evaluate to
true. 

Could bpf_core_poison_insn() then overwrite insn + 1 with a BPF_CALL
instruction past the end of the prog->insns array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914131701.2529725-1-memxor@gmail.com?part=4