Re: [RFC PATCH bpf-next 10/12] bpf: Support attach and detach for SDT observer program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Trampoline key space collision between SDT probe offsets and BTF Type IDs leading to verifier bypass and arbitrary memory access.
- [Low] Multi-line comment opening is not on its own line, violating BPF subsystem guidelines.
--

commit b1194a14a242f8613863b3524830be07aca1a7e7
Author: Xu Kuohai <xukuohai@huawei.com>

bpf: Support attach and detach for SDT observer program

This commit adds support to attach an SDT observer program by reusing the
bpf trampoline attach and detach mechanism. The trampoline is keyed by
the target program fd and the offset of the probe. The function prototype
is resolved from prog->aux->sdt_probe.

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 93c196f38b878..6548a2dd76da6 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1270,6 +1270,12 @@ struct btf_func_model {
>   */
>  #define BPF_TRAMP_F_INDIRECT		BIT(8)
>  
> +/* Trampoline is for an SDT probe site: arguments arrive in
> + * BPF registers recorded by the SDT macro at build time,
> + * described by btf_func_model.arg_regs[] instead of ABI order.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred
BPF subsystem style?

The opening block comment normally has the text starting on the next line:

/*
 * Trampoline is for an SDT probe site: arguments arrive in
 * ...

[ ... ]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8b2c73bb6c2a8..53897ff1100fa 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c

[ ... ]

> +static int bpf_sdt_link_attach(struct bpf_prog *prog)
> +{

[ ... ]

> +	err = bpf_link_prime(&sdt_link->link.link, &primer);
> +	if (err) {
> +		kfree(sdt_link);
> +		return err;
> +	}
> +
> +	/* use orig_off to compute the key as orig_off is unique for each probe */
> +	tr_key = bpf_trampoline_compute_key(tgt_prog, NULL, sdt->val.orig_off);

[Severity: Critical]
Does using sdt->val.orig_off here create a risk of trampoline key collisions
with BTF Type IDs?

Looking at bpf_trampoline_compute_key():

include/linux/bpf_verifier.h:bpf_trampoline_compute_key() {
    if (tgt_prog)
        return ((u64)tgt_prog->aux->id << 32) | btf_id;
    ...
}

Because the instruction offset is passed in place of the btf_id, an SDT probe
at an offset could generate the exact same 64-bit key as a subprogram with a
BTF Type ID matching that offset.

If both an SDT observer program and an fentry program attach using this same
key, they would share the same trampoline.

Could this allow the verifier to treat arbitrary scalars from the SDT probe
context as trusted pointers if they are passed into the fentry program?

> +	tgt_info.tgt_addr = sdt->probe_ip;
> +	tgt_info.tgt_name = sdt->val.name;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782571533.git.xukuohai@huawei.com?part=10