| 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
| ||
|
Date: Mon, 7 Mar 2022 05:13:08 +0530
From: Kumar Kartikeya Dwivedi <memxor@...il.com>
To: bpf@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>,
Andrii Nakryiko <andrii@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Toke Høiland-Jørgensen <toke@...hat.com>,
Jesper Dangaard Brouer <hawk@...nel.org>,
Lorenzo Bianconi <lorenzo@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Jakub Kicinski <kuba@...nel.org>, Lorenz Bauer <linux@....io>,
netdev@...r.kernel.org
Subject: [PATCH bpf-next v1 2/5] bpf: Introduce pkt_uid concept for PTR_TO_PACKET
Add a new member in PTR_TO_PACKET specific register state, namely
pkt_uid. This is used to classify packet pointers into different sets,
and the invariant is that any pkt pointers not belonging to the same
set, i.e. not sharing same pkt_uid, won't be allowed for comparison with
each other. During range propagation in __find_good_pkt_pointers, we now
need to take care to skip packet pointers with a different pkt_uid.
This can be used to set for a packet pointer returned from a helper
'bpf_packet_pointer' in the next patch, that encodes the range from the
len parameter it gets. Generating a distinct pkt_uid means this pointer
cannot be compared with other packet pointers and its range cannot be
manipulated.
Note that for helpers which change underlying packet data, we don't make
any distinction based on pkt_uid for clear_all_pkt_pointers, since even
though the pkt_uid is different, they all point into ctx.
regsafe is updated to match non-zero pkt_uid using the idmap to ensure
it rejects distinct pkt_uid pkt pointers.
We also replace memset of reg->raw to set range to 0, but it is helpful
to elaborate on why replacing it with reg->range = 0 is correct. In
commit 0962590e5533 ("bpf: fix partial copy of map_ptr when dst is scalar"),
the copying was changed to use raw so that all possible members of type
specific register state are copied, since at that point the type of
register is not known. But inside the reg_is_pkt_pointer block, there is
no need to memset the whole 'raw' struct, since we also have a pkt_uid
member that we now want to preserve after copying from one register to
another, for pkt pointers. A test for this case has been included to
prevent regressions.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
---
include/linux/bpf_verifier.h | 9 ++++++-
kernel/bpf/verifier.c | 47 ++++++++++++++++++++++++++++--------
2 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c1fc4af47f69..0379f953cf22 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -50,7 +50,14 @@ struct bpf_reg_state {
s32 off;
union {
/* valid when type == PTR_TO_PACKET */
- int range;
+ struct {
+ int range;
+ /* This is used to tag some PTR_TO_PACKET so that they
+ * cannot be compared existing PTR_TO_PACKET with
+ * different pkt_uid.
+ */
+ u32 pkt_uid;
+ };
/* valid when type == CONST_PTR_TO_MAP | PTR_TO_MAP_VALUE |
* PTR_TO_MAP_VALUE_OR_NULL
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0373d5bd240f..88ac2c833bed 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -712,8 +712,14 @@ static void print_verifier_state(struct bpf_verifier_env *env,
verbose_a("ref_obj_id=%d", reg->ref_obj_id);
if (t != SCALAR_VALUE)
verbose_a("off=%d", reg->off);
- if (type_is_pkt_pointer(t))
+ if (type_is_pkt_pointer(t)) {
verbose_a("r=%d", reg->range);
+ /* pkt_uid is only set for PTR_TO_PACKET, so
+ * type_is_pkt_pointer check is enough.
+ */
+ if (reg->pkt_uid)
+ verbose_a("pkt_uid=%d", reg->pkt_uid);
+ }
else if (base_type(t) == CONST_PTR_TO_MAP ||
base_type(t) == PTR_TO_MAP_KEY ||
base_type(t) == PTR_TO_MAP_VALUE)
@@ -7604,7 +7610,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
if (reg_is_pkt_pointer(ptr_reg)) {
dst_reg->id = ++env->id_gen;
/* something was added to pkt_ptr, set range to zero */
- memset(&dst_reg->raw, 0, sizeof(dst_reg->raw));
+ dst_reg->range = 0;
}
break;
case BPF_SUB:
@@ -7664,7 +7670,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
dst_reg->id = ++env->id_gen;
/* something was added to pkt_ptr, set range to zero */
if (smin_val < 0)
- memset(&dst_reg->raw, 0, sizeof(dst_reg->raw));
+ dst_reg->range = 0;
}
break;
case BPF_AND:
@@ -8701,7 +8707,8 @@ static void __find_good_pkt_pointers(struct bpf_func_state *state,
for (i = 0; i < MAX_BPF_REG; i++) {
reg = &state->regs[i];
- if (reg->type == type && reg->id == dst_reg->id)
+ if (reg->type == type && reg->id == dst_reg->id &&
+ reg->pkt_uid == dst_reg->pkt_uid)
/* keep the maximum range already checked */
reg->range = max(reg->range, new_range);
}
@@ -8709,7 +8716,8 @@ static void __find_good_pkt_pointers(struct bpf_func_state *state,
bpf_for_each_spilled_reg(i, state, reg) {
if (!reg)
continue;
- if (reg->type == type && reg->id == dst_reg->id)
+ if (reg->type == type && reg->id == dst_reg->id &&
+ reg->pkt_uid == dst_reg->pkt_uid)
reg->range = max(reg->range, new_range);
}
}
@@ -9330,6 +9338,14 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
__mark_ptr_or_null_regs(vstate->frame[i], id, is_null);
}
+static bool is_bad_pkt_comparison(const struct bpf_reg_state *dst_reg,
+ const struct bpf_reg_state *src_reg)
+{
+ if (!reg_is_pkt_pointer_any(dst_reg) || !reg_is_pkt_pointer_any(src_reg))
+ return false;
+ return dst_reg->pkt_uid != src_reg->pkt_uid;
+}
+
static bool try_match_pkt_pointers(const struct bpf_insn *insn,
struct bpf_reg_state *dst_reg,
struct bpf_reg_state *src_reg,
@@ -9343,6 +9359,9 @@ static bool try_match_pkt_pointers(const struct bpf_insn *insn,
if (BPF_CLASS(insn->code) == BPF_JMP32)
return false;
+ if (is_bad_pkt_comparison(dst_reg, src_reg))
+ return false;
+
switch (BPF_OP(insn->code)) {
case BPF_JGT:
if ((dst_reg->type == PTR_TO_PACKET &&
@@ -9640,11 +9659,17 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
mark_ptr_or_null_regs(other_branch, insn->dst_reg,
opcode == BPF_JEQ);
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg],
- this_branch, other_branch) &&
- is_pointer_value(env, insn->dst_reg)) {
- verbose(env, "R%d pointer comparison prohibited\n",
- insn->dst_reg);
- return -EACCES;
+ this_branch, other_branch)) {
+ if (is_pointer_value(env, insn->dst_reg)) {
+ verbose(env, "R%d pointer comparison prohibited\n",
+ insn->dst_reg);
+ return -EACCES;
+ }
+ if (is_bad_pkt_comparison(dst_reg, ®s[insn->src_reg])) {
+ verbose(env, "R%d, R%d pkt pointer comparison prohibited\n",
+ insn->dst_reg, insn->src_reg);
+ return -EACCES;
+ }
}
if (env->log.level & BPF_LOG_LEVEL)
print_insn_state(env, this_branch->frame[this_branch->curframe]);
@@ -10891,6 +10916,8 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
/* id relations must be preserved */
if (rold->id && !check_ids(rold->id, rcur->id, idmap))
return false;
+ if (rold->pkt_uid && !check_ids(rold->pkt_uid, rcur->pkt_uid, idmap))
+ return false;
/* new val must satisfy old val knowledge */
return range_within(rold, rcur) &&
tnum_in(rold->var_off, rcur->var_off);
--
2.35.1
Powered by blists - more mailing lists