Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1659883 > unrolled thread
| Started by | Edward Cree <ecree@solarflare.com> |
|---|---|
| First post | 2017-06-07 17:00 +0200 |
| Last post | 2017-06-08 22:20 +0200 |
| Articles | 20 on this page of 23 — 4 participants |
Back to article view | Back to linux.kernel
[RFC PATCH net-next 0/5] bpf: rewrite value tracking in verifier Edward Cree <ecree@solarflare.com> - 2017-06-07 17:00 +0200
[RFC PATCH net-next 1/5] selftests/bpf: add test for mixed signed and unsigned bounds checks Edward Cree <ecree@solarflare.com> - 2017-06-07 17:00 +0200
[RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations Edward Cree <ecree@solarflare.com> - 2017-06-07 17:10 +0200
Re: [RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 04:50 +0200
Re: [RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations Edward Cree <ecree@solarflare.com> - 2017-06-08 17:30 +0200
[RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Edward Cree <ecree@solarflare.com> - 2017-06-07 17:10 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 04:40 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Edward Cree <ecree@solarflare.com> - 2017-06-08 17:30 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 19:00 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Edward Cree <ecree@solarflare.com> - 2017-06-08 19:20 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 20:50 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Edward Cree <ecree@solarflare.com> - 2017-06-08 21:10 +0200
Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 23:20 +0200
Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 04:40 +0200
Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking Edward Cree <ecree@solarflare.com> - 2017-06-08 17:00 +0200
Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 18:50 +0200
Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking Edward Cree <ecree@solarflare.com> - 2017-06-08 21:40 +0200
Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 23:30 +0200
Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking Daniel Borkmann <daniel@iogearbox.net> - 2017-06-09 15:30 +0200
Re: [RFC PATCH net-next 4/5] bpf/verifier: track signed and unsigned min/max values Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 04:50 +0200
Re: [RFC PATCH net-next 4/5] bpf/verifier: track signed and unsigned min/max values Edward Cree <ecree@solarflare.com> - 2017-06-08 17:30 +0200
Re: [RFC PATCH net-next 4/5] bpf/verifier: track signed and unsigned min/max values Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2017-06-08 18:50 +0200
Re: [RFC PATCH net-next 0/5] bpf: rewrite value tracking in verifier David Miller <davem@davemloft.net> - 2017-06-08 22:20 +0200
Page 1 of 2 [1] 2 Next page →
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-07 17:00 +0200 |
| Subject | [RFC PATCH net-next 0/5] bpf: rewrite value tracking in verifier |
| Message-ID | <tPKBd-7Z0-25@gated-at.bofh.it> |
This series simplifies alignment tracking, generalises bounds tracking and
fixes some bounds-tracking bugs in the BPF verifier. Pointer arithmetic on
packet pointers, stack pointers, map value pointers and context pointers has
been unified, and bounds on these pointers are only checked when the pointer
is dereferenced.
Operations on pointers which destroy all relation to the original pointer
(such as multiplies and shifts) are disallowed if !env->allow_ptr_leaks,
otherwise they convert the pointer to an unknown scalar and feed it to the
normal scalar arithmetic handling.
Pointer types have been unified with the corresponding adjusted-pointer types
where those existed (e.g. PTR_TO_MAP_VALUE[_ADJ] or FRAME_PTR vs
PTR_TO_STACK); similarly, CONST_IMM and UNKNOWN_VALUE have been unified into
SCALAR_VALUE.
Pointer types (except CONST_PTR_TO_MAP, PTR_TO_MAP_VALUE_OR_NULL and
PTR_TO_PACKET_END, which do not allow arithmetic) have a 'fixed offset' and
a 'variable offset'; the former is used when e.g. adding an immediate or a
known-constant register, as long as it does not overflow. Otherwise the
latter is used, and any operation creating a new variable offset creates a
new 'id' (and, for PTR_TO_PACKET, clears the 'range').
SCALAR_VALUEs use the 'variable offset' fields to track the range of possible
values; the 'fixed offset' should never be set on a scalar.
Patch 2/5 is rather on the big side, but since it changes the contents and
semantics of a fairly central data structure, I'm not really sure how to go
about splitting it up further without producing broken intermediate states.
With the changes in patch 5/5, all tools/testing/selftests/bpf/test_verifier
tests pass.
Edward Cree (5):
selftests/bpf: add test for mixed signed and unsigned bounds checks
bpf/verifier: rework value tracking
bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU
path
bpf/verifier: track signed and unsigned min/max values
selftests/bpf: change test_verifier expectations
include/linux/bpf.h | 34 +-
include/linux/bpf_verifier.h | 56 +-
include/linux/tnum.h | 58 +
kernel/bpf/Makefile | 2 +-
kernel/bpf/tnum.c | 163 +++
kernel/bpf/verifier.c | 1852 ++++++++++++++++-----------
tools/testing/selftests/bpf/test_verifier.c | 248 ++--
7 files changed, 1482 insertions(+), 931 deletions(-)
create mode 100644 include/linux/tnum.h
create mode 100644 kernel/bpf/tnum.c
[toc] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-07 17:00 +0200 |
| Subject | [RFC PATCH net-next 1/5] selftests/bpf: add test for mixed signed and unsigned bounds checks |
| Message-ID | <tPKBd-7Z0-31@gated-at.bofh.it> |
| In reply to | #1659883 |
Currently fails due to bug in verifier bounds handling.
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
tools/testing/selftests/bpf/test_verifier.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index cabb19b..5074cfa 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -5169,6 +5169,32 @@ static struct bpf_test tests[] = {
},
.result = ACCEPT,
},
+ {
+ "bounds checks mixing signed and unsigned",
+ .insns = {
+ BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 7),
+ BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, -8),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_10, -16),
+ BPF_MOV64_IMM(BPF_REG_2, -1),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_1, BPF_REG_2, 3),
+ BPF_JMP_IMM(BPF_JSGT, BPF_REG_1, 1, 2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
+ BPF_ST_MEM(BPF_B, BPF_REG_0, 0, 0),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map1 = { 3 },
+ .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
+ .result = REJECT,
+ .result_unpriv = REJECT,
+ },
};
static int probe_filter_length(const struct bpf_insn *fp)
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-07 17:10 +0200 |
| Subject | [RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations |
| Message-ID | <tPKKR-8hH-5@gated-at.bofh.it> |
| In reply to | #1659883 |
Some of the verifier's error messages have changed, and some constructs
that previously couldn't be verified are now accepted.
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
tools/testing/selftests/bpf/test_verifier.c | 226 ++++++++++++++--------------
1 file changed, 116 insertions(+), 110 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 5074cfa..f5281df 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -421,7 +421,7 @@ static struct bpf_test tests[] = {
BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 0),
BPF_EXIT_INSN(),
},
- .errstr_unpriv = "R1 pointer arithmetic",
+ .errstr_unpriv = "R1 subtraction from stack pointer",
.result_unpriv = REJECT,
.errstr = "R1 invalid mem access",
.result = REJECT,
@@ -603,8 +603,9 @@ static struct bpf_test tests[] = {
BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_2, -4),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned stack access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"invalid map_fd for function call",
@@ -650,8 +651,9 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map1 = { 3 },
- .errstr = "misaligned access",
+ .errstr = "misaligned value access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"sometimes access memory with incorrect alignment",
@@ -672,6 +674,7 @@ static struct bpf_test tests[] = {
.errstr = "R0 invalid mem access",
.errstr_unpriv = "R0 leaks addr",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"jump test 1",
@@ -1184,8 +1187,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[0]) + 1),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: half, oob 1",
@@ -1279,8 +1283,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[0]) + 2),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: word, unaligned 2",
@@ -1290,8 +1295,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[4]) + 1),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: word, unaligned 3",
@@ -1301,8 +1307,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[4]) + 2),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: word, unaligned 4",
@@ -1312,8 +1319,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[4]) + 3),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: double",
@@ -1339,8 +1347,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[1])),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: double, unaligned 2",
@@ -1350,8 +1359,9 @@ static struct bpf_test tests[] = {
offsetof(struct __sk_buff, cb[3])),
BPF_EXIT_INSN(),
},
- .errstr = "misaligned access",
+ .errstr = "misaligned context access",
.result = REJECT,
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"check cb access: double, oob 1",
@@ -1505,7 +1515,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "misaligned access off -6 size 8",
+ .errstr = "misaligned stack access off (0x0; 0x0)+-8+2 size 8",
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"PTR_TO_STACK store/load - bad alignment on reg",
@@ -1517,7 +1528,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "misaligned access off -2 size 8",
+ .errstr = "misaligned stack access off (0x0; 0x0)+-10+8 size 8",
+ .flags = F_LOAD_WITH_STRICT_ALIGNMENT,
},
{
"PTR_TO_STACK store/load - out of bounds low",
@@ -1561,8 +1573,6 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = ACCEPT,
- .result_unpriv = REJECT,
- .errstr_unpriv = "R1 pointer arithmetic",
},
{
"unpriv: add pointer to pointer",
@@ -1573,7 +1583,7 @@ static struct bpf_test tests[] = {
},
.result = ACCEPT,
.result_unpriv = REJECT,
- .errstr_unpriv = "R1 pointer arithmetic",
+ .errstr_unpriv = "R1 pointer += pointer",
},
{
"unpriv: neg pointer",
@@ -1914,10 +1924,7 @@ static struct bpf_test tests[] = {
BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, -8),
BPF_EXIT_INSN(),
},
- .errstr_unpriv = "pointer arithmetic prohibited",
- .result_unpriv = REJECT,
- .errstr = "R1 invalid mem access",
- .result = REJECT,
+ .result = ACCEPT,
},
{
"unpriv: cmp of stack pointer",
@@ -1981,7 +1988,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid stack type R3",
+ .errstr = "R4 min value is negative",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
@@ -1998,7 +2005,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid stack type R3",
+ .errstr = "R4 min value is negative",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
@@ -2200,7 +2207,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid stack type R3 off=-1 access_size=-1",
+ .errstr = "R4 min value is negative",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
@@ -2217,7 +2224,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid stack type R3 off=-1 access_size=2147483647",
+ .errstr = "R4 unbounded memory access, use 'var &= const' or 'if (var < const)'",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
@@ -2234,7 +2241,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid stack type R3 off=-512 access_size=2147483647",
+ .errstr = "R4 unbounded memory access, use 'var &= const' or 'if (var < const)'",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
@@ -2634,7 +2641,7 @@ static struct bpf_test tests[] = {
BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1),
BPF_JMP_A(-6),
},
- .errstr = "misaligned packet access off 2+15+-4 size 4",
+ .errstr = "misaligned packet access off 2+(0x0; 0x0)+15+-4 size 4",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.flags = F_LOAD_WITH_STRICT_ALIGNMENT,
@@ -2929,7 +2936,7 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test14, cls helper fail sub",
+ "helper access to packet: test14, cls helper ok sub",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -2949,12 +2956,36 @@ static struct bpf_test tests[] = {
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN(),
},
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
+ "helper access to packet: test15, cls helper fail sub",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 7),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_1, BPF_REG_7, 6),
+ BPF_ALU64_IMM(BPF_SUB, BPF_REG_1, 12),
+ BPF_MOV64_IMM(BPF_REG_2, 4),
+ BPF_MOV64_IMM(BPF_REG_3, 0),
+ BPF_MOV64_IMM(BPF_REG_4, 0),
+ BPF_MOV64_IMM(BPF_REG_5, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_csum_diff),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
.result = REJECT,
- .errstr = "type=inv expected=fp",
+ .errstr = "invalid access to packet",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test15, cls helper fail range 1",
+ "helper access to packet: test16, cls helper fail range 1",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -2979,7 +3010,7 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test16, cls helper fail range 2",
+ "helper access to packet: test17, cls helper fail range 2",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -3000,11 +3031,11 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid access to packet",
+ .errstr = "R2 min value is negative",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test17, cls helper fail range 3",
+ "helper access to packet: test18, cls helper fail range 3",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -3025,11 +3056,11 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.result = REJECT,
- .errstr = "invalid access to packet",
+ .errstr = "R2 min value is negative",
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test18, cls helper fail range zero",
+ "helper access to packet: test19, cls helper fail range zero",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -3054,7 +3085,7 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test19, pkt end as input",
+ "helper access to packet: test20, pkt end as input",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -3079,7 +3110,7 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
- "helper access to packet: test20, wrong reg",
+ "helper access to packet: test21, wrong reg",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
offsetof(struct __sk_buff, data)),
@@ -3139,7 +3170,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result_unpriv = REJECT,
.result = ACCEPT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -3163,7 +3194,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result_unpriv = REJECT,
.result = ACCEPT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -3191,7 +3222,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result_unpriv = REJECT,
.result = ACCEPT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -3232,9 +3263,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
.errstr = "R0 min value is outside of the array range",
- .result_unpriv = REJECT,
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
@@ -3256,9 +3285,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
- .errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
- .result_unpriv = REJECT,
+ .errstr = "R0 unbounded memory access, make sure to bounds check any array access into a map",
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
@@ -3272,7 +3299,7 @@ static struct bpf_test tests[] = {
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 7),
- BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, 0),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 0),
BPF_MOV32_IMM(BPF_REG_2, MAX_ENTRIES),
BPF_JMP_REG(BPF_JSGT, BPF_REG_2, BPF_REG_1, 1),
BPF_MOV32_IMM(BPF_REG_1, 0),
@@ -3283,8 +3310,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
- .errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
+ .errstr_unpriv = "R0 leaks addr",
+ .errstr = "R0 unbounded memory access",
.result_unpriv = REJECT,
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -3310,7 +3337,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.errstr = "invalid access to map value, value_size=48 off=44 size=8",
.result_unpriv = REJECT,
.result = REJECT,
@@ -3340,8 +3367,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3, 11 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
- .errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
+ .errstr_unpriv = "R0 pointer += pointer",
+ .errstr = "R0 invalid mem access 'inv'",
.result_unpriv = REJECT,
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -3483,34 +3510,6 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS
},
{
- "multiple registers share map_lookup_elem bad reg type",
- .insns = {
- BPF_MOV64_IMM(BPF_REG_1, 10),
- BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
- BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
- BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
- BPF_LD_MAP_FD(BPF_REG_1, 0),
- BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
- BPF_FUNC_map_lookup_elem),
- BPF_MOV64_REG(BPF_REG_2, BPF_REG_0),
- BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),
- BPF_MOV64_REG(BPF_REG_4, BPF_REG_0),
- BPF_MOV64_REG(BPF_REG_5, BPF_REG_0),
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
- BPF_MOV64_IMM(BPF_REG_1, 1),
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
- BPF_MOV64_IMM(BPF_REG_1, 2),
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_3, 0, 1),
- BPF_ST_MEM(BPF_DW, BPF_REG_3, 0, 0),
- BPF_MOV64_IMM(BPF_REG_1, 3),
- BPF_EXIT_INSN(),
- },
- .fixup_map1 = { 4 },
- .result = REJECT,
- .errstr = "R3 invalid mem access 'inv'",
- .prog_type = BPF_PROG_TYPE_SCHED_CLS
- },
- {
"invalid map access from else condition",
.insns = {
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
@@ -3528,9 +3527,9 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "R0 unbounded memory access, make sure to bounds check any array access into a map",
+ .errstr = "R0 unbounded memory access",
.result = REJECT,
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result_unpriv = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
@@ -3842,7 +3841,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "invalid access to map value, value_size=48 off=0 size=-8",
+ .errstr = "R2 min value is negative",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -3954,7 +3953,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "invalid access to map value, value_size=48 off=4 size=-8",
+ .errstr = "R2 min value is negative",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -3976,7 +3975,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "R1 min value is outside of the array range",
+ .errstr = "R2 min value is negative",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -4092,7 +4091,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "invalid access to map value, value_size=48 off=4 size=-8",
+ .errstr = "R2 min value is negative",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -4115,7 +4114,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "R1 min value is outside of the array range",
+ .errstr = "R2 min value is negative",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -4203,13 +4202,13 @@ static struct bpf_test tests[] = {
BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_0, 0),
BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_3),
- BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_MOV64_IMM(BPF_REG_2, 1),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_EMIT_CALL(BPF_FUNC_probe_read),
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr = "R1 min value is negative, either use unsigned index or do a if (index >=0) check",
+ .errstr = "R1 unbounded memory access",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -4329,7 +4328,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result = ACCEPT,
.result_unpriv = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -4357,7 +4356,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result = ACCEPT,
.result_unpriv = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -4376,7 +4375,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 bitwise operator &= on pointer",
.errstr = "invalid mem access 'inv'",
.result = REJECT,
.result_unpriv = REJECT,
@@ -4395,7 +4394,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 32-bit pointer arithmetic prohibited",
.errstr = "invalid mem access 'inv'",
.result = REJECT,
.result_unpriv = REJECT,
@@ -4414,7 +4413,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 pointer arithmetic with /= operator",
.errstr = "invalid mem access 'inv'",
.result = REJECT,
.result_unpriv = REJECT,
@@ -4457,10 +4456,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 invalid mem access 'inv'",
.errstr = "R0 invalid mem access 'inv'",
.result = REJECT,
- .result_unpriv = REJECT,
},
{
"map element value is preserved across register spilling",
@@ -4482,7 +4479,7 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
+ .errstr_unpriv = "R0 leaks addr",
.result = ACCEPT,
.result_unpriv = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
@@ -4664,7 +4661,8 @@ static struct bpf_test tests[] = {
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN(),
},
- .errstr = "R2 unbounded memory access",
+ /* because max wasn't checked, signed min is negative */
+ .errstr = "R2 min value is negative, either use unsigned or 'var &= const'",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
@@ -4720,7 +4718,7 @@ static struct bpf_test tests[] = {
BPF_JMP_IMM(BPF_JSGT, BPF_REG_2,
sizeof(struct test_val), 4),
BPF_MOV64_IMM(BPF_REG_4, 0),
- BPF_JMP_REG(BPF_JGE, BPF_REG_4, BPF_REG_2, 2),
+ BPF_JMP_REG(BPF_JSGE, BPF_REG_4, BPF_REG_2, 2),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_EMIT_CALL(BPF_FUNC_probe_read),
BPF_MOV64_IMM(BPF_REG_0, 0),
@@ -4746,7 +4744,7 @@ static struct bpf_test tests[] = {
BPF_JMP_IMM(BPF_JSGT, BPF_REG_2,
sizeof(struct test_val) + 1, 4),
BPF_MOV64_IMM(BPF_REG_4, 0),
- BPF_JMP_REG(BPF_JGE, BPF_REG_4, BPF_REG_2, 2),
+ BPF_JMP_REG(BPF_JSGE, BPF_REG_4, BPF_REG_2, 2),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_EMIT_CALL(BPF_FUNC_probe_read),
BPF_MOV64_IMM(BPF_REG_0, 0),
@@ -4774,7 +4772,7 @@ static struct bpf_test tests[] = {
BPF_JMP_IMM(BPF_JSGT, BPF_REG_2,
sizeof(struct test_val) - 20, 4),
BPF_MOV64_IMM(BPF_REG_4, 0),
- BPF_JMP_REG(BPF_JGE, BPF_REG_4, BPF_REG_2, 2),
+ BPF_JMP_REG(BPF_JSGE, BPF_REG_4, BPF_REG_2, 2),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_EMIT_CALL(BPF_FUNC_probe_read),
BPF_MOV64_IMM(BPF_REG_0, 0),
@@ -4801,7 +4799,7 @@ static struct bpf_test tests[] = {
BPF_JMP_IMM(BPF_JSGT, BPF_REG_2,
sizeof(struct test_val) - 19, 4),
BPF_MOV64_IMM(BPF_REG_4, 0),
- BPF_JMP_REG(BPF_JGE, BPF_REG_4, BPF_REG_2, 2),
+ BPF_JMP_REG(BPF_JSGE, BPF_REG_4, BPF_REG_2, 2),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_EMIT_CALL(BPF_FUNC_probe_read),
BPF_MOV64_IMM(BPF_REG_0, 0),
@@ -4813,6 +4811,20 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
},
{
+ "helper access to variable memory: size = 0 allowed on NULL",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_1, 0),
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_MOV64_IMM(BPF_REG_3, 0),
+ BPF_MOV64_IMM(BPF_REG_4, 0),
+ BPF_MOV64_IMM(BPF_REG_5, 0),
+ BPF_EMIT_CALL(BPF_FUNC_csum_diff),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
"helper access to variable memory: size > 0 not allowed on NULL",
.insns = {
BPF_MOV64_IMM(BPF_REG_1, 0),
@@ -4826,7 +4838,7 @@ static struct bpf_test tests[] = {
BPF_EMIT_CALL(BPF_FUNC_csum_diff),
BPF_EXIT_INSN(),
},
- .errstr = "R1 type=imm expected=fp",
+ .errstr = "R1 type=inv expected=fp",
.result = REJECT,
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
@@ -4911,7 +4923,7 @@ static struct bpf_test tests[] = {
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
- BPF_MOV64_IMM(BPF_REG_1, 6),
+ BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
@@ -4920,10 +4932,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
- .errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
+ .errstr = "R0 max value is outside of the array range",
.result = REJECT,
- .result_unpriv = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
{
@@ -4952,10 +4962,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map2 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
- .errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
+ .errstr = "R0 max value is outside of the array range",
.result = REJECT,
- .result_unpriv = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
{
@@ -5002,7 +5010,7 @@ static struct bpf_test tests[] = {
},
.fixup_map_in_map = { 3 },
.errstr = "R1 type=inv expected=map_ptr",
- .errstr_unpriv = "R1 pointer arithmetic prohibited",
+ .errstr_unpriv = "R1 pointer arithmetic on CONST_PTR_TO_MAP prohibited",
.result = REJECT,
},
{
@@ -5190,10 +5198,8 @@ static struct bpf_test tests[] = {
BPF_EXIT_INSN(),
},
.fixup_map1 = { 3 },
- .errstr_unpriv = "R0 pointer arithmetic prohibited",
.errstr = "R0 min value is negative, either use unsigned index or do a if (index >=0) check.",
.result = REJECT,
- .result_unpriv = REJECT,
},
};
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 04:50 +0200 |
| Subject | Re: [RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations |
| Message-ID | <tPVGh-6Kh-5@gated-at.bofh.it> |
| In reply to | #1659909 |
On Wed, Jun 07, 2017 at 04:00:02PM +0100, Edward Cree wrote: > Some of the verifier's error messages have changed, and some constructs > that previously couldn't be verified are now accepted. > > Signed-off-by: Edward Cree <ecree@solarflare.com> > --- > tools/testing/selftests/bpf/test_verifier.c | 226 ++++++++++++++-------------- > 1 file changed, 116 insertions(+), 110 deletions(-) imo this rewrite needs more than one additional test. Like i counted at least 2 new verifier features (like negative and ptr & 0x40) All the new logic needs to be covered by tests.
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-08 17:30 +0200 |
| Subject | Re: [RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations |
| Message-ID | <tQ7xM-60p-17@gated-at.bofh.it> |
| In reply to | #1660681 |
On 08/06/17 03:43, Alexei Starovoitov wrote: > On Wed, Jun 07, 2017 at 04:00:02PM +0100, Edward Cree wrote: >> Some of the verifier's error messages have changed, and some constructs >> that previously couldn't be verified are now accepted. >> >> Signed-off-by: Edward Cree <ecree@solarflare.com> >> --- >> tools/testing/selftests/bpf/test_verifier.c | 226 ++++++++++++++-------------- >> 1 file changed, 116 insertions(+), 110 deletions(-) > imo this rewrite needs more than one additional test. > Like i counted at least 2 new verifier features (like negative and ptr & 0x40) > All the new logic needs to be covered by tests. Yes, I will write some new tests to cover the new features. I just wanted to get some comments on the patch first, in case I was barking up entirely the wrong tree. -Ed
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-07 17:10 +0200 |
| Subject | [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tPKKT-8hH-33@gated-at.bofh.it> |
| In reply to | #1659883 |
If pointer leaks are allowed, and adjust_ptr_min_max_vals returns -EACCES,
treat the pointer as an unknown scalar and try again, because we might be
able to conclude something about the result (e.g. pointer & 0x40 is either
0 or 0x40).
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
kernel/bpf/verifier.c | 244 ++++++++++++++++++++++++++------------------------
1 file changed, 127 insertions(+), 117 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index dd06e4e..1ff5b5d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1566,6 +1566,8 @@ static void coerce_reg_to_32(struct bpf_reg_state *reg)
/* Handles arithmetic on a pointer and a scalar: computes new min/max and align.
* Caller must check_reg_overflow all argument regs beforehand.
* Caller should also handle BPF_MOV case separately.
+ * If we return -EACCES, caller may want to try again treating pointer as a
+ * scalar. So we only emit a diagnostic if !env->allow_ptr_leaks.
*/
static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
struct bpf_insn *insn,
@@ -1588,43 +1590,29 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
if (BPF_CLASS(insn->code) != BPF_ALU64) {
/* 32-bit ALU ops on pointers produce (meaningless) scalars */
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d 32-bit pointer arithmetic prohibited\n",
dst);
- return -EACCES;
- }
- __mark_reg_unknown(dst_reg);
- /* High bits are known zero */
- dst_reg->align.mask = (u32)-1;
- return 0;
+ return -EACCES;
}
if (ptr_reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
dst);
- return -EACCES;
- }
- __mark_reg_unknown(dst_reg);
- return 0;
+ return -EACCES;
}
if (ptr_reg->type == CONST_PTR_TO_MAP) {
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d pointer arithmetic on CONST_PTR_TO_MAP prohibited\n",
dst);
- return -EACCES;
- }
- __mark_reg_unknown(dst_reg);
- return 0;
+ return -EACCES;
}
if (ptr_reg->type == PTR_TO_PACKET_END) {
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d pointer arithmetic on PTR_TO_PACKET_END prohibited\n",
dst);
- return -EACCES;
- }
- __mark_reg_unknown(dst_reg);
- return 0;
+ return -EACCES;
}
/* In case of 'scalar += pointer', dst_reg inherits pointer type and id.
@@ -1648,8 +1636,9 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
break;
}
if (max_val == BPF_REGISTER_MAX_RANGE) {
- verbose("R%d tried to add unbounded value to pointer\n",
- dst);
+ if (!env->allow_ptr_leaks)
+ verbose("R%d tried to add unbounded value to pointer\n",
+ dst);
return -EACCES;
}
/* A new variable offset is created. Note that off_reg->off
@@ -1676,28 +1665,20 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
case BPF_SUB:
if (dst_reg == off_reg) {
/* scalar -= pointer. Creates an unknown scalar */
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d tried to subtract pointer from scalar\n",
dst);
- return -EACCES;
- }
- /* Make it an unknown scalar */
- __mark_reg_unknown(dst_reg);
- break;
+ return -EACCES;
}
/* We don't allow subtraction from FP, because (according to
* test_verifier.c test "invalid fp arithmetic", JITs might not
* be able to deal with it.
*/
if (ptr_reg->type == PTR_TO_STACK) {
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d subtraction from stack pointer prohibited\n",
dst);
- return -EACCES;
- }
- /* Make it an unknown scalar */
- __mark_reg_unknown(dst_reg);
- break;
+ return -EACCES;
}
if (known && (ptr_reg->off - min_val ==
(s64)(s32)(ptr_reg->off - min_val))) {
@@ -1713,14 +1694,10 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
* This can happen if off_reg is an immediate.
*/
if ((s64)max_val < 0) {
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d tried to subtract negative max_val %lld from pointer\n",
dst, (s64)max_val);
- return -EACCES;
- }
- /* Make it an unknown scalar */
- __mark_reg_unknown(dst_reg);
- break;
+ return -EACCES;
}
/* A new variable offset is created. If the subtrahend is known
* nonnegative, then any reg->range we had before is still good.
@@ -1747,99 +1724,37 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
* (However, in principle we could allow some cases, e.g.
* ptr &= ~3 which would reduce min_value by 3.)
*/
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d bitwise operator %s on pointer prohibited\n",
dst, bpf_alu_string[opcode >> 4]);
- return -EACCES;
- }
- /* Make it an unknown scalar */
- __mark_reg_unknown(dst_reg);
+ return -EACCES;
default:
/* other operators (e.g. MUL,LSH) produce non-pointer results */
- if (!env->allow_ptr_leaks) {
+ if (!env->allow_ptr_leaks)
verbose("R%d pointer arithmetic with %s operator prohibited\n",
dst, bpf_alu_string[opcode >> 4]);
- return -EACCES;
- }
- /* Make it an unknown scalar */
- __mark_reg_unknown(dst_reg);
+ return -EACCES;
}
check_reg_overflow(dst_reg);
return 0;
}
-/* Handles ALU ops other than BPF_END, BPF_NEG and BPF_MOV: computes new min/max
- * and align.
- * TODO: check this is legit for ALU32, particularly around negatives
- */
-static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
- struct bpf_insn *insn)
+static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
+ struct bpf_insn *insn,
+ struct bpf_reg_state *dst_reg,
+ struct bpf_reg_state *src_reg)
{
- struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg, *src_reg;
- struct bpf_reg_state *ptr_reg = NULL, off_reg = {0};
+ struct bpf_reg_state *regs = env->cur_state.regs;
s64 min_val = BPF_REGISTER_MIN_RANGE;
u64 max_val = BPF_REGISTER_MAX_RANGE;
u8 opcode = BPF_OP(insn->code);
bool src_known, dst_known;
- dst_reg = ®s[insn->dst_reg];
- check_reg_overflow(dst_reg);
- src_reg = NULL;
- if (dst_reg->type != SCALAR_VALUE)
- ptr_reg = dst_reg;
- if (BPF_SRC(insn->code) == BPF_X) {
- src_reg = ®s[insn->src_reg];
- check_reg_overflow(src_reg);
-
- if (src_reg->type != SCALAR_VALUE) {
- if (dst_reg->type != SCALAR_VALUE) {
- /* Combining two pointers by any ALU op yields
- * an arbitrary scalar.
- */
- if (!env->allow_ptr_leaks) {
- verbose("R%d pointer %s pointer prohibited\n",
- insn->dst_reg,
- bpf_alu_string[opcode >> 4]);
- return -EACCES;
- }
- mark_reg_unknown(regs, insn->dst_reg);
- return 0;
- } else {
- /* scalar += pointer
- * This is legal, but we have to reverse our
- * src/dest handling in computing the range
- */
- return adjust_ptr_min_max_vals(env, insn,
- src_reg, dst_reg);
- }
- } else if (ptr_reg) {
- /* pointer += scalar */
- return adjust_ptr_min_max_vals(env, insn,
- dst_reg, src_reg);
- }
- } else {
- /* Pretend the src is a reg with a known value, since we only
- * need to be able to read from this state.
- */
- off_reg.type = SCALAR_VALUE;
- off_reg.align = tn_const(insn->imm);
- off_reg.min_value = insn->imm;
- off_reg.max_value = insn->imm;
- src_reg = &off_reg;
- if (ptr_reg) /* pointer += K */
- return adjust_ptr_min_max_vals(env, insn,
- ptr_reg, src_reg);
- }
-
- /* Got here implies adding two SCALAR_VALUEs */
- if (WARN_ON_ONCE(ptr_reg)) {
- verbose("verifier internal error\n");
- return -EINVAL;
- }
- if (WARN_ON(!src_reg)) {
- verbose("verifier internal error\n");
- return -EINVAL;
+ if (BPF_CLASS(insn->code) != BPF_ALU64) {
+ /* 32-bit ALU ops are (32,32)->64 */
+ coerce_reg_to_32(dst_reg);
+ coerce_reg_to_32(src_reg);
}
if (BPF_CLASS(insn->code) != BPF_ALU64) {
/* 32-bit ALU ops are (32,32)->64 */
@@ -1990,6 +1905,101 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
return 0;
}
+/* Handles ALU ops other than BPF_END, BPF_NEG and BPF_MOV: computes new min/max
+ * and align.
+ */
+static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
+ struct bpf_insn *insn)
+{
+ struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg, *src_reg;
+ struct bpf_reg_state *ptr_reg = NULL, off_reg = {0};
+ u8 opcode = BPF_OP(insn->code);
+ int rc;
+
+ dst_reg = ®s[insn->dst_reg];
+ check_reg_overflow(dst_reg);
+ src_reg = NULL;
+ if (dst_reg->type != SCALAR_VALUE)
+ ptr_reg = dst_reg;
+ if (BPF_SRC(insn->code) == BPF_X) {
+ src_reg = ®s[insn->src_reg];
+ check_reg_overflow(src_reg);
+
+ if (src_reg->type != SCALAR_VALUE) {
+ if (dst_reg->type != SCALAR_VALUE) {
+ /* Combining two pointers by any ALU op yields
+ * an arbitrary scalar.
+ */
+ if (!env->allow_ptr_leaks) {
+ verbose("R%d pointer %s pointer prohibited\n",
+ insn->dst_reg,
+ bpf_alu_string[opcode >> 4]);
+ return -EACCES;
+ }
+ mark_reg_unknown(regs, insn->dst_reg);
+ return 0;
+ } else {
+ /* scalar += pointer
+ * This is legal, but we have to reverse our
+ * src/dest handling in computing the range
+ */
+ rc = adjust_ptr_min_max_vals(env, insn,
+ src_reg, dst_reg);
+ if (rc == -EACCES && env->allow_ptr_leaks) {
+ /* scalar += unknown scalar */
+ __mark_reg_unknown(&off_reg);
+ return adjust_scalar_min_max_vals(
+ env, insn,
+ dst_reg, &off_reg);
+ }
+ return rc;
+ }
+ } else if (ptr_reg) {
+ /* pointer += scalar */
+ rc = adjust_ptr_min_max_vals(env, insn,
+ dst_reg, src_reg);
+ if (rc == -EACCES && env->allow_ptr_leaks) {
+ /* unknown scalar += scalar */
+ __mark_reg_unknown(dst_reg);
+ return adjust_scalar_min_max_vals(
+ env, insn, dst_reg, src_reg);
+ }
+ return rc;
+ }
+ } else {
+ /* Pretend the src is a reg with a known value, since we only
+ * need to be able to read from this state.
+ */
+ off_reg.type = SCALAR_VALUE;
+ off_reg.align = tn_const(insn->imm);
+ off_reg.min_value = insn->imm;
+ off_reg.max_value = insn->imm;
+ src_reg = &off_reg;
+ if (ptr_reg) { /* pointer += K */
+ rc = adjust_ptr_min_max_vals(env, insn,
+ ptr_reg, src_reg);
+ if (rc == -EACCES && env->allow_ptr_leaks) {
+ /* unknown scalar += K */
+ __mark_reg_unknown(dst_reg);
+ return adjust_scalar_min_max_vals(
+ env, insn, dst_reg, &off_reg);
+ }
+ return rc;
+ }
+ }
+
+ /* Got here implies adding two SCALAR_VALUEs */
+ if (WARN_ON_ONCE(ptr_reg)) {
+ verbose("verifier internal error\n");
+ return -EINVAL;
+ }
+ if (WARN_ON(!src_reg)) {
+ verbose("verifier internal error\n");
+ return -EINVAL;
+ }
+ return adjust_scalar_min_max_vals(env, insn, dst_reg, src_reg);
+}
+
/* check validity of 32-bit and 64-bit arithmetic operations */
static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
{
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 04:40 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tPVwC-6GZ-17@gated-at.bofh.it> |
| In reply to | #1659918 |
On Wed, Jun 07, 2017 at 03:58:50PM +0100, Edward Cree wrote:
> If pointer leaks are allowed, and adjust_ptr_min_max_vals returns -EACCES,
> treat the pointer as an unknown scalar and try again, because we might be
> able to conclude something about the result (e.g. pointer & 0x40 is either
> 0 or 0x40).
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> kernel/bpf/verifier.c | 244 ++++++++++++++++++++++++++------------------------
> 1 file changed, 127 insertions(+), 117 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index dd06e4e..1ff5b5d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1566,6 +1566,8 @@ static void coerce_reg_to_32(struct bpf_reg_state *reg)
> /* Handles arithmetic on a pointer and a scalar: computes new min/max and align.
> * Caller must check_reg_overflow all argument regs beforehand.
> * Caller should also handle BPF_MOV case separately.
> + * If we return -EACCES, caller may want to try again treating pointer as a
> + * scalar. So we only emit a diagnostic if !env->allow_ptr_leaks.
> */
> static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> struct bpf_insn *insn,
> @@ -1588,43 +1590,29 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
>
> if (BPF_CLASS(insn->code) != BPF_ALU64) {
> /* 32-bit ALU ops on pointers produce (meaningless) scalars */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d 32-bit pointer arithmetic prohibited\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - /* High bits are known zero */
> - dst_reg->align.mask = (u32)-1;
> - return 0;
> + return -EACCES;
> }
>
> if (ptr_reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - return 0;
> + return -EACCES;
> }
> if (ptr_reg->type == CONST_PTR_TO_MAP) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic on CONST_PTR_TO_MAP prohibited\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - return 0;
> + return -EACCES;
> }
> if (ptr_reg->type == PTR_TO_PACKET_END) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic on PTR_TO_PACKET_END prohibited\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - return 0;
> + return -EACCES;
> }
>
> /* In case of 'scalar += pointer', dst_reg inherits pointer type and id.
> @@ -1648,8 +1636,9 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> break;
> }
> if (max_val == BPF_REGISTER_MAX_RANGE) {
> - verbose("R%d tried to add unbounded value to pointer\n",
> - dst);
> + if (!env->allow_ptr_leaks)
> + verbose("R%d tried to add unbounded value to pointer\n",
> + dst);
> return -EACCES;
> }
> /* A new variable offset is created. Note that off_reg->off
> @@ -1676,28 +1665,20 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> case BPF_SUB:
> if (dst_reg == off_reg) {
> /* scalar -= pointer. Creates an unknown scalar */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d tried to subtract pointer from scalar\n",
> dst);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> - break;
> + return -EACCES;
> }
> /* We don't allow subtraction from FP, because (according to
> * test_verifier.c test "invalid fp arithmetic", JITs might not
> * be able to deal with it.
> */
> if (ptr_reg->type == PTR_TO_STACK) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d subtraction from stack pointer prohibited\n",
> dst);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> - break;
> + return -EACCES;
> }
> if (known && (ptr_reg->off - min_val ==
> (s64)(s32)(ptr_reg->off - min_val))) {
> @@ -1713,14 +1694,10 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> * This can happen if off_reg is an immediate.
> */
> if ((s64)max_val < 0) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d tried to subtract negative max_val %lld from pointer\n",
> dst, (s64)max_val);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> - break;
> + return -EACCES;
> }
> /* A new variable offset is created. If the subtrahend is known
> * nonnegative, then any reg->range we had before is still good.
> @@ -1747,99 +1724,37 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> * (However, in principle we could allow some cases, e.g.
> * ptr &= ~3 which would reduce min_value by 3.)
> */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d bitwise operator %s on pointer prohibited\n",
> dst, bpf_alu_string[opcode >> 4]);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> + return -EACCES;
> default:
> /* other operators (e.g. MUL,LSH) produce non-pointer results */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic with %s operator prohibited\n",
> dst, bpf_alu_string[opcode >> 4]);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> + return -EACCES;
> }
>
> check_reg_overflow(dst_reg);
> return 0;
> }
>
> -/* Handles ALU ops other than BPF_END, BPF_NEG and BPF_MOV: computes new min/max
> - * and align.
> - * TODO: check this is legit for ALU32, particularly around negatives
> - */
> -static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
> - struct bpf_insn *insn)
> +static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
> + struct bpf_insn *insn,
> + struct bpf_reg_state *dst_reg,
> + struct bpf_reg_state *src_reg)
> {
> - struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg, *src_reg;
> - struct bpf_reg_state *ptr_reg = NULL, off_reg = {0};
> + struct bpf_reg_state *regs = env->cur_state.regs;
> s64 min_val = BPF_REGISTER_MIN_RANGE;
> u64 max_val = BPF_REGISTER_MAX_RANGE;
> u8 opcode = BPF_OP(insn->code);
> bool src_known, dst_known;
>
> - dst_reg = ®s[insn->dst_reg];
> - check_reg_overflow(dst_reg);
> - src_reg = NULL;
> - if (dst_reg->type != SCALAR_VALUE)
> - ptr_reg = dst_reg;
> - if (BPF_SRC(insn->code) == BPF_X) {
> - src_reg = ®s[insn->src_reg];
> - check_reg_overflow(src_reg);
> -
> - if (src_reg->type != SCALAR_VALUE) {
> - if (dst_reg->type != SCALAR_VALUE) {
> - /* Combining two pointers by any ALU op yields
> - * an arbitrary scalar.
> - */
> - if (!env->allow_ptr_leaks) {
> - verbose("R%d pointer %s pointer prohibited\n",
> - insn->dst_reg,
> - bpf_alu_string[opcode >> 4]);
> - return -EACCES;
> - }
> - mark_reg_unknown(regs, insn->dst_reg);
> - return 0;
> - } else {
> - /* scalar += pointer
> - * This is legal, but we have to reverse our
> - * src/dest handling in computing the range
> - */
> - return adjust_ptr_min_max_vals(env, insn,
> - src_reg, dst_reg);
> - }
> - } else if (ptr_reg) {
> - /* pointer += scalar */
> - return adjust_ptr_min_max_vals(env, insn,
> - dst_reg, src_reg);
> - }
> - } else {
> - /* Pretend the src is a reg with a known value, since we only
> - * need to be able to read from this state.
> - */
> - off_reg.type = SCALAR_VALUE;
> - off_reg.align = tn_const(insn->imm);
> - off_reg.min_value = insn->imm;
> - off_reg.max_value = insn->imm;
> - src_reg = &off_reg;
> - if (ptr_reg) /* pointer += K */
> - return adjust_ptr_min_max_vals(env, insn,
> - ptr_reg, src_reg);
> - }
> -
> - /* Got here implies adding two SCALAR_VALUEs */
> - if (WARN_ON_ONCE(ptr_reg)) {
> - verbose("verifier internal error\n");
> - return -EINVAL;
> - }
> - if (WARN_ON(!src_reg)) {
> - verbose("verifier internal error\n");
> - return -EINVAL;
such large back and forth move doesn't help reviewing.
may be just merge it into previous patch?
Or keep that function in the right place in patch 2 already?
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-08 17:30 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tQ7xN-60p-39@gated-at.bofh.it> |
| In reply to | #1660678 |
On 08/06/17 03:35, Alexei Starovoitov wrote: > such large back and forth move doesn't help reviewing. > may be just merge it into previous patch? > Or keep that function in the right place in patch 2 already? I think 'diff' got a bit confused, and maybe with different options I could have got it to produce something more readable. But I think I will just merge this into patch 2; it's only separate because it started out as an experiment. -Ed
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 19:00 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tQ8WS-6KR-15@gated-at.bofh.it> |
| In reply to | #1661389 |
On Thu, Jun 08, 2017 at 04:25:39PM +0100, Edward Cree wrote: > On 08/06/17 03:35, Alexei Starovoitov wrote: > > such large back and forth move doesn't help reviewing. > > may be just merge it into previous patch? > > Or keep that function in the right place in patch 2 already? > I think 'diff' got a bit confused, and maybe with different options I could > have got it to produce something more readable. But I think I will just > merge this into patch 2; it's only separate because it started out as an > experiment. after sleeping on it I'm not sure we should be allowing such pointer arithmetic. In normal C code people do fancy tricks with lower 3 bits of the pointer, but in bpf code I cannot see such use case. What kind of realistic code will be doing ptr & 0x40 ?
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-08 19:20 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tQ9ge-76r-11@gated-at.bofh.it> |
| In reply to | #1661493 |
On 08/06/17 17:50, Alexei Starovoitov wrote: > On Thu, Jun 08, 2017 at 04:25:39PM +0100, Edward Cree wrote: >> On 08/06/17 03:35, Alexei Starovoitov wrote: >>> such large back and forth move doesn't help reviewing. >>> may be just merge it into previous patch? >>> Or keep that function in the right place in patch 2 already? >> I think 'diff' got a bit confused, and maybe with different options I could >> have got it to produce something more readable. But I think I will just >> merge this into patch 2; it's only separate because it started out as an >> experiment. > after sleeping on it I'm not sure we should be allowing such pointer > arithmetic. In normal C code people do fancy tricks with lower 3 bits > of the pointer, but in bpf code I cannot see such use case. > What kind of realistic code will be doing ptr & 0x40 ? Well, I didn't support it because I saw a use case. I supported it because it seemed easy to do and the code came out reasonably elegant-looking. Since this is guarded by env->allow_ptr_leaks, I can't see any reason _not_ to let people try fancy tricks with the low bits of pointers. I agree ptr & 0x40 is a crazy thing with no imaginable use case, but... "Unix was not designed to stop its users from doing stupid things, as that would also stop them from doing clever things." ;-) -Ed
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 20:50 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tQaFk-7QA-15@gated-at.bofh.it> |
| In reply to | #1661504 |
On Thu, Jun 08, 2017 at 06:12:39PM +0100, Edward Cree wrote: > On 08/06/17 17:50, Alexei Starovoitov wrote: > > On Thu, Jun 08, 2017 at 04:25:39PM +0100, Edward Cree wrote: > >> On 08/06/17 03:35, Alexei Starovoitov wrote: > >>> such large back and forth move doesn't help reviewing. > >>> may be just merge it into previous patch? > >>> Or keep that function in the right place in patch 2 already? > >> I think 'diff' got a bit confused, and maybe with different options I could > >> have got it to produce something more readable. But I think I will just > >> merge this into patch 2; it's only separate because it started out as an > >> experiment. > > after sleeping on it I'm not sure we should be allowing such pointer > > arithmetic. In normal C code people do fancy tricks with lower 3 bits > > of the pointer, but in bpf code I cannot see such use case. > > What kind of realistic code will be doing ptr & 0x40 ? > Well, I didn't support it because I saw a use case. I supported it because > it seemed easy to do and the code came out reasonably elegant-looking. > Since this is guarded by env->allow_ptr_leaks, I can't see any reason _not_ > to let people try fancy tricks with the low bits of pointers. > I agree ptr & 0x40 is a crazy thing with no imaginable use case, but... > "Unix was not designed to stop its users from doing stupid things, as that > would also stop them from doing clever things." ;-) well, I agree with the philosophy :) but I also see few reasons not to allow it: 1. it immediately becomes uapi and if later we find out that it's preventing us to do something we actually really need we'll be stuck looking for workaround 2. it's the same pruning concern. probably doesn't fully apply here, but the reason we don't track 'if (reg == 1) ...' is if we mark that register as known const_imm in the true branch, it will screw up pruning quite badly. It's trivial to track and may seem useful, but hurts instead.
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-08 21:10 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tQaYH-8eD-39@gated-at.bofh.it> |
| In reply to | #1661579 |
On 08/06/17 19:41, Alexei Starovoitov wrote: > On Thu, Jun 08, 2017 at 06:12:39PM +0100, Edward Cree wrote: >> On 08/06/17 17:50, Alexei Starovoitov wrote: >>> On Thu, Jun 08, 2017 at 04:25:39PM +0100, Edward Cree wrote: >>>> On 08/06/17 03:35, Alexei Starovoitov wrote: >>>>> such large back and forth move doesn't help reviewing. >>>>> may be just merge it into previous patch? >>>>> Or keep that function in the right place in patch 2 already? >>>> I think 'diff' got a bit confused, and maybe with different options I could >>>> have got it to produce something more readable. But I think I will just >>>> merge this into patch 2; it's only separate because it started out as an >>>> experiment. >>> after sleeping on it I'm not sure we should be allowing such pointer >>> arithmetic. In normal C code people do fancy tricks with lower 3 bits >>> of the pointer, but in bpf code I cannot see such use case. >>> What kind of realistic code will be doing ptr & 0x40 ? >> Well, I didn't support it because I saw a use case. I supported it because >> it seemed easy to do and the code came out reasonably elegant-looking. >> Since this is guarded by env->allow_ptr_leaks, I can't see any reason _not_ >> to let people try fancy tricks with the low bits of pointers. >> I agree ptr & 0x40 is a crazy thing with no imaginable use case, but... >> "Unix was not designed to stop its users from doing stupid things, as that >> would also stop them from doing clever things." ;-) > well, I agree with the philosophy :) but I also see few reasons not to allow it: > 1. it immediately becomes uapi and if later we find out that it's preventing us > to do something we actually really need we'll be stuck looking for workaround What could it prevent us from doing, though? It's basically equivalent to giving BPF an opcode that casts a pointer to a u64, which of course is only allowed if allow_ptr_leaks is true. And since we don't feed any knowledge about the pointer into the verifier, it's just like any other way of filling a register with arbitrary, unknown bits. I can fully appreciate why you're being cautious, what with uapi and all. But I don't think there's any actual problem here. Open to being convinced, though. > 2. it's the same pruning concern. probably doesn't fully apply here, but > the reason we don't track 'if (reg == 1) ...' Don't we though? http://elixir.free-electrons.com/linux/v4.12-rc4/source/kernel/bpf/verifier.c#L2127 > is if we mark that > register as known const_imm in the true branch, it will screw up > pruning quite badly. It's trivial to track and may seem useful, > but hurts instead. (Thinking out loud...) What would be really nice is a way to propagate limits backwards as well as forwards, so that the verifier can say "when I tested this branch, I used this part of the state, I read four bytes past this pointer". Then when it wants to prune, it can say "well, the state this time isn't as strong, but it still satisfies everything I actually used". But that sounds like it would be very hard indeed to do. Maybe with the basic-block DAG stuff David's been talking about, we could find all the paths that reach a block, and take the union of their states, and then run through the block feeding it that combined state. But that could reject code that relies on correlation of the state (i.e. if r1 != 0 then r2 is valid ptr I can access, etc) so would still need the 'walk with each individual state' as a fallback. Though at least you'd have all the states at once so you could find out which ones were subsumed, instead of hoping you get to them in the right order. -Ed
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 23:20 +0200 |
| Subject | Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path |
| Message-ID | <tQd0u-11f-23@gated-at.bofh.it> |
| In reply to | #1661598 |
On Thu, Jun 08, 2017 at 08:07:53PM +0100, Edward Cree wrote:
> On 08/06/17 19:41, Alexei Starovoitov wrote:
> > On Thu, Jun 08, 2017 at 06:12:39PM +0100, Edward Cree wrote:
> >> On 08/06/17 17:50, Alexei Starovoitov wrote:
> >>> On Thu, Jun 08, 2017 at 04:25:39PM +0100, Edward Cree wrote:
> >>>> On 08/06/17 03:35, Alexei Starovoitov wrote:
> >>>>> such large back and forth move doesn't help reviewing.
> >>>>> may be just merge it into previous patch?
> >>>>> Or keep that function in the right place in patch 2 already?
> >>>> I think 'diff' got a bit confused, and maybe with different options I could
> >>>> have got it to produce something more readable. But I think I will just
> >>>> merge this into patch 2; it's only separate because it started out as an
> >>>> experiment.
> >>> after sleeping on it I'm not sure we should be allowing such pointer
> >>> arithmetic. In normal C code people do fancy tricks with lower 3 bits
> >>> of the pointer, but in bpf code I cannot see such use case.
> >>> What kind of realistic code will be doing ptr & 0x40 ?
> >> Well, I didn't support it because I saw a use case. I supported it because
> >> it seemed easy to do and the code came out reasonably elegant-looking.
> >> Since this is guarded by env->allow_ptr_leaks, I can't see any reason _not_
> >> to let people try fancy tricks with the low bits of pointers.
> >> I agree ptr & 0x40 is a crazy thing with no imaginable use case, but...
> >> "Unix was not designed to stop its users from doing stupid things, as that
> >> would also stop them from doing clever things." ;-)
> > well, I agree with the philosophy :) but I also see few reasons not to allow it:
> > 1. it immediately becomes uapi and if later we find out that it's preventing us
> > to do something we actually really need we'll be stuck looking for workaround
> What could it prevent us from doing, though? It's basically equivalent to giving
> BPF an opcode that casts a pointer to a u64, which of course is only allowed if
> allow_ptr_leaks is true. And since we don't feed any knowledge about the pointer
> into the verifier, it's just like any other way of filling a register with
> arbitrary, unknown bits.
> I can fully appreciate why you're being cautious, what with uapi and all. But I
> don't think there's any actual problem here. Open to being convinced, though.
The leaking is not a concern. It's if we started accepting a certain
class of programs we need to keep accepting them in the future.
Another reason is 'ptr & mask' could have been simply a bug and rejecting it
suppose to help users find issues sooner...
but I don't have a strong opinion here.
> > 2. it's the same pruning concern. probably doesn't fully apply here, but
> > the reason we don't track 'if (reg == 1) ...'
> Don't we though?
> http://elixir.free-electrons.com/linux/v4.12-rc4/source/kernel/bpf/verifier.c#L2127
> > is if we mark that
> > register as known const_imm in the true branch, it will screw up
> > pruning quite badly. It's trivial to track and may seem useful,
> > but hurts instead.
> (Thinking out loud...)
>
> What would be really nice is a way to propagate limits backwards as well as
> forwards, so that the verifier can say "when I tested this branch, I used
> this part of the state, I read four bytes past this pointer". Then when it
> wants to prune, it can say "well, the state this time isn't as strong, but
> it still satisfies everything I actually used".
> But that sounds like it would be very hard indeed to do.
that's more or less what i'm trying to do. liveness info per basic block
will trim the state.
> Maybe with the basic-block DAG stuff David's been talking about, we could
> find all the paths that reach a block, and take the union of their states,
> and then run through the block feeding it that combined state. But that
> could reject code that relies on correlation of the state (i.e. if r1 != 0
> then r2 is valid ptr I can access, etc) so would still need the 'walk with
> each individual state' as a fallback. Though at least you'd have all the
> states at once so you could find out which ones were subsumed, instead of
> hoping you get to them in the right order.
I think it's important to optimize verification speed for good programs.
If bad program takes slightly longer, not a big deal. Right now we have
global lock which needs to go away, but that's a minor fix.
In that sense I see that combining the state can help find bad programs
sooner, but I don't see it's helping good programs.
Also we already have programs like:
if (...) {
var1 = ptr
var2 = size
} else {
var1 = different ptr
var2 = different size
}
call_helper(...var1, var2)
So the state needs to be considered together. Cannot just mix and match.
Initially I was thinking to build Use/Def chains for all operands
of loads, stores and calls and follow them from Use spot to all Defs
recursively to determine validity, but above use case breaks that.
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 04:40 +0200 |
| Subject | Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking |
| Message-ID | <tPVwB-6GZ-1@gated-at.bofh.it> |
| In reply to | #1659883 |
On Wed, Jun 07, 2017 at 03:58:31PM +0100, Edward Cree wrote:
> Tracks value alignment by means of tracking known & unknown bits.
> Tightens some min/max value checks and fixes a couple of bugs therein.
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> include/linux/bpf.h | 34 +-
> include/linux/bpf_verifier.h | 40 +-
> include/linux/tnum.h | 58 ++
> kernel/bpf/Makefile | 2 +-
> kernel/bpf/tnum.c | 163 +++++
> kernel/bpf/verifier.c | 1641 +++++++++++++++++++++++-------------------
> 6 files changed, 1170 insertions(+), 768 deletions(-)
yeah! That's cool. Overall I like the direction.
I don't understand it completely yet, so ony few nits so far:
> +/* Arithmetic and logical ops */
> +/* Shift a tnum left (by a fixed shift) */
> +struct tnum tn_sl(struct tnum a, u8 shift);
> +/* Shift a tnum right (by a fixed shift) */
> +struct tnum tn_sr(struct tnum a, u8 shift);
I think in few month we will forget what these abbreviations mean.
Can you change it to tnum_rshift, tnum_lshift, tnum_add ?
> +/* half-multiply add: acc += (unknown * mask * value) */
> +static struct tnum hma(struct tnum acc, u64 value, u64 mask)
hma? is it a standard abbreviation?
> -static void init_reg_state(struct bpf_reg_state *regs)
> +static void mark_reg_known_zero(struct bpf_reg_state *regs, u32 regno)
> {
> - int i;
> -
> - for (i = 0; i < MAX_BPF_REG; i++)
> - mark_reg_not_init(regs, i);
> -
> - /* frame pointer */
> - regs[BPF_REG_FP].type = FRAME_PTR;
> -
> - /* 1st arg to a function */
> - regs[BPF_REG_1].type = PTR_TO_CTX;
> + BUG_ON(regno >= MAX_BPF_REG);
> + __mark_reg_known_zero(regs + regno);
I know we have BUG_ONs in the code and it was never hit,
but since you're rewriting it please change it to WARN_ON and
set all regs into NOT_INIT in such case.
This way if we really have a bug, it hopefully won't crash.
> -/* check read/write into an adjusted map element */
> -static int check_map_access_adj(struct bpf_verifier_env *env, u32 regno,
> +/* check read/write into a map element with possible variable offset */
> +static int check_map_access(struct bpf_verifier_env *env, u32 regno,
> int off, int size)
> {
> struct bpf_verifier_state *state = &env->cur_state;
> struct bpf_reg_state *reg = &state->regs[regno];
> int err;
>
> - /* We adjusted the register to this map value, so we
> - * need to change off and size to min_value and max_value
> - * respectively to make sure our theoretical access will be
> - * safe.
> + /* We may have adjusted the register to this map value, so we
> + * need to try adding each of min_value and max_value to off
> + * to make sure our theoretical access will be safe.
> */
> if (log_level)
> print_verifier_state(state);
> - env->varlen_map_value_access = true;
> + /* If the offset is variable, we will need to be stricter in state
> + * pruning from now on.
> + */
> + if (reg->align.mask)
> + env->varlen_map_value_access = true;
i think this align.mask access was used in few places.
May be worth to do static inline helper with clear name?
> switch (reg->type) {
> case PTR_TO_PACKET:
> + /* special case, because of NET_IP_ALIGN */
> return check_pkt_ptr_alignment(reg, off, size, strict);
> - case PTR_TO_MAP_VALUE_ADJ:
> - return check_val_ptr_alignment(reg, size, strict);
> + case PTR_TO_MAP_VALUE:
> + pointer_desc = "value ";
> + break;
> + case PTR_TO_CTX:
> + pointer_desc = "context ";
> + break;
> + case PTR_TO_STACK:
> + pointer_desc = "stack ";
> + break;
thank you for making errors more human readable.
> + char tn_buf[48];
> +
> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
> + verbose("variable ctx access align=%s off=%d size=%d",
> + tn_buf, off, size);
> + return -EACCES;
> + }
> + off += reg->align.value;
I think 'align' is an odd name for this field.
May be rename off/align fields into
s32 fixed_off;
struct tnum var_off;
>
> - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
> + } else if (reg->type == PTR_TO_STACK) {
> + /* stack accesses must be at a fixed offset, so that we can
> + * determine what type of data were returned.
> + */
> + if (reg->align.mask) {
> + char tn_buf[48];
> +
> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
> + verbose("variable stack access align=%s off=%d size=%d",
> + tn_buf, off, size);
> + return -EACCES;
hmm. why this restriction?
I thought one of key points of the diff that ptr+var tracking logic
will now apply not only to map_value, but to stack_ptr as well?
> }
>
> - if (!err && size <= 2 && value_regno >= 0 && env->allow_ptr_leaks &&
> - state->regs[value_regno].type == UNKNOWN_VALUE) {
> - /* 1 or 2 byte load zero-extends, determine the number of
> - * zero upper bits. Not doing it fo 4 byte load, since
> - * such values cannot be added to ptr_to_packet anyway.
> - */
> - state->regs[value_regno].imm = 64 - size * 8;
> + if (!err && size < BPF_REG_SIZE && value_regno >= 0 && t == BPF_READ &&
> + state->regs[value_regno].type == SCALAR_VALUE) {
> + /* b/h/w load zero-extends, mark upper bits as known 0 */
> + state->regs[value_regno].align.value &= (1ULL << (size * 8)) - 1;
> + state->regs[value_regno].align.mask &= (1ULL << (size * 8)) - 1;
probably another helper from tnum.h is needed.
> + /* sign bit is known zero, so we can bound the value */
> + state->regs[value_regno].min_value = 0;
> + state->regs[value_regno].max_value = min_t(u64,
> + state->regs[value_regno].align.mask,
> + BPF_REGISTER_MAX_RANGE);
min_t with mask? should it be align.value?
> }
> return err;
> }
> @@ -1000,9 +1068,18 @@ static int check_xadd(struct bpf_verifier_env *env, struct bpf_insn *insn)
> BPF_SIZE(insn->code), BPF_WRITE, -1);
> }
>
> +/* Does this register contain a constant zero? */
> +static bool register_is_null(struct bpf_reg_state reg)
> +{
> + return reg.type == SCALAR_VALUE && reg.align.mask == 0 &&
> + reg.align.value == 0;
align.mask == 0 && align.value==0 into helper in tnum.h ?
> @@ -1024,7 +1101,15 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
> return -EACCES;
> }
>
> - off = regs[regno].imm;
> + /* Only allow fixed-offset stack reads */
> + if (regs[regno].align.mask) {
> + char tn_buf[48];
> +
> + tn_strn(tn_buf, sizeof(tn_buf), regs[regno].align);
> + verbose("invalid variable stack read R%d align=%s\n",
> + regno, tn_buf);
> + }
same question as before. can it be relaxed?
The support for char arr[32]; accee arr[n] was requested several times
and folks used map_value[n] as a workaround.
Seems with this var stack logic it's one step away, no?
> - if (src_reg->imm < 48) {
> - verbose("cannot add integer value with %lld upper zero bits to ptr_to_packet\n",
> - src_reg->imm);
> - return -EACCES;
> - }
> -
> - had_id = (dst_reg->id != 0);
> -
> - /* dst_reg stays as pkt_ptr type and since some positive
> - * integer value was added to the pointer, increment its 'id'
> - */
> - dst_reg->id = ++env->id_gen;
great to see it's being generalized.
> + if (ptr_reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
> + if (!env->allow_ptr_leaks) {
> + verbose("R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
> + dst);
> + return -EACCES;
> + }
i guess mark_map_reg() logic will cover good cases and
actual math on ptr_to_map_or_null will happen only in broken programs.
just feels a bit fragile, since it probably depends on order we will
evaluate the branches? it's not an issue with this patch. we have
the same situation today. just thinking out loud.
> + /* Got here implies adding two SCALAR_VALUEs */
> + if (WARN_ON_ONCE(ptr_reg)) {
> + verbose("verifier internal error\n");
> + return -EINVAL;
...
> + if (WARN_ON(!src_reg)) {
> + verbose("verifier internal error\n");
> + return -EINVAL;
> }
i'm lost with these bits.
Can you add a comment in what circumstances this can be hit
and what would be the consequences?
> +/* Returns true if (rold safe implies rcur safe) */
> +static bool regsafe(struct bpf_reg_state *rold,
> + struct bpf_reg_state *rcur,
> + bool varlen_map_access)
> +{
> + if (memcmp(rold, rcur, sizeof(*rold)) == 0)
> return true;
> + if (rold->type == NOT_INIT)
> + /* explored state can't have used this */
> return true;
> + if (rcur->type == NOT_INIT)
> + return false;
> + switch (rold->type) {
> + case SCALAR_VALUE:
> + if (rcur->type == SCALAR_VALUE) {
> + /* new val must satisfy old val knowledge */
> + return range_within(rold, rcur) &&
> + tn_in(rold->align, rcur->align);
> + } else {
> + /* if we knew anything about the old value, we're not
> + * equal, because we can't know anything about the
> + * scalar value of the pointer in the new value.
> + */
> + return rold->min_value == BPF_REGISTER_MIN_RANGE &&
> + rold->max_value == BPF_REGISTER_MAX_RANGE &&
> + !~rold->align.mask;
> + }
> + case PTR_TO_MAP_VALUE:
> + if (varlen_map_access) {
> + /* If the new min/max/align satisfy the old ones and
> + * everything else matches, we are OK.
> + * We don't care about the 'id' value, because nothing
> + * uses it for PTR_TO_MAP_VALUE (only for ..._OR_NULL)
> + */
> + return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
> + range_within(rold, rcur) &&
> + tn_in(rold->align, rcur->align);
> + } else {
> + /* If the ranges/align were not the same, but
> + * everything else was and we didn't do a variable
> + * access into a map then we are a-ok.
> + */
> + return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0;
> + }
> + case PTR_TO_MAP_VALUE_OR_NULL:
does this new state comparison logic helps?
Do you have any numbers before/after in the number of insns it had to process
for the tests in selftests ?
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-08 17:00 +0200 |
| Subject | Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking |
| Message-ID | <tQ74K-5AT-13@gated-at.bofh.it> |
| In reply to | #1660673 |
On 08/06/17 03:32, Alexei Starovoitov wrote:
> On Wed, Jun 07, 2017 at 03:58:31PM +0100, Edward Cree wrote:
>> +/* Arithmetic and logical ops */
>> +/* Shift a tnum left (by a fixed shift) */
>> +struct tnum tn_sl(struct tnum a, u8 shift);
>> +/* Shift a tnum right (by a fixed shift) */
>> +struct tnum tn_sr(struct tnum a, u8 shift);
> I think in few month we will forget what these abbreviations mean.
> Can you change it to tnum_rshift, tnum_lshift, tnum_add ?
Sure, will do.
>> +/* half-multiply add: acc += (unknown * mask * value) */
>> +static struct tnum hma(struct tnum acc, u64 value, u64 mask)
> hma? is it a standard abbreviation?
No, just a weird operation that appears in my multiply algorithm. Since
it's static I didn't worry too much about naming it well.
(The abbreviation was inspired by floating point 'fma', fused multiply-add.)
>> -static void init_reg_state(struct bpf_reg_state *regs)
>> +static void mark_reg_known_zero(struct bpf_reg_state *regs, u32 regno)
>> {
>> - int i;
>> -
>> - for (i = 0; i < MAX_BPF_REG; i++)
>> - mark_reg_not_init(regs, i);
>> -
>> - /* frame pointer */
>> - regs[BPF_REG_FP].type = FRAME_PTR;
>> -
>> - /* 1st arg to a function */
>> - regs[BPF_REG_1].type = PTR_TO_CTX;
>> + BUG_ON(regno >= MAX_BPF_REG);
>> + __mark_reg_known_zero(regs + regno);
> I know we have BUG_ONs in the code and it was never hit,
> but since you're rewriting it please change it to WARN_ON and
> set all regs into NOT_INIT in such case.
> This way if we really have a bug, it hopefully won't crash.
Sure, will do.
>> -/* check read/write into an adjusted map element */
>> -static int check_map_access_adj(struct bpf_verifier_env *env, u32 regno,
>> +/* check read/write into a map element with possible variable offset */
>> +static int check_map_access(struct bpf_verifier_env *env, u32 regno,
>> int off, int size)
>> {
>> struct bpf_verifier_state *state = &env->cur_state;
>> struct bpf_reg_state *reg = &state->regs[regno];
>> int err;
>>
>> - /* We adjusted the register to this map value, so we
>> - * need to change off and size to min_value and max_value
>> - * respectively to make sure our theoretical access will be
>> - * safe.
>> + /* We may have adjusted the register to this map value, so we
>> + * need to try adding each of min_value and max_value to off
>> + * to make sure our theoretical access will be safe.
>> */
>> if (log_level)
>> print_verifier_state(state);
>> - env->varlen_map_value_access = true;
>> + /* If the offset is variable, we will need to be stricter in state
>> + * pruning from now on.
>> + */
>> + if (reg->align.mask)
>> + env->varlen_map_value_access = true;
> i think this align.mask access was used in few places.
> May be worth to do static inline helper with clear name?
Sure, seems reasonable.
>> + char tn_buf[48];
>> +
>> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
>> + verbose("variable ctx access align=%s off=%d size=%d",
>> + tn_buf, off, size);
>> + return -EACCES;
>> + }
>> + off += reg->align.value;
> I think 'align' is an odd name for this field.
> May be rename off/align fields into
> s32 fixed_off;
> struct tnum var_off;
Yeah, it got that name for 'historical' reasons i.e. this patch series
started out as just a rewrite of the alignment tracking, then grew...
I'll do the rename in the next version.
>>
>> - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
>> + } else if (reg->type == PTR_TO_STACK) {
>> + /* stack accesses must be at a fixed offset, so that we can
>> + * determine what type of data were returned.
>> + */
>> + if (reg->align.mask) {
>> + char tn_buf[48];
>> +
>> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
>> + verbose("variable stack access align=%s off=%d size=%d",
>> + tn_buf, off, size);
>> + return -EACCES;
> hmm. why this restriction?
> I thought one of key points of the diff that ptr+var tracking logic
> will now apply not only to map_value, but to stack_ptr as well?
As the comment above it says, we need to determine what was returned:
was it STACK_MISC or STACK_SPILL, and if the latter, what kind of pointer
was spilled there? See check_stack_read(), which I should probably
mention in the comment.
>> }
>>
>> - if (!err && size <= 2 && value_regno >= 0 && env->allow_ptr_leaks &&
>> - state->regs[value_regno].type == UNKNOWN_VALUE) {
>> - /* 1 or 2 byte load zero-extends, determine the number of
>> - * zero upper bits. Not doing it fo 4 byte load, since
>> - * such values cannot be added to ptr_to_packet anyway.
>> - */
>> - state->regs[value_regno].imm = 64 - size * 8;
>> + if (!err && size < BPF_REG_SIZE && value_regno >= 0 && t == BPF_READ &&
>> + state->regs[value_regno].type == SCALAR_VALUE) {
>> + /* b/h/w load zero-extends, mark upper bits as known 0 */
>> + state->regs[value_regno].align.value &= (1ULL << (size * 8)) - 1;
>> + state->regs[value_regno].align.mask &= (1ULL << (size * 8)) - 1;
> probably another helper from tnum.h is needed.
I could rewrite as
reg->align = tn_and(reg->align, tn_const((1ULL << (size * 8)) - 1))
or do you mean a helper that takes 'size' as an argument?
>> + /* sign bit is known zero, so we can bound the value */
>> + state->regs[value_regno].min_value = 0;
>> + state->regs[value_regno].max_value = min_t(u64,
>> + state->regs[value_regno].align.mask,
>> + BPF_REGISTER_MAX_RANGE);
> min_t with mask? should it be align.value?
Hmm, I think actually it should be (mask | value), because this is the
max (we're taking the min of two maxes to see which is tighter).
>> }
>> return err;
>> }
>> @@ -1000,9 +1068,18 @@ static int check_xadd(struct bpf_verifier_env *env, struct bpf_insn *insn)
>> BPF_SIZE(insn->code), BPF_WRITE, -1);
>> }
>>
>> +/* Does this register contain a constant zero? */
>> +static bool register_is_null(struct bpf_reg_state reg)
>> +{
>> + return reg.type == SCALAR_VALUE && reg.align.mask == 0 &&
>> + reg.align.value == 0;
> align.mask == 0 && align.value==0 into helper in tnum.h ?
Could do, but it seems unnecessary; I don't think anything but this
function would use it.
>> + /* Got here implies adding two SCALAR_VALUEs */
>> + if (WARN_ON_ONCE(ptr_reg)) {
>> + verbose("verifier internal error\n");
>> + return -EINVAL;
> ...
>> + if (WARN_ON(!src_reg)) {
>> + verbose("verifier internal error\n");
>> + return -EINVAL;
>> }
> i'm lost with these bits.
> Can you add a comment in what circumstances this can be hit
> and what would be the consequences?
It should be impossible to hit either of these cases. If we let the
first through, we'd probably do invalid pointer arithmetic (e.g. we
could multiply a pointer by two and think we'd just multiplied the
variable offset). As for the latter, we access through that pointer
so if it were NULL we would promptly oops.
>> +/* Returns true if (rold safe implies rcur safe) */
>> +static bool regsafe(struct bpf_reg_state *rold,
>> + struct bpf_reg_state *rcur,
>> + bool varlen_map_access)
>> +{
>> + if (memcmp(rold, rcur, sizeof(*rold)) == 0)
>> return true;
>> + if (rold->type == NOT_INIT)
>> + /* explored state can't have used this */
>> return true;
>> + if (rcur->type == NOT_INIT)
>> + return false;
>> + switch (rold->type) {
>> + case SCALAR_VALUE:
>> + if (rcur->type == SCALAR_VALUE) {
>> + /* new val must satisfy old val knowledge */
>> + return range_within(rold, rcur) &&
>> + tn_in(rold->align, rcur->align);
>> + } else {
>> + /* if we knew anything about the old value, we're not
>> + * equal, because we can't know anything about the
>> + * scalar value of the pointer in the new value.
>> + */
>> + return rold->min_value == BPF_REGISTER_MIN_RANGE &&
>> + rold->max_value == BPF_REGISTER_MAX_RANGE &&
>> + !~rold->align.mask;
>> + }
>> + case PTR_TO_MAP_VALUE:
>> + if (varlen_map_access) {
>> + /* If the new min/max/align satisfy the old ones and
>> + * everything else matches, we are OK.
>> + * We don't care about the 'id' value, because nothing
>> + * uses it for PTR_TO_MAP_VALUE (only for ..._OR_NULL)
>> + */
>> + return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
>> + range_within(rold, rcur) &&
>> + tn_in(rold->align, rcur->align);
>> + } else {
>> + /* If the ranges/align were not the same, but
>> + * everything else was and we didn't do a variable
>> + * access into a map then we are a-ok.
>> + */
>> + return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0;
>> + }
>> + case PTR_TO_MAP_VALUE_OR_NULL:
> does this new state comparison logic helps? Do you have any numbers before/after in the number of insns it had to process for the tests in selftests ?
I don't have the numbers, no (I'll try to collect them). This rewrite was
more because the data structures had changed so the old code needed changing
to match. It's mainly just a refactor and reimplementation of the existing
logic, I think, extended to cover the new 'align' member as well.
-Ed
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 18:50 +0200 |
| Subject | Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking |
| Message-ID | <tQ8Nc-6HE-15@gated-at.bofh.it> |
| In reply to | #1661356 |
On Thu, Jun 08, 2017 at 03:53:36PM +0100, Edward Cree wrote:
> >>
> >> - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
> >> + } else if (reg->type == PTR_TO_STACK) {
> >> + /* stack accesses must be at a fixed offset, so that we can
> >> + * determine what type of data were returned.
> >> + */
> >> + if (reg->align.mask) {
> >> + char tn_buf[48];
> >> +
> >> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
> >> + verbose("variable stack access align=%s off=%d size=%d",
> >> + tn_buf, off, size);
> >> + return -EACCES;
> > hmm. why this restriction?
> > I thought one of key points of the diff that ptr+var tracking logic
> > will now apply not only to map_value, but to stack_ptr as well?
> As the comment above it says, we need to determine what was returned:
> was it STACK_MISC or STACK_SPILL, and if the latter, what kind of pointer
> was spilled there? See check_stack_read(), which I should probably
> mention in the comment.
this piece of code is not only spill/fill, but normal ldx/stx stack access.
Consider the frequent pattern that many folks tried to do:
bpf_prog()
{
char buf[64];
int len;
bpf_probe_read(&len, sizeof(len), kernel_ptr_to_filename_len);
bpf_probe_read(buf, sizeof(buf), kernel_ptr_to_filename);
buf[len & (sizeof(buf) - 1)] = 0;
...
currently above is not supported, but when 'buf' is a pointer to map value
it works fine. Allocating extra bpf map just to do such workaround
isn't nice and since this patch generalized map_value_adj with ptr_to_stack
we can support above code too.
We can check that all bytes of stack for this variable access were
initialized already.
In the example above it will happen by bpf_probe_read (in the verifier code):
for (i = 0; i < meta.access_size; i++) {
err = check_mem_access(env, meta.regno, i, BPF_B, BPF_WRITE, -1);
so at the time of
buf[len & ..] = 0
we can check that 'stx' is within the range of inited stack and allow it.
> >> + if (!err && size < BPF_REG_SIZE && value_regno >= 0 && t == BPF_READ &&
> >> + state->regs[value_regno].type == SCALAR_VALUE) {
> >> + /* b/h/w load zero-extends, mark upper bits as known 0 */
> >> + state->regs[value_regno].align.value &= (1ULL << (size * 8)) - 1;
> >> + state->regs[value_regno].align.mask &= (1ULL << (size * 8)) - 1;
> > probably another helper from tnum.h is needed.
> I could rewrite as
> reg->align = tn_and(reg->align, tn_const((1ULL << (size * 8)) - 1))
yep. that's perfect.
> >> + /* Got here implies adding two SCALAR_VALUEs */
> >> + if (WARN_ON_ONCE(ptr_reg)) {
> >> + verbose("verifier internal error\n");
> >> + return -EINVAL;
> > ...
> >> + if (WARN_ON(!src_reg)) {
> >> + verbose("verifier internal error\n");
> >> + return -EINVAL;
> >> }
> > i'm lost with these bits.
> > Can you add a comment in what circumstances this can be hit
> > and what would be the consequences?
> It should be impossible to hit either of these cases. If we let the
> first through, we'd probably do invalid pointer arithmetic (e.g. we
> could multiply a pointer by two and think we'd just multiplied the
> variable offset). As for the latter, we access through that pointer
> so if it were NULL we would promptly oops.
I see. May be print verifier state in such warn_ons and make error
more human readable?
> >> + case PTR_TO_MAP_VALUE_OR_NULL:
> > does this new state comparison logic helps? Do you have any numbers before/after in the number of insns it had to process for the tests in selftests ?
> I don't have the numbers, no (I'll try to collect them). This rewrite was
Thanks. The main concern is that right now some complex programs
that cilium is using are close to the verifier complexity limit and these
big changes to amount of info recognized by the verifier can cause pruning
to be ineffective, so we need to test on big programs.
I think Daniel will be happy to test your next rev of the patches.
I'll test them as well.
At least 'insn_processed' from C code in tools/testing/selftests/bpf/
is a good estimate of how these changes affect pruning.
btw, I'm working on bpf_call support and also refactoring verifier
quite a bit, but my stuff is far from ready and I'll wait for
your rewrite to land first.
One of the things I'm working on is trying to get rid of state pruning
heuristics and use register+stack liveness information instead.
It's all experimental so far.
[toc] | [prev] | [next] | [standalone]
| From | Edward Cree <ecree@solarflare.com> |
|---|---|
| Date | 2017-06-08 21:40 +0200 |
| Subject | Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking |
| Message-ID | <tQbrI-8p2-17@gated-at.bofh.it> |
| In reply to | #1661480 |
On 08/06/17 17:45, Alexei Starovoitov wrote:
> On Thu, Jun 08, 2017 at 03:53:36PM +0100, Edward Cree wrote:
>>>>
>>>> - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
>>>> + } else if (reg->type == PTR_TO_STACK) {
>>>> + /* stack accesses must be at a fixed offset, so that we can
>>>> + * determine what type of data were returned.
>>>> + */
>>>> + if (reg->align.mask) {
>>>> + char tn_buf[48];
>>>> +
>>>> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
>>>> + verbose("variable stack access align=%s off=%d size=%d",
>>>> + tn_buf, off, size);
>>>> + return -EACCES;
>>> hmm. why this restriction?
>>> I thought one of key points of the diff that ptr+var tracking logic
>>> will now apply not only to map_value, but to stack_ptr as well?
>> As the comment above it says, we need to determine what was returned:
>> was it STACK_MISC or STACK_SPILL, and if the latter, what kind of pointer
>> was spilled there? See check_stack_read(), which I should probably
>> mention in the comment.
> this piece of code is not only spill/fill, but normal ldx/stx stack access.
> Consider the frequent pattern that many folks tried to do:
> bpf_prog()
> {
> char buf[64];
> int len;
>
> bpf_probe_read(&len, sizeof(len), kernel_ptr_to_filename_len);
> bpf_probe_read(buf, sizeof(buf), kernel_ptr_to_filename);
> buf[len & (sizeof(buf) - 1)] = 0;
> ...
>
> currently above is not supported, but when 'buf' is a pointer to map value
> it works fine. Allocating extra bpf map just to do such workaround
> isn't nice and since this patch generalized map_value_adj with ptr_to_stack
> we can support above code too.
> We can check that all bytes of stack for this variable access were
> initialized already.
> In the example above it will happen by bpf_probe_read (in the verifier code):
> for (i = 0; i < meta.access_size; i++) {
> err = check_mem_access(env, meta.regno, i, BPF_B, BPF_WRITE, -1);
> so at the time of
> buf[len & ..] = 0
> we can check that 'stx' is within the range of inited stack and allow it.
Yes, we could check every byte of the stack within the range [buf, buf+63]
is a STACK_MISC and if so allow it. But since this is not supported by the
existing code (so it's not a regression), I'd prefer to leave that for a
future patch - this one is quite big enough already ;-)
>>>> + if (!err && size < BPF_REG_SIZE && value_regno >= 0 && t == BPF_READ &&
>>>> + state->regs[value_regno].type == SCALAR_VALUE) {
>>>> + /* b/h/w load zero-extends, mark upper bits as known 0 */
>>>> + state->regs[value_regno].align.value &= (1ULL << (size * 8)) - 1;
>>>> + state->regs[value_regno].align.mask &= (1ULL << (size * 8)) - 1;
>>> probably another helper from tnum.h is needed.
>> I could rewrite as
>> reg->align = tn_and(reg->align, tn_const((1ULL << (size * 8)) - 1))
> yep. that's perfect.
In the end I settled on adding a helper
struct tnum tnum_cast(struct tnum a, u8 size);
since I have a bunch of other places that cast things to 32 bits.
> I see. May be print verifier state in such warn_ons and make error
> more human readable?
Good idea, I'll do that.
>>>> + case PTR_TO_MAP_VALUE_OR_NULL:
>>> does this new state comparison logic helps? Do you have any numbers before/after in the number of insns it had to process for the tests in selftests ?
>> I don't have the numbers, no (I'll try to collect them). This rewrite was
> Thanks. The main concern is that right now some complex programs
> that cilium is using are close to the verifier complexity limit and these
> big changes to amount of info recognized by the verifier can cause pruning
> to be ineffective, so we need to test on big programs.
> I think Daniel will be happy to test your next rev of the patches.
> I'll test them as well.
> At least 'insn_processed' from C code in tools/testing/selftests/bpf/
> is a good estimate of how these changes affect pruning.
It looks like the only place this gets recorded is as "processed %d insns"
in the log_buf. Is there a convenient way to get at this, or am I going
to have to make bpf_verify_program grovel through the log sscanf()ing for
a matching line?
-Ed
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 23:30 +0200 |
| Subject | Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking |
| Message-ID | <tQdaa-14l-9@gated-at.bofh.it> |
| In reply to | #1661618 |
On Thu, Jun 08, 2017 at 08:38:29PM +0100, Edward Cree wrote:
> On 08/06/17 17:45, Alexei Starovoitov wrote:
> > On Thu, Jun 08, 2017 at 03:53:36PM +0100, Edward Cree wrote:
> >>>>
> >>>> - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
> >>>> + } else if (reg->type == PTR_TO_STACK) {
> >>>> + /* stack accesses must be at a fixed offset, so that we can
> >>>> + * determine what type of data were returned.
> >>>> + */
> >>>> + if (reg->align.mask) {
> >>>> + char tn_buf[48];
> >>>> +
> >>>> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
> >>>> + verbose("variable stack access align=%s off=%d size=%d",
> >>>> + tn_buf, off, size);
> >>>> + return -EACCES;
> >>> hmm. why this restriction?
> >>> I thought one of key points of the diff that ptr+var tracking logic
> >>> will now apply not only to map_value, but to stack_ptr as well?
> >> As the comment above it says, we need to determine what was returned:
> >> was it STACK_MISC or STACK_SPILL, and if the latter, what kind of pointer
> >> was spilled there? See check_stack_read(), which I should probably
> >> mention in the comment.
> > this piece of code is not only spill/fill, but normal ldx/stx stack access.
> > Consider the frequent pattern that many folks tried to do:
> > bpf_prog()
> > {
> > char buf[64];
> > int len;
> >
> > bpf_probe_read(&len, sizeof(len), kernel_ptr_to_filename_len);
> > bpf_probe_read(buf, sizeof(buf), kernel_ptr_to_filename);
> > buf[len & (sizeof(buf) - 1)] = 0;
> > ...
> >
> > currently above is not supported, but when 'buf' is a pointer to map value
> > it works fine. Allocating extra bpf map just to do such workaround
> > isn't nice and since this patch generalized map_value_adj with ptr_to_stack
> > we can support above code too.
> > We can check that all bytes of stack for this variable access were
> > initialized already.
> > In the example above it will happen by bpf_probe_read (in the verifier code):
> > for (i = 0; i < meta.access_size; i++) {
> > err = check_mem_access(env, meta.regno, i, BPF_B, BPF_WRITE, -1);
> > so at the time of
> > buf[len & ..] = 0
> > we can check that 'stx' is within the range of inited stack and allow it.
> Yes, we could check every byte of the stack within the range [buf, buf+63]
> is a STACK_MISC and if so allow it. But since this is not supported by the
> existing code (so it's not a regression), I'd prefer to leave that for a
> future patch - this one is quite big enough already ;-)
of course! just exploring.
> >>>> + if (!err && size < BPF_REG_SIZE && value_regno >= 0 && t == BPF_READ &&
> >>>> + state->regs[value_regno].type == SCALAR_VALUE) {
> >>>> + /* b/h/w load zero-extends, mark upper bits as known 0 */
> >>>> + state->regs[value_regno].align.value &= (1ULL << (size * 8)) - 1;
> >>>> + state->regs[value_regno].align.mask &= (1ULL << (size * 8)) - 1;
> >>> probably another helper from tnum.h is needed.
> >> I could rewrite as
> >> reg->align = tn_and(reg->align, tn_const((1ULL << (size * 8)) - 1))
> > yep. that's perfect.
> In the end I settled on adding a helper
> struct tnum tnum_cast(struct tnum a, u8 size);
> since I have a bunch of other places that cast things to 32 bits.
sounds good to me
> > I see. May be print verifier state in such warn_ons and make error
> > more human readable?
> Good idea, I'll do that.
> >>>> + case PTR_TO_MAP_VALUE_OR_NULL:
> >>> does this new state comparison logic helps? Do you have any numbers before/after in the number of insns it had to process for the tests in selftests ?
> >> I don't have the numbers, no (I'll try to collect them). This rewrite was
> > Thanks. The main concern is that right now some complex programs
> > that cilium is using are close to the verifier complexity limit and these
> > big changes to amount of info recognized by the verifier can cause pruning
> > to be ineffective, so we need to test on big programs.
> > I think Daniel will be happy to test your next rev of the patches.
> > I'll test them as well.
> > At least 'insn_processed' from C code in tools/testing/selftests/bpf/
> > is a good estimate of how these changes affect pruning.
> It looks like the only place this gets recorded is as "processed %d insns"
> in the log_buf. Is there a convenient way to get at this, or am I going
> to have to make bpf_verify_program grovel through the log sscanf()ing for
> a matching line?
typically we just run the tests with hacked log_level and grep.
similar stuff Dave did in test_align.c
[toc] | [prev] | [next] | [standalone]
| From | Daniel Borkmann <daniel@iogearbox.net> |
|---|---|
| Date | 2017-06-09 15:30 +0200 |
| Subject | Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking |
| Message-ID | <tQs9c-22p-5@gated-at.bofh.it> |
| In reply to | #1661480 |
On 06/08/2017 06:45 PM, Alexei Starovoitov wrote: [...] > I think Daniel will be happy to test your next rev of the patches. > I'll test them as well. > At least 'insn_processed' from C code in tools/testing/selftests/bpf/ > is a good estimate of how these changes affect pruning. Without having looked more deeply (yet), I ran couple of tests with the cilium test suite to track complexity. Overall programs load with the set applied, worst case increase I've seen for some of the current progs was by ~80% from ~33k to ~60k insns. Will still go over the code for an initial review either today or tomorrow. > btw, I'm working on bpf_call support and also refactoring verifier > quite a bit, but my stuff is far from ready and I'll wait for > your rewrite to land first. > One of the things I'm working on is trying to get rid of state pruning > heuristics and use register+stack liveness information instead. > It's all experimental so far. Thanks, Daniel
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2017-06-08 04:50 +0200 |
| Subject | Re: [RFC PATCH net-next 4/5] bpf/verifier: track signed and unsigned min/max values |
| Message-ID | <tPVGh-6Kh-7@gated-at.bofh.it> |
| In reply to | #1659883 |
On Wed, Jun 07, 2017 at 03:59:25PM +0100, Edward Cree wrote:
> Allows us to, sometimes, combine information from a signed check of one
> bound and an unsigned check of the other.
> We now track the full range of possible values, rather than restricting
> ourselves to [0, 1<<30) and considering anything beyond that as
> unknown. While this is probably not necessary, it makes the code more
> straightforward and symmetrical between signed and unsigned bounds.
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> include/linux/bpf_verifier.h | 22 +-
> kernel/bpf/verifier.c | 661 +++++++++++++++++++++++++------------------
> 2 files changed, 395 insertions(+), 288 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index e341469..10a5944 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -11,11 +11,15 @@
> #include <linux/filter.h> /* for MAX_BPF_STACK */
> #include <linux/tnum.h>
>
> - /* Just some arbitrary values so we can safely do math without overflowing and
> - * are obviously wrong for any sort of memory access.
> - */
> -#define BPF_REGISTER_MAX_RANGE (1024 * 1024 * 1024)
> -#define BPF_REGISTER_MIN_RANGE -1
> +/* Maximum variable offset umax_value permitted when resolving memory accesses.
> + * In practice this is far bigger than any realistic pointer offset; this limit
> + * ensures that umax_value + (int)off + (int)size cannot overflow a u64.
> + */
> +#define BPF_MAX_VAR_OFF (1ULL << 31)
> +/* Maximum variable size permitted for ARG_CONST_SIZE[_OR_ZERO]. This ensures
> + * that converting umax_value to int cannot overflow.
> + */
> +#define BPF_MAX_VAR_SIZ INT_MAX
>
> struct bpf_reg_state {
> enum bpf_reg_type type;
> @@ -38,7 +42,7 @@ struct bpf_reg_state {
> * PTR_TO_MAP_VALUE_OR_NULL, we have to NULL-check it _first_.
> */
> u32 id;
> - /* These three fields must be last. See states_equal() */
> + /* These five fields must be last. See states_equal() */
> /* For scalar types (SCALAR_VALUE), this represents our knowledge of
> * the actual value.
> * For pointer types, this represents the variable part of the offset
> @@ -51,8 +55,10 @@ struct bpf_reg_state {
> * These refer to the same value as align, not necessarily the actual
> * contents of the register.
> */
> - s64 min_value; /* minimum possible (s64)value */
> - u64 max_value; /* maximum possible (u64)value */
> + s64 smin_value; /* minimum possible (s64)value */
> + s64 smax_value; /* maximum possible (s64)value */
> + u64 umin_value; /* minimum possible (u64)value */
> + u64 umax_value; /* maximum possible (u64)value */
have uneasy feeling about this one.
It's 16 extra bytes to be stored in every reg_state and memcmp later
while we didn't have cases where people wanted negative values
in ptr+var cases. Why bother than?
> unknown. While this is probably not necessary, it makes the code more
> straightforward and symmetrical between signed and unsigned bounds.
it's hard for me to see the 'straightforward' part yet.
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web