| 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: <b7de0135f7dcca0485ce9dc853d6ca812c30244b.camel@gmail.com>
Date: Tue, 28 Jan 2025 16:19:25 -0800
From: Eduard Zingerman <eddyz87@...il.com>
To: Peilin Ye <yepeilin@...gle.com>, bpf@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Cc: bpf@...f.org, Xu Kuohai <xukuohai@...weicloud.com>, David Vernet
<void@...ifault.com>, 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>, Jonathan Corbet
<corbet@....net>, "Paul E. McKenney" <paulmck@...nel.org>, Puranjay Mohan
<puranjay@...nel.org>, Catalin Marinas <catalin.marinas@....com>, Will
Deacon <will@...nel.org>, Quentin Monnet <qmo@...nel.org>, Mykola Lysenko
<mykolal@...com>, Shuah Khan <shuah@...nel.org>, Josh Don
<joshdon@...gle.com>, Barret Rhoden <brho@...gle.com>, Neel Natu
<neelnatu@...gle.com>, Benjamin Segall <bsegall@...gle.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v1 3/8] bpf: Introduce load-acquire and
store-release instructions
On Sat, 2025-01-25 at 02:18 +0000, Peilin Ye wrote:
> Introduce BPF instructions with load-acquire and store-release
> semantics, as discussed in [1]. The following new flags are defined:
>
> BPF_ATOMIC_LOAD 0x10
> BPF_ATOMIC_STORE 0x20
> BPF_ATOMIC_TYPE(imm) ((imm) & 0xf0)
>
> BPF_RELAXED 0x0
> BPF_ACQUIRE 0x1
> BPF_RELEASE 0x2
> BPF_ACQ_REL 0x3
> BPF_SEQ_CST 0x4
>
> BPF_LOAD_ACQ (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
> BPF_STORE_REL (BPF_ATOMIC_STORE | BPF_RELEASE)
>
> A "load-acquire" is a BPF_STX | BPF_ATOMIC instruction with the 'imm'
> field set to BPF_LOAD_ACQ (0x11).
>
> Similarly, a "store-release" is a BPF_STX | BPF_ATOMIC instruction with
> the 'imm' field set to BPF_STORE_REL (0x22).
>
> Unlike existing atomic operations that only support BPF_W (32-bit) and
> BPF_DW (64-bit) size modifiers, load-acquires and store-releases also
> support BPF_B (8-bit) and BPF_H (16-bit). An 8- or 16-bit load-acquire
> zero-extends the value before writing it to a 32-bit register, just like
> ARM64 instruction LDARH and friends.
>
> As an example, consider the following 64-bit load-acquire BPF
> instruction:
>
> db 10 00 00 11 00 00 00 r0 = load_acquire((u64 *)(r1 + 0x0))
>
> opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
> imm (0x00000011): BPF_LOAD_ACQ
>
> Similarly, a 16-bit BPF store-release:
>
> cb 21 00 00 22 00 00 00 store_release((u16 *)(r1 + 0x0), w2)
>
> opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
> imm (0x00000022): BPF_STORE_REL
>
> [1] https://lore.kernel.org/all/20240729183246.4110549-1-yepeilin@google.com/
>
> Signed-off-by: Peilin Ye <yepeilin@...gle.com>
> ---
I think bpf_jit_supports_insn() in arch/{x86,s390}/net/bpf_jit_comp.c
need an update, as both would accept BPF_LOAD_ACQ/BPF_STORE_REL at the
moment.
Acked-by: Eduard Zingerman <eddyz87@...il.com>
[...]
> +static int check_atomic_load(struct bpf_verifier_env *env, int insn_idx,
> + struct bpf_insn *insn)
> +{
> + struct bpf_reg_state *regs = cur_regs(env);
> + int err;
> +
> + err = check_reg_arg(env, insn->src_reg, SRC_OP);
> + if (err)
> + return err;
> +
> + err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
> + if (err)
> + return err;
> +
> + if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {
> + verbose(env, "BPF_ATOMIC loads from R%d %s is not allowed\n",
> + insn->src_reg,
> + reg_type_str(env, reg_state(env, insn->src_reg)->type));
> + return -EACCES;
> + }
> +
> + if (is_arena_reg(env, insn->src_reg)) {
> + err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
> + if (err)
> + return err;
Nit: this and the next function look very similar to processing of
generic load and store in do_check(). Maybe extract that code
as an auxiliary function and call it in both places?
The only major difference is is_arena_reg() check guarding
save_aux_ptr_type(), but I think it is ok to do save_aux_ptr_type
unconditionally. Fwiw, the code would be a bit simpler,
just spent half an hour convincing myself that such conditional handling
is not an error. Wdyt?
> + }
> +
> + /* Check whether we can read the memory. */
> + err = check_mem_access(env, insn_idx, insn->src_reg, insn->off,
> + BPF_SIZE(insn->code), BPF_READ, insn->dst_reg,
> + true, false);
> + if (err)
> + return err;
> +
> + err = reg_bounds_sanity_check(env, ®s[insn->dst_reg], "atomic_load");
> + if (err)
> + return err;
> + return 0;
> +}
> +
> +static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
> + struct bpf_insn *insn)
> +{
> + int err;
> +
> + err = check_reg_arg(env, insn->src_reg, SRC_OP);
> + if (err)
> + return err;
> +
> + err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> + if (err)
> + return err;
> +
> + if (is_pointer_value(env, insn->src_reg)) {
> + verbose(env, "R%d leaks addr into mem\n", insn->src_reg);
> + return -EACCES;
> + }
> +
> + if (!atomic_ptr_type_ok(env, insn->dst_reg, insn)) {
> + verbose(env, "BPF_ATOMIC stores into R%d %s is not allowed\n",
> + insn->dst_reg,
> + reg_type_str(env, reg_state(env, insn->dst_reg)->type));
> + return -EACCES;
> + }
> +
> + if (is_arena_reg(env, insn->dst_reg)) {
> + err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
> + if (err)
> + return err;
> + }
> +
> + /* Check whether we can write into the memory. */
> + err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> + BPF_SIZE(insn->code), BPF_WRITE, insn->src_reg,
> + true, false);
> + if (err)
> + return err;
> + return 0;
> +}
> +
[...]
Powered by blists - more mailing lists