On 07/09/2023 21:06, Jiri Olsa wrote:
> Currently the multi_kprobe link attach does not check error
> injection list for programs with bpf_override_return helper
> and allows them to attach anywhere. Adding the missing check.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
For the series,
Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
...with one small question below...
> ---
> kernel/trace/bpf_trace.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a7264b2c17ad..c1c1af63ced2 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
> return arr.mods_cnt;
> }
>
> +static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
> +{
> + u32 i;
> +
> + for (i = 0; i < cnt; i++) {
> + if (!within_error_injection_list(addrs[i]))
> + return -EINVAL;
do we need a check like trace_kprobe_on_func_entry() to verify that
it's a combination of function entry+kprobe override, or is that
handled elsewhere/not needed? perf_event_attach_bpf_prog() does
/*
* Kprobe override only works if they are on the function entry,
* and only if they are on the opt-in list.
*/
if (prog->kprobe_override &&
(!trace_kprobe_on_func_entry(event->tp_event) ||
!trace_kprobe_error_injectable(event->tp_event)))
return -EINVAL;
if this is needed, it might be good to augment the selftest to
cover the case of specifying non-entry+kprobe override. thanks!
Alan
> + return 0;
> +}
> +
> int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> {
> struct bpf_kprobe_multi_link *link = NULL;
> @@ -2930,6 +2941,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> goto error;
> }
>
> + if (prog->kprobe_override && addrs_check_error_injection_list(addrs, cnt)) {
> + err = -EINVAL;
> + goto error;
> + }
> +
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (!link) {
> err = -ENOMEM;
