[bpf-next,v3,11/12] bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

Commit Message

Eduard Zingerman July 15, 2024, 11:02 p.m. UTC

There is a number of BPF helper functions that use ARG_ANYTHING to
mark parameters that are used as memory addresses.
An address of BPF stack slot could be passed as such parameter
for two such helper functions:
- bpf_probe_read_kernel
- bpf_probe_read_kernel_str

This might lead to a surprising behavior in combination with nocsr
rewrites, e.g. consider the program below:

     1: r1 = 1;
        /* nocsr pattern with stack offset -16 */
     2: *(u64 *)(r10 - 16) = r1;
     3: call %[bpf_get_smp_processor_id];
     4: r1 = *(u64 *)(r10 - 16);
     5: r1 = r10;
     6: r1 += -8;
     7: r2 = 1;
     8: r3 = r10;
     9: r3 += -16;
        /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
    10: call %[bpf_probe_read_kernel];
    11: exit;

Here nocsr rewrite logic would remove instructions (2) and (4).
However, (2) writes a value that is later read by a call at (10).

Function check_func_arg() is called from check_helper_call() and is
responsible for memory access checks, when helper argument type is
declared as ARG_PTR_TO_... .

However, for ARG_ANYTHING this function returns early and does not
call check_stack_{read,write}().

This patch opts to add a check_nocsr_stack_contract() to the
ARG_ANYTHING return path if passed parameter is a pointer to stack.

Signed-off-by: Eduard Zingerman <[e-mail]>
---
 kernel/bpf/verifier.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Alexei Starovoitov July 16, 2024, 2 a.m. UTC | #1

On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[e-mail]> wrote:
>
> There is a number of BPF helper functions that use ARG_ANYTHING to
> mark parameters that are used as memory addresses.
> An address of BPF stack slot could be passed as such parameter
> for two such helper functions:
> - bpf_probe_read_kernel
> - bpf_probe_read_kernel_str
>
> This might lead to a surprising behavior in combination with nocsr
> rewrites, e.g. consider the program below:
>
>      1: r1 = 1;
>         /* nocsr pattern with stack offset -16 */
>      2: *(u64 *)(r10 - 16) = r1;
>      3: call %[bpf_get_smp_processor_id];
>      4: r1 = *(u64 *)(r10 - 16);
>      5: r1 = r10;
>      6: r1 += -8;
>      7: r2 = 1;
>      8: r3 = r10;
>      9: r3 += -16;
>         /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
>     10: call %[bpf_probe_read_kernel];
>     11: exit;
>
> Here nocsr rewrite logic would remove instructions (2) and (4).
> However, (2) writes a value that is later read by a call at (10).

This makes no sense to me.
This bpf prog is broken.
If probe_read is used to read stack it will read garbage.
JITs and the verifier are allowed to do any transformation
that keeps the program semantics and safety.

pw-bot: cr

Eduard Zingerman July 16, 2024, 10:03 a.m. UTC | #2

On Mon, 2024-07-15 at 19:00 -0700, Alexei Starovoitov wrote:
> On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[e-mail]> wrote:

[...]

> > This might lead to a surprising behavior in combination with nocsr
> > rewrites, e.g. consider the program below:
> > 
> >      1: r1 = 1;
> >         /* nocsr pattern with stack offset -16 */
> >      2: *(u64 *)(r10 - 16) = r1;
> >      3: call %[bpf_get_smp_processor_id];
> >      4: r1 = *(u64 *)(r10 - 16);
> >      5: r1 = r10;
> >      6: r1 += -8;
> >      7: r2 = 1;
> >      8: r3 = r10;
> >      9: r3 += -16;
> >         /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
> >     10: call %[bpf_probe_read_kernel];
> >     11: exit;
> > 
> > Here nocsr rewrite logic would remove instructions (2) and (4).
> > However, (2) writes a value that is later read by a call at (10).
> 
> This makes no sense to me.
> This bpf prog is broken.
> If probe_read is used to read stack it will read garbage.
> JITs and the verifier are allowed to do any transformation
> that keeps the program semantics and safety.

I tried to run the following program
(should have run it earlier):

SEC("raw_tp")
__retval(42)
__success
int bpf_probe_read_kernel_stack_ptr(void *ctx)
{
	unsigned long a = 17;
	unsigned long b = 42;
	int err;

	err = bpf_probe_read_kernel(&a, 8, &b);
	if (err)
		return -1;
	return a;
}

And indeed, it does not produce expected result,
the retval varies around 22079.
However, I don't really understand why this program is broken.
E.g. from C compiler pov pointer &b escapes, and compiler is not
really allowed to replace object at that offset with garbage.
Is it a limitation of the bpf_probe_read_kernel() that it cannot read
BPF program stack?

Eduard Zingerman July 16, 2024, 6:15 p.m. UTC | #3

On Tue, 2024-07-16 at 03:03 -0700, Eduard Zingerman wrote:
> On Mon, 2024-07-15 at 19:00 -0700, Alexei Starovoitov wrote:
> > On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[e-mail]> wrote:
> 
> [...]
> 
> > > This might lead to a surprising behavior in combination with nocsr
> > > rewrites, e.g. consider the program below:
> > > 
> > >      1: r1 = 1;
> > >         /* nocsr pattern with stack offset -16 */
> > >      2: *(u64 *)(r10 - 16) = r1;
> > >      3: call %[bpf_get_smp_processor_id];
> > >      4: r1 = *(u64 *)(r10 - 16);
> > >      5: r1 = r10;
> > >      6: r1 += -8;
> > >      7: r2 = 1;
> > >      8: r3 = r10;
> > >      9: r3 += -16;
> > >         /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
> > >     10: call %[bpf_probe_read_kernel];
> > >     11: exit;
> > > 
> > > Here nocsr rewrite logic would remove instructions (2) and (4).
> > > However, (2) writes a value that is later read by a call at (10).
> > 
> > This makes no sense to me.
> > This bpf prog is broken.
> > If probe_read is used to read stack it will read garbage.
> > JITs and the verifier are allowed to do any transformation
> > that keeps the program semantics and safety.

Ok, my bad, the following program works at the moment:

SEC("socket") // <---- used wrong program type
__retval(42)
__success
int bpf_probe_read_kernel_stack_ptr(void *ctx)
{
	unsigned long a = 17;
	unsigned long b = 42;
	int err;

	err = bpf_probe_read_kernel(&a, 8, &b);
	if (err)
		return 1;
	return a;
}

And it is compiled to BPF as one would expect:

       ... fp[-8,-16] setup ...
       4:	r1 = r10
       5:	r1 += -0x8
       6:	r3 = r10
       7:	r3 += -0x10
       8:	w2 = 0x8
       9:	call 0x71
       ... return check ...

So, the point stands: from C compiler pov pointer &b escapes,
and compiler is not really allowed to replace object at that offset
with garbage. Why do you think the program is broken?

I don't mind dropping the patch in question, but I agree with Andrii's
viewpoint that there is nothing wrong with this use case.

Alexei Starovoitov July 20, 2024, 1:54 a.m. UTC | #4

On Tue, Jul 16, 2024 at 11:15 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Tue, 2024-07-16 at 03:03 -0700, Eduard Zingerman wrote:
> > On Mon, 2024-07-15 at 19:00 -0700, Alexei Starovoitov wrote:
> > > On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[e-mail]> wrote:
> >
> > [...]
> >
> > > > This might lead to a surprising behavior in combination with nocsr
> > > > rewrites, e.g. consider the program below:
> > > >
> > > >      1: r1 = 1;
> > > >         /* nocsr pattern with stack offset -16 */
> > > >      2: *(u64 *)(r10 - 16) = r1;
> > > >      3: call %[bpf_get_smp_processor_id];
> > > >      4: r1 = *(u64 *)(r10 - 16);
> > > >      5: r1 = r10;
> > > >      6: r1 += -8;
> > > >      7: r2 = 1;
> > > >      8: r3 = r10;
> > > >      9: r3 += -16;
> > > >         /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
> > > >     10: call %[bpf_probe_read_kernel];
> > > >     11: exit;
> > > >
> > > > Here nocsr rewrite logic would remove instructions (2) and (4).
> > > > However, (2) writes a value that is later read by a call at (10).
> > >
> > > This makes no sense to me.
> > > This bpf prog is broken.
> > > If probe_read is used to read stack it will read garbage.
> > > JITs and the verifier are allowed to do any transformation
> > > that keeps the program semantics and safety.
>
> Ok, my bad, the following program works at the moment:
>
> SEC("socket") // <---- used wrong program type
> __retval(42)
> __success
> int bpf_probe_read_kernel_stack_ptr(void *ctx)
> {
>         unsigned long a = 17;
>         unsigned long b = 42;
>         int err;
>
>         err = bpf_probe_read_kernel(&a, 8, &b);
>         if (err)
>                 return 1;
>         return a;
> }
>
> And it is compiled to BPF as one would expect:
>
>        ... fp[-8,-16] setup ...
>        4:       r1 = r10
>        5:       r1 += -0x8
>        6:       r3 = r10
>        7:       r3 += -0x10
>        8:       w2 = 0x8
>        9:       call 0x71
>        ... return check ...
>
> So, the point stands: from C compiler pov pointer &b escapes,
> and compiler is not really allowed to replace object at that offset
> with garbage. Why do you think the program is broken?

This is apples to oranges.
Compiler sees that the address of 'b' is taken and passed
into a function with side effect.
Whether 3rd arg of bpf_probe_read_kernel() is void * or long
is irrelevant. Compilers will do it, because it's a C language
requirement.

> I don't mind dropping the patch in question, but I agree with Andrii's
> viewpoint that there is nothing wrong with this use case.

bpf_probe_read_kernel() is not special and it's 3rd argument is
some kernel address. Whether it's stack pointer or anything else
is irrelevant.
JITs and verifier are allowed to do any optimizations on stack
and any other memory completely ignoring presence of
bpf_probe_read_kernel() and what is being passed into it.

Tomorrow we will teach arm64 JIT to replace stack spill/fill with
spare register read/write. There is no way we're going to special case
a particular fp-16 slot because fp-16 was passed into probe_read.

Eduard Zingerman July 20, 2024, 1:58 a.m. UTC | #5

On Fri, 2024-07-19 at 18:54 -0700, Alexei Starovoitov wrote:

[...]

> > So, the point stands: from C compiler pov pointer &b escapes,
> > and compiler is not really allowed to replace object at that offset
> > with garbage. Why do you think the program is broken?
> 
> This is apples to oranges.
> Compiler sees that the address of 'b' is taken and passed
> into a function with side effect.
> Whether 3rd arg of bpf_probe_read_kernel() is void * or long
> is irrelevant. Compilers will do it, because it's a C language
> requirement.
> 
> > I don't mind dropping the patch in question, but I agree with Andrii's
> > viewpoint that there is nothing wrong with this use case.
> 
> bpf_probe_read_kernel() is not special and it's 3rd argument is
> some kernel address. Whether it's stack pointer or anything else
> is irrelevant.
> JITs and verifier are allowed to do any optimizations on stack
> and any other memory completely ignoring presence of
> bpf_probe_read_kernel() and what is being passed into it.
> 
> Tomorrow we will teach arm64 JIT to replace stack spill/fill with
> spare register read/write. There is no way we're going to special case
> a particular fp-16 slot because fp-16 was passed into probe_read.

Ok, I will re-submit w/o these two patches.
Andrii also requested to re-structure the check contract function to
reset .nocsr_pattern and .spills_num marks to 0 upon contract violation.

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 438daf36a694..77affc563a64 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8684,11 +8684,15 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		return err;
 
 	if (arg_type == ARG_ANYTHING) {
+		/* return value depends on env->allow_ptr_leaks */
 		if (is_pointer_value(env, regno)) {
 			verbose(env, "R%d leaks addr into helper function\n",
 				regno);
 			return -EACCES;
 		}
+		if (reg->type == PTR_TO_STACK)
+			check_nocsr_stack_contract(env, cur_func(env), insn_idx,
+						   reg->smin_value + reg->off);
 		return 0;
 	}