netdev - Re: [PATCH v2 bpf-next 3/7] bpf: Introduce function-by-function verification

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: <20200109221739.a7wuiqe37rqameqh@ast-mbp>
Date:   Thu, 9 Jan 2020 14:17:40 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Song Liu <songliubraving@...com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        David Miller <davem@...emloft.net>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 3/7] bpf: Introduce function-by-function
 verification

On Thu, Jan 09, 2020 at 06:09:08PM +0000, Song Liu wrote:
> 
> 
> > On Jan 8, 2020, at 10:37 PM, Alexei Starovoitov <ast@...nel.org> wrote:
> 
> [...]
> 
> > 
> > Note that the stack limit of 512 still applies to the call chain regardless whether
> > functions were static or global. The nested level of 8 also still applies. The
> > same recursion prevention checks are in place as well.
> > 
> > The type information and static/global kind is preserved after the verification
> > hence in the above example global function f2() and f3() can be replaced later
> > by equivalent functions with the same types that are loaded and verified later
> > without affecting safety of this main() program. Such replacement (re-linking)
> > of global functions is a subject of future patches.
> > 
> > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> 
> Acked-by: Song Liu <songliubraving@...com>
> 
> With one nit below. 
> 
> [...]
> 
> > +
> > +static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > +{
> > +	struct bpf_verifier_state *state;
> > +	struct bpf_reg_state *regs;
> > +	int ret, i;
> > +
> > +	env->prev_linfo = NULL;
> > +	env->pass_cnt++;
> > +
> > +	state = kzalloc(sizeof(struct bpf_verifier_state), GFP_KERNEL);
> > +	if (!state)
> > +		return -ENOMEM;
> > +	state->curframe = 0;
> > +	state->speculative = false;
> > +	state->branches = 1;
> > +	state->frame[0] = kzalloc(sizeof(struct bpf_func_state), GFP_KERNEL);
> > +	if (!state->frame[0]) {
> > +		kfree(state);
> > +		return -ENOMEM;
> > +	}
> > +	env->cur_state = state;
> > +	init_func_state(env, state->frame[0],
> > +			BPF_MAIN_FUNC /* callsite */,
> > +			0 /* frameno */,
> > +			subprog);
> > +
> > +	regs = state->frame[state->curframe]->regs;
> > +	if (subprog) {
> > +		ret = btf_prepare_func_args(env, subprog, regs);
> > +		if (ret)
> > +			goto out;
> > +		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> > +			if (regs[i].type == PTR_TO_CTX)
> > +				mark_reg_known_zero(env, regs, i);
> > +			else if (regs[i].type == SCALAR_VALUE)
> > +				mark_reg_unknown(env, regs, i);
> > +		}
> > +	} else {
> > +		/* 1st arg to a function */
> > +		regs[BPF_REG_1].type = PTR_TO_CTX;
> > +		mark_reg_known_zero(env, regs, BPF_REG_1);
> > +		ret = btf_check_func_arg_match(env, subprog, regs);
> > +		if (ret == -EFAULT)
> > +			/* unlikely verifier bug. abort.
> > +			 * ret == 0 and ret < 0 are sadly acceptable for
> > +			 * main() function due to backward compatibility.
> > +			 * Like socket filter program may be written as:
> > +			 * int bpf_prog(struct pt_regs *ctx)
> > +			 * and never dereference that ctx in the program.
> > +			 * 'struct pt_regs' is a type mismatch for socket
> > +			 * filter that should be using 'struct __sk_buff'.
> > +			 */
> > +			goto out;
> > +	}
> > +
> > +	ret = do_check(env);
> > +out:
> > +	if (env->cur_state) {
> 
> I think env->cur_state will never be NULL here. This check is necessary 
> before this patch (when we allocate cur_state in do_check()). 

yeah. good catch. 'if' can be dropped. I'll follow up with a clean up patch or
will fold it if respin is necessary for other reasons.