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: <B7A2A8DD-B070-4F80-A9A0-6570260D4346@fb.com>
Date:   Thu, 9 Jan 2020 18:09:08 +0000
From:   Song Liu <songliubraving@...com>
To:     Alexei Starovoitov <ast@...nel.org>
CC:     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 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()). 

Thanks,
Song