iv/linux
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bpf: Account for BPF_FETCH in insn_has_def32()

Browse Source 
insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes
adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1].
This happens because insn_no_def() does not know about the BPF_FETCH
variants of BPF_STX.

Fix in two steps.

First, replace insn_no_def() with insn_def_regno(), which returns the
register an insn defines. Normally insn_no_def() calls are followed by
insn->dst_reg uses; replace those with the insn_def_regno() return
value.

Second, adjust the BPF_STX special case in is_reg64() to deal with
queries made from opt_subreg_zext_lo32_rnd_hi32(), where the state
information is no longer available. Add a comment, since the purpose
of this special case is not clear at first glance.

  [1] https://lore.kernel.org/bpf/20210223150845.1857620-1-jackmanb@google.com/

Fixes: 5ffa25502b ("bpf: Add instructions for atomic_[cmp]xchg")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Martin KaFai Lau <kafai@fb.com>
Acked-by: Brendan Jackman <jackmanb@google.com>
Link: https://lore.kernel.org/bpf/20210301154019.129110-1-iii@linux.ibm.com
This commit is contained in:
Ilya Leoshkevich 2021-03-01 16:40:19 +01:00 committed by Daniel Borkmann
parent 2b2aedabc4
commit 83a2881903
1 changed files with 39 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
kernel/bpf/verifier.c
View File

@ -1703,7 +1703,11 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
}
if (class == BPF_STX) {
if (reg->type != SCALAR_VALUE)
/* BPF_STX (including atomic variants) has multiple source
* operands, one of which is a ptr. Check whether the caller is
* asking about it.
*/
if (t == SRC_OP && reg->type != SCALAR_VALUE)
return true;
return BPF_SIZE(code) == BPF_DW;
}
@ -1735,22 +1739,38 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
return true;
}
/* Return TRUE if INSN doesn't have explicit value define. */
static bool insn_no_def(struct bpf_insn *insn)
/* Return the regno defined by the insn, or -1. */
static int insn_def_regno(const struct bpf_insn *insn)
{
u8 class = BPF_CLASS(insn->code);
return (class == BPF_JMP || class == BPF_JMP32 ||
class == BPF_STX || class == BPF_ST);
switch (BPF_CLASS(insn->code)) {
case BPF_JMP:
case BPF_JMP32:
case BPF_ST:
return -1;
case BPF_STX:
if (BPF_MODE(insn->code) == BPF_ATOMIC &&
(insn->imm & BPF_FETCH)) {
if (insn->imm == BPF_CMPXCHG)
return BPF_REG_0;
else
return insn->src_reg;
} else {
return -1;
}
default:
return insn->dst_reg;
}
}
/* Return TRUE if INSN has defined any 32-bit value explicitly. */
static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn)
{
if (insn_no_def(insn))
int dst_reg = insn_def_regno(insn);
if (dst_reg == -1)
return false;
return !is_reg64(env, insn, insn->dst_reg, NULL, DST_OP);
return !is_reg64(env, insn, dst_reg, NULL, DST_OP);
}
static void mark_insn_zext(struct bpf_verifier_env *env,
@ -11006,9 +11026,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
for (i = 0; i < len; i++) {
int adj_idx = i + delta;
struct bpf_insn insn;
u8 load_reg;
int load_reg;
insn = insns[adj_idx];
load_reg = insn_def_regno(&insn);
if (!aux[adj_idx].zext_dst) {
u8 code, class;
u32 imm_rnd;
@ -11018,14 +11039,14 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
code = insn.code;
class = BPF_CLASS(code);
if (insn_no_def(&insn))
if (load_reg == -1)
continue;
/* NOTE: arg "reg" (the fourth one) is only used for
* BPF_STX which has been ruled out in above
* check, it is safe to pass NULL here.
* BPF_STX + SRC_OP, so it is safe to pass NULL
* here.
*/
if (is_reg64(env, &insn, insn.dst_reg, NULL, DST_OP)) {
if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) {
if (class == BPF_LD &&
BPF_MODE(code) == BPF_IMM)
i++;
@ -11040,7 +11061,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
imm_rnd = get_random_int();
rnd_hi32_patch[0] = insn;
rnd_hi32_patch[1].imm = imm_rnd;
rnd_hi32_patch[3].dst_reg = insn.dst_reg;
rnd_hi32_patch[3].dst_reg = load_reg;
patch = rnd_hi32_patch;
patch_len = 4;
goto apply_patch_buffer;
@ -11049,22 +11070,9 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
if (!bpf_jit_needs_zext())
continue;
/* zext_dst means that we want to zero-extend whatever register
* the insn defines, which is dst_reg most of the time, with
* the notable exception of BPF_STX + BPF_ATOMIC + BPF_FETCH.
*/
if (BPF_CLASS(insn.code) == BPF_STX &&
BPF_MODE(insn.code) == BPF_ATOMIC) {
/* BPF_STX + BPF_ATOMIC insns without BPF_FETCH do not
* define any registers, therefore zext_dst cannot be
* set.
*/
if (WARN_ON(!(insn.imm & BPF_FETCH)))
return -EINVAL;
load_reg = insn.imm == BPF_CMPXCHG ? BPF_REG_0
: insn.src_reg;
} else {
load_reg = insn.dst_reg;
if (WARN_ON(load_reg == -1)) {
verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
return -EFAULT;
}
zext_patch[0] = insn;