On 7/16/25 3:13 AM, Shung-Hsi Yu wrote:
Hi Andrii and Yonghong,
On Fri, May 23, 2025 at 09:13:40PM -0700, Yonghong Song wrote:
Add two tests:
- one test has 'rX <op> r10' where rX is not r10, and
- another test has 'rX <op> rY' where rX and rY are not r10
but there is an early insn 'rX = r10'.
Without previous verifier change, both tests will fail.
Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
---
.../selftests/bpf/progs/verifier_precision.c | 53 +++++++++++++++++++
1 file changed, 53 insertions(+)
I was looking this commit (5ffb537e416e) since it was a BPF selftest
test for CVE-2025-38279, but upon looking I found that the commit
differs from the patch, there is an extra hunk that changed
kernel/bpf/verifier.c that wasn't found the Yonghong's original patch.
I suppose it was meant to be squashed into the previous commit
e2d2115e56c4 "bpf: Do not include stack ptr register in precision
backtracking bookkeeping"?
Andrii made some change to my original patch for easy understanding.
See
https://lore.kernel.org/bpf/20250524041335.4046126-1-yonghong.song@xxxxxxxxx
Quoted below:
"
I've moved it inside the preceding if/else (twice), so it's more
obvious that BPF_X deal with both src_reg and dst_reg, and BPF_K case
deals only with BPF_K. The end result is the same, but I found this
way a bit easier to follow. Applied to bpf-next, thanks.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index
831c2eff56e1..c9a372ca7830 100644 --- a/kernel/bpf/verifier.c +++
b/kernel/bpf/verifier.c @@ -16471,6 +16471,8 @@ static int
check_cond_jmp_op(struct bpf_verifier_env *env,
if (src_reg->type == PTR_TO_STACK)
insn_flags |= INSN_F_SRC_REG_STACK;
+ if (dst_reg->type == PTR_TO_STACK) + insn_flags |= INSN_F_DST_REG_STACK; } else {
if (insn->src_reg != BPF_REG_0) {
verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
@@ -16480,10 +16482,11 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
memset(src_reg, 0, sizeof(*src_reg));
src_reg->type = SCALAR_VALUE;
__mark_reg_known(src_reg, insn->imm);
+ + if (dst_reg->type == PTR_TO_STACK) + insn_flags |=
INSN_F_DST_REG_STACK; }
- if (dst_reg->type == PTR_TO_STACK) - insn_flags |= INSN_F_DST_REG_STACK; if (insn_flags) {
err = push_insn_history(env, this_branch, insn_flags, 0);
if (err)
...
"
Since stable backports got only e2d2115e56c4, but not the 5ffb537e416e
here with the extra change for kernel/bpf/verifier.c, I'd guess the
backtracking logic in the stable kernel isn't correct at the moment,
so I'll send 5ffb537e416e "selftests/bpf: Add tests with stack ptr
register in conditional jmp" to stable as well. Let me know if that's
not the right thing to do.
Shung-Hsi
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 98c52829936e..a7d6e0c5928b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -16456,6 +16456,8 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
if (src_reg->type == PTR_TO_STACK)
insn_flags |= INSN_F_SRC_REG_STACK;
+ if (dst_reg->type == PTR_TO_STACK)
+ insn_flags |= INSN_F_DST_REG_STACK;
} else {
if (insn->src_reg != BPF_REG_0) {
verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
@@ -16465,10 +16467,11 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
memset(src_reg, 0, sizeof(*src_reg));
src_reg->type = SCALAR_VALUE;
__mark_reg_known(src_reg, insn->imm);
+
+ if (dst_reg->type == PTR_TO_STACK)
+ insn_flags |= INSN_F_DST_REG_STACK;
}
- if (dst_reg->type == PTR_TO_STACK)
- insn_flags |= INSN_F_DST_REG_STACK;
if (insn_flags) {
err = push_insn_history(env, this_branch, insn_flags, 0);
if (err)
diff --git a/tools/testing/selftests/bpf/progs/verifier_precision.c b/tools/testing/selftests/bpf/progs/verifier_precision.c
...
