linux-kernel - Re: [PATCH 1/2] bpf: add a bpf_override

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: <59FBA64D.1050400@iogearbox.net>
Date:   Fri, 03 Nov 2017 00:12:13 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Josef Bacik <josef@...icpanda.com>, rostedt@...dmis.org,
        mingo@...hat.com, davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, ast@...nel.org, kernel-team@...com
CC:     Josef Bacik <jbacik@...com>
Subject: Re: [PATCH 1/2] bpf: add a bpf_override_function helper

Hi Josef,

one more issue I just noticed, see comment below:

On 11/02/2017 03:37 PM, Josef Bacik wrote:
[...]
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index cdd78a7beaae..dfa44fd74bae 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -458,7 +458,8 @@ struct bpf_prog {
>   				locked:1,	/* Program image locked? */
>   				gpl_compatible:1, /* Is filter GPL compatible? */
>   				cb_access:1,	/* Is control block accessed? */
> -				dst_needed:1;	/* Do we need dst entry? */
> +				dst_needed:1,	/* Do we need dst entry? */
> +				kprobe_override:1; /* Do we override a kprobe? */
>   	kmemcheck_bitfield_end(meta);
>   	enum bpf_prog_type	type;		/* Type of BPF program */
>   	u32			len;		/* Number of filter blocks */
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d906775e12c1..f8f7927a9152 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>   			prog->dst_needed = 1;
>   		if (insn->imm == BPF_FUNC_get_prandom_u32)
>   			bpf_user_rnd_init_once();
> +		if (insn->imm == BPF_FUNC_override_return)
> +			prog->kprobe_override = 1;
>   		if (insn->imm == BPF_FUNC_tail_call) {
>   			/* If we tail call into other programs, we
>   			 * cannot make any assumptions since they can
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9660ee65fbef..0d7fce52391d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
>   		return -EINVAL;
>   	}
>
> +	/* Kprobe override only works for kprobes, not uprobes. */
> +	if (prog->kprobe_override &&
> +	    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {
> +		bpf_prog_put(prog);
> +		return -EINVAL;
> +	}

Can we somehow avoid the prog->kprobe_override flag here completely
and also same in the perf_event_attach_bpf_prog() handler?

Reason is that it's not reliable for bailing out this way: Think of
the main program you're attaching doesn't use bpf_override_return()
helper, but it tail-calls into other BPF progs that make use of it
instead. So above check would be useless and will fail and we continue
to attach the prog for probes where it's not intended to be used.

We've had similar issues in the past e.g. c2002f983767 ("bpf: fix
checking xdp_adjust_head on tail calls") is just one of those. Thus,
can we avoid the flag altogether and handle such error case differently?

>   	if (is_tracepoint || is_syscall_tp) {
>   		int off = trace_event_get_offsets(event->tp_event);

Thanks,
Daniel