| 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
| ||
|
Message-ID: <15294d369d94cf005c9aa722967e5ddb1fa8cee3.camel@gmail.com>
Date: Thu, 01 May 2025 11:22:53 -0700
From: Eduard Zingerman <eddyz87@...il.com>
To: Luis Gerhorst <luis.gerhorst@....de>, Alexei Starovoitov
<ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko
<andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu
<song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>, John Fastabend
<john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Stanislav
Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, Jiri Olsa
<jolsa@...nel.org>, Puranjay Mohan <puranjay@...nel.org>, Xu Kuohai
<xukuohai@...weicloud.com>, Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Hari Bathini <hbathini@...ux.ibm.com>,
Christophe Leroy <christophe.leroy@...roup.eu>, Naveen N Rao
<naveen@...nel.org>, Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael
Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>, Mykola
Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, Henriette Herzog
<henriette.herzog@....de>, Saket Kumar Bhaskar <skb99@...ux.ibm.com>,
Cupertino Miranda <cupertino.miranda@...cle.com>, Jiayuan Chen
<mrpre@....com>, Matan Shachnai <m.shachnai@...il.com>, Dimitar Kanaliev
<dimitar.kanaliev@...eground.com>, Shung-Hsi Yu <shung-hsi.yu@...e.com>,
Daniel Xu <dxu@...uu.xyz>, bpf@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-kselftest@...r.kernel.org
Cc: Maximilian Ott <ott@...fau.de>, Milan Stephan <milan.stephan@....de>
Subject: Re: [PATCH bpf-next v3 02/11] bpf: Move insn if/else into
do_check_insn()
On Thu, 2025-05-01 at 09:35 +0200, Luis Gerhorst wrote:
> This is required to catch the errors later and fall back to a nospec if
> on a speculative path.
>
> Eliminate the regs variable as it is only used once and insn_idx is not
> modified in-between the definition and usage.
>
> Still pass insn simply to match the other check_*() functions. As Eduard
> points out [1], insn is assumed to correspond to env->insn_idx in many
> places (e.g, __check_reg_arg()).
>
> Move code into do_check_insn(), replace
> * "continue" with "return 0" after modifying insn_idx
> * "goto process_bpf_exit" with "return PROCESS_BPF_EXIT"
> * "do_print_state = " with "*do_print_state = "
>
> [1] https://lore.kernel.org/all/293dbe3950a782b8eb3b87b71d7a967e120191fd.camel@gmail.com/
>
> Signed-off-by: Luis Gerhorst <luis.gerhorst@....de>
> Acked-by: Henriette Herzog <henriette.herzog@....de>
> Cc: Maximilian Ott <ott@...fau.de>
> Cc: Milan Stephan <milan.stephan@....de>
> ---
Except two notes below, I think this patch looks good.
Thank you, this is a good refactoring.
[...]
> +static int do_check_insn(struct bpf_verifier_env *env, struct bpf_insn *insn,
> + bool *do_print_state)
> +{
[...]
> + } else if (class == BPF_ST) {
> + enum bpf_reg_type dst_reg_type;
> +
> + if (BPF_MODE(insn->code) != BPF_MEM ||
> + insn->src_reg != BPF_REG_0) {
> + verbose(env, "BPF_ST uses reserved fields\n");
> + return -EINVAL;
> + }
> + /* check src operand */
> + err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> + if (err)
> + return err;
> +
> + dst_reg_type = cur_regs(env)[insn->dst_reg].type;
Implicitly relying on `insn == &env->prog->insnsi[env->cur_idx]`
is weird. Still think that `insn` parameter should be dropped and
computed inside this function instead.
> +
> + /* check that memory (dst_reg + off) is writeable */
> + err = check_mem_access(env, env->insn_idx, insn->dst_reg,
> + insn->off, BPF_SIZE(insn->code),
> + BPF_WRITE, -1, false, false);
> + if (err)
> + return err;
> +
> + err = save_aux_ptr_type(env, dst_reg_type, false);
> + if (err)
> + return err;
> + } else if (class == BPF_JMP || class == BPF_JMP32) {
[...]
> + } else if (opcode == BPF_EXIT) {
> + if (BPF_SRC(insn->code) != BPF_K ||
> + insn->imm != 0 ||
> + insn->src_reg != BPF_REG_0 ||
> + insn->dst_reg != BPF_REG_0 ||
> + class == BPF_JMP32) {
> + verbose(env, "BPF_EXIT uses reserved fields\n");
> + return -EINVAL;
> + }
> +process_bpf_exit_full:
Nit: since we are refactoring I'd extract this as a function instead of goto.
> + /* We must do check_reference_leak here before
> + * prepare_func_exit to handle the case when
> + * state->curframe > 0, it may be a callback function,
> + * for which reference_state must match caller reference
> + * state when it exits.
> + */
> + err = check_resource_leak(env, exception_exit, !env->cur_state->curframe,
> + "BPF_EXIT instruction in main prog");
> + if (err)
> + return err;
> +
> + /* The side effect of the prepare_func_exit which is
> + * being skipped is that it frees bpf_func_state.
> + * Typically, process_bpf_exit will only be hit with
> + * outermost exit. copy_verifier_state in pop_stack will
> + * handle freeing of any extra bpf_func_state left over
> + * from not processing all nested function exits. We
> + * also skip return code checks as they are not needed
> + * for exceptional exits.
> + */
> + if (exception_exit)
> + return PROCESS_BPF_EXIT;
> +
> + if (env->cur_state->curframe) {
> + /* exit from nested function */
> + err = prepare_func_exit(env, &env->insn_idx);
> + if (err)
> + return err;
> + *do_print_state = true;
> + return 0;
> + }
> +
> + err = check_return_code(env, BPF_REG_0, "R0");
> + if (err)
> + return err;
> + return PROCESS_BPF_EXIT;
[...]
Powered by blists - more mailing lists