* [PATCH v2] bpf: verifier: MOV64 don't mark dst reg unbounded
@ 2018-07-31 17:17 Arthur Fabre
2018-07-31 18:02 ` Edward Cree
2018-07-31 21:07 ` Daniel Borkmann
0 siblings, 2 replies; 3+ messages in thread
From: Arthur Fabre @ 2018-07-31 17:17 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Y Song, Edward Cree, netdev
Cc: Arthur Fabre
When check_alu_op() handles a BPF_MOV64 between two registers,
it calls check_reg_arg(DST_OP) on the dst register, marking it as unbounded.
If the src and dst register are the same, this marks the src as
unbounded, which can lead to unexpected errors for further checks that
rely on bounds info. For example:
BPF_MOV64_IMM(BPF_REG_2, 0),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_2),
BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN(),
results in:
"math between ctx pointer and register with unbounded min value is not
allowed"
check_alu_op() now uses check_reg_arg(DST_OP_NO_MARK), and MOVs
that need to mark the dst register (MOVIMM, MOV32) do so.
Added a test case for MOV64 dst == src, and dst != src.
Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
---
v2: Add mov64 tests, always use DST_OP_NO_MARK
kernel/bpf/verifier.c | 6 +++--
tools/testing/selftests/bpf/test_verifier.c | 26 +++++++++++++++++++++
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 63aaac52a265..ec63b56be4af 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3238,8 +3238,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
}
}
- /* check dest operand */
- err = check_reg_arg(env, insn->dst_reg, DST_OP);
+ /* check dest operand, mark as required later */
+ err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
if (err)
return err;
@@ -3265,6 +3265,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
/* case: R = imm
* remember the value we stored into this reg
*/
+ /* clear any state __mark_reg_known doesn't set */
+ mark_reg_unknown(env, regs, insn->dst_reg);
regs[insn->dst_reg].type = SCALAR_VALUE;
if (BPF_CLASS(insn->code) == BPF_ALU64) {
__mark_reg_known(regs + insn->dst_reg,
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 41106d9d5cc7..79f10e95e7df 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -12372,6 +12372,32 @@ static struct bpf_test tests[] = {
.result = REJECT,
.errstr = "variable ctx access var_off=(0x0; 0x4)",
},
+ {
+ "mov64 src == dst",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_2),
+ // Check bounds are OK
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "mov64 src != dst",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_3, 0),
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_3),
+ // Check bounds are OK
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
};
static int probe_filter_length(const struct bpf_insn *fp)
--
2.18.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] bpf: verifier: MOV64 don't mark dst reg unbounded
2018-07-31 17:17 [PATCH v2] bpf: verifier: MOV64 don't mark dst reg unbounded Arthur Fabre
@ 2018-07-31 18:02 ` Edward Cree
2018-07-31 21:07 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: Edward Cree @ 2018-07-31 18:02 UTC (permalink / raw)
To: Arthur Fabre, Alexei Starovoitov, Daniel Borkmann, Y Song, netdev
On 31/07/18 18:17, Arthur Fabre wrote:
> When check_alu_op() handles a BPF_MOV64 between two registers,
> it calls check_reg_arg(DST_OP) on the dst register, marking it as unbounded.
> If the src and dst register are the same, this marks the src as
> unbounded, which can lead to unexpected errors for further checks that
> rely on bounds info. For example:
>
> BPF_MOV64_IMM(BPF_REG_2, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
> BPF_MOV64_IMM(BPF_REG_0, 0),
> BPF_EXIT_INSN(),
>
> results in:
>
> "math between ctx pointer and register with unbounded min value is not
> allowed"
>
> check_alu_op() now uses check_reg_arg(DST_OP_NO_MARK), and MOVs
> that need to mark the dst register (MOVIMM, MOV32) do so.
>
> Added a test case for MOV64 dst == src, and dst != src.
>
> Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
> ---
> v2: Add mov64 tests, always use DST_OP_NO_MARK
>
> kernel/bpf/verifier.c | 6 +++--
> tools/testing/selftests/bpf/test_verifier.c | 26 +++++++++++++++++++++
> 2 files changed, 30 insertions(+), 2 deletions(-)
Acked-by: Edward Cree <ecree@solarflare.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] bpf: verifier: MOV64 don't mark dst reg unbounded
2018-07-31 17:17 [PATCH v2] bpf: verifier: MOV64 don't mark dst reg unbounded Arthur Fabre
2018-07-31 18:02 ` Edward Cree
@ 2018-07-31 21:07 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2018-07-31 21:07 UTC (permalink / raw)
To: Arthur Fabre, Alexei Starovoitov, Y Song, Edward Cree, netdev
On 07/31/2018 07:17 PM, Arthur Fabre wrote:
> When check_alu_op() handles a BPF_MOV64 between two registers,
> it calls check_reg_arg(DST_OP) on the dst register, marking it as unbounded.
> If the src and dst register are the same, this marks the src as
> unbounded, which can lead to unexpected errors for further checks that
> rely on bounds info. For example:
>
> BPF_MOV64_IMM(BPF_REG_2, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
> BPF_MOV64_IMM(BPF_REG_0, 0),
> BPF_EXIT_INSN(),
>
> results in:
>
> "math between ctx pointer and register with unbounded min value is not
> allowed"
>
> check_alu_op() now uses check_reg_arg(DST_OP_NO_MARK), and MOVs
> that need to mark the dst register (MOVIMM, MOV32) do so.
>
> Added a test case for MOV64 dst == src, and dst != src.
>
> Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
Looks good, applied to bpf-next, thanks Arthur!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-07-31 22:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-31 17:17 [PATCH v2] bpf: verifier: MOV64 don't mark dst reg unbounded Arthur Fabre
2018-07-31 18:02 ` Edward Cree
2018-07-31 21:07 ` Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.