netdev - Re: [PATCH bpf] bpf: report verifier bugs as warnings

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Apr 2019 16:37:19 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Paul Chaignon <paul.chaignon@...nge.com>,
        Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     Xiao Han <xiao.han@...nge.com>, paul.chaignon@...il.com,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH bpf] bpf: report verifier bugs as warnings

On 04/02/2019 01:58 PM, Paul Chaignon wrote:
> Three checks for verifier bugs were introduced in commit f4d7e40 ("bpf:
> introduce function calls (verification)").  The bugs were reported as
> incorrect programs instead of kernel warnings as the present patch
> implements.
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@...nge.com>

Thanks for the patch, I think these WARN_ONCE() are a bit out of context though,
meaning it lacks additional information about the program in kernel log once we
actually manage to trigger it which we'd otherwise would potentially have had with
the verbose() log. And from a program debugging pov, it makes it harder after this
patch when verification log would suggest that all is fine. Looks like we already
have a few WARN_ONCE() in verifier, they should probably be converted to verbose()
as well to be consistent. If we really want to have a kernel warn, then lets add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps
the verbose log intact as well.

> ---
>  kernel/bpf/verifier.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 87221fda1321..12499e72b0d5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1145,9 +1145,9 @@ static int mark_reg_read(struct bpf_verifier_env *env,
>  		if (writes && state->live & REG_LIVE_WRITTEN)
>  			break;
>  		if (parent->live & REG_LIVE_DONE) {
> -			verbose(env, "verifier BUG type %s var_off %lld off %d\n",
> -				reg_type_str[parent->type],
> -				parent->var_off.value, parent->off);
> +			WARN_ONCE(1, "verifier bug type %s var_off %lld off %d\n",
> +				  reg_type_str[parent->type],
> +				  parent->var_off.value, parent->off);
>  			return -EFAULT;
>  		}
>  		/* ... then we depend on parent's value */
> @@ -2888,15 +2888,15 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	target_insn = *insn_idx + insn->imm;
>  	subprog = find_subprog(env, target_insn + 1);
>  	if (subprog < 0) {
> -		verbose(env, "verifier bug. No program starts at insn %d\n",
> -			target_insn + 1);
> +		WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
> +			  target_insn + 1);
>  		return -EFAULT;
>  	}
>  
>  	caller = state->frame[state->curframe];
>  	if (state->frame[state->curframe + 1]) {
> -		verbose(env, "verifier bug. Frame %d already allocated\n",
> -			state->curframe + 1);
> +		WARN_ONCE(1, "verifier bug. Frame %d already allocated\n",
> +			  state->curframe + 1);
>  		return -EFAULT;
>  	}
>  
>