Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add tests with stack ptr register in conditional jmp

Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add tests with stack ptr register in conditional jmp

[Shung-Hsi Yu]

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@linux.dev>
> ---
>  .../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"?

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
...

Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add tests with stack ptr register in conditional jmp

[Yonghong Song]

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@linux.dev>
>> ---
>>   .../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@linux.dev
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
> ...
>

Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add tests with stack ptr register in conditional jmp

[Shung-Hsi Yu]

On Wed, Jul 16, 2025 at 09:05:05AM -0700, Yonghong Song wrote:
> 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@linux.dev>
> > > ---
> > >   .../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@linux.dev
> 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.

Argh, indeed I missed the sibling thread. Thanks for point that out.

Shung-Hsi

...