| Message ID | 20210929111855.50254-9-hbathini@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf powerpc: Add BPF_PROBE_MEM support in powerpc JIT compiler | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | warning | Found 8 issues from 2 of 4 jobs. |
Le 29/09/2021 à 13:18, Hari Bathini a écrit : > With KUAP enabled, any kernel code which wants to access userspace > needs to be surrounded by disable-enable KUAP. But that is not > happening for BPF_PROBE_MEM load instruction. Though PPC32 does not > support read protection, considering the fact that PTR_TO_BTF_ID > (which uses BPF_PROBE_MEM mode) could either be a valid kernel pointer > or NULL but should never be a pointer to userspace address, execute > BPF_PROBE_MEM load only if addr is kernel address, otherwise set > dst_reg=0 and move on. > > This will catch NULL, valid or invalid userspace pointers. Only bad > kernel pointer will be handled by BPF exception table. > > [Alexei suggested for x86] > Suggested-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > > Changes in v4: > * Adjusted the emit code to avoid using temporary reg. > > > arch/powerpc/net/bpf_jit_comp32.c | 34 +++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c > index 6ee13a09c70d..2ac81563c78d 100644 > --- a/arch/powerpc/net/bpf_jit_comp32.c > +++ b/arch/powerpc/net/bpf_jit_comp32.c > @@ -818,6 +818,40 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * > case BPF_LDX | BPF_PROBE_MEM | BPF_W: > case BPF_LDX | BPF_MEM | BPF_DW: /* dst = *(u64 *)(ul) (src + off) */ > case BPF_LDX | BPF_PROBE_MEM | BPF_DW: > + /* > + * As PTR_TO_BTF_ID that uses BPF_PROBE_MEM mode could either be a valid > + * kernel pointer or NULL but not a userspace address, execute BPF_PROBE_MEM > + * load only if addr is kernel address (see is_kernel_addr()), otherwise > + * set dst_reg=0 and move on. > + */ > + if (BPF_MODE(code) == BPF_PROBE_MEM) { > + PPC_LI32(_R0, TASK_SIZE - off); > + EMIT(PPC_RAW_CMPLW(src_reg, _R0)); > + PPC_BCC(COND_GT, (ctx->idx + 5) * 4); > + EMIT(PPC_RAW_LI(dst_reg, 0)); > + /* > + * For BPF_DW case, "li reg_h,0" would be needed when > + * !fp->aux->verifier_zext. Emit NOP otherwise. > + * > + * Note that "li reg_h,0" is emitted for BPF_B/H/W case, > + * if necessary. So, jump there insted of emitting an > + * additional "li reg_h,0" instruction. > + */ > + if (size == BPF_DW && !fp->aux->verifier_zext) > + EMIT(PPC_RAW_LI(dst_reg_h, 0)); > + else > + EMIT(PPC_RAW_NOP()); > + /* > + * Need to jump two instructions instead of one for BPF_DW case > + * as there are two load instructions for dst_reg_h & dst_reg > + * respectively. > + */ > + if (size == BPF_DW) > + PPC_JMP((ctx->idx + 3) * 4); > + else > + PPC_JMP((ctx->idx + 2) * 4); > + } > + > switch (size) { > case BPF_B: > EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off)); >
Hi, Le 29/09/2021 à 13:18, Hari Bathini a écrit : > With KUAP enabled, any kernel code which wants to access userspace > needs to be surrounded by disable-enable KUAP. But that is not > happening for BPF_PROBE_MEM load instruction. Though PPC32 does not > support read protection, considering the fact that PTR_TO_BTF_ID > (which uses BPF_PROBE_MEM mode) could either be a valid kernel pointer > or NULL but should never be a pointer to userspace address, execute > BPF_PROBE_MEM load only if addr is kernel address, otherwise set > dst_reg=0 and move on. While looking at the series "bpf: verifier: stop emitting zext for LDX" from Puranjay I got a question on this old commit, see below. > > This will catch NULL, valid or invalid userspace pointers. Only bad > kernel pointer will be handled by BPF exception table. > > [Alexei suggested for x86] > Suggested-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> > --- > > Changes in v4: > * Adjusted the emit code to avoid using temporary reg. > > > arch/powerpc/net/bpf_jit_comp32.c | 34 +++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c > index 6ee13a09c70d..2ac81563c78d 100644 > --- a/arch/powerpc/net/bpf_jit_comp32.c > +++ b/arch/powerpc/net/bpf_jit_comp32.c > @@ -818,6 +818,40 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * > case BPF_LDX | BPF_PROBE_MEM | BPF_W: > case BPF_LDX | BPF_MEM | BPF_DW: /* dst = *(u64 *)(ul) (src + off) */ > case BPF_LDX | BPF_PROBE_MEM | BPF_DW: > + /* > + * As PTR_TO_BTF_ID that uses BPF_PROBE_MEM mode could either be a valid > + * kernel pointer or NULL but not a userspace address, execute BPF_PROBE_MEM > + * load only if addr is kernel address (see is_kernel_addr()), otherwise > + * set dst_reg=0 and move on. > + */ > + if (BPF_MODE(code) == BPF_PROBE_MEM) { > + PPC_LI32(_R0, TASK_SIZE - off); > + EMIT(PPC_RAW_CMPLW(src_reg, _R0)); > + PPC_BCC(COND_GT, (ctx->idx + 5) * 4); > + EMIT(PPC_RAW_LI(dst_reg, 0)); > + /* > + * For BPF_DW case, "li reg_h,0" would be needed when > + * !fp->aux->verifier_zext. Emit NOP otherwise. > + * > + * Note that "li reg_h,0" is emitted for BPF_B/H/W case, > + * if necessary. So, jump there insted of emitting an > + * additional "li reg_h,0" instruction. > + */ > + if (size == BPF_DW && !fp->aux->verifier_zext) > + EMIT(PPC_RAW_LI(dst_reg_h, 0)); > + else > + EMIT(PPC_RAW_NOP()); While do you need a NOP in the else case ? Can't we just emit no instruction in that case ? > + /* > + * Need to jump two instructions instead of one for BPF_DW case > + * as there are two load instructions for dst_reg_h & dst_reg > + * respectively. > + */ > + if (size == BPF_DW) > + PPC_JMP((ctx->idx + 3) * 4); > + else > + PPC_JMP((ctx->idx + 2) * 4); > + } > + > switch (size) { > case BPF_B: > EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
On 14/09/23 11:48 am, Christophe Leroy wrote: > Hi, > Hi Christophe, > Le 29/09/2021 à 13:18, Hari Bathini a écrit : >> With KUAP enabled, any kernel code which wants to access userspace >> needs to be surrounded by disable-enable KUAP. But that is not >> happening for BPF_PROBE_MEM load instruction. Though PPC32 does not >> support read protection, considering the fact that PTR_TO_BTF_ID >> (which uses BPF_PROBE_MEM mode) could either be a valid kernel pointer >> or NULL but should never be a pointer to userspace address, execute >> BPF_PROBE_MEM load only if addr is kernel address, otherwise set >> dst_reg=0 and move on. > > While looking at the series "bpf: verifier: stop emitting zext for LDX" > from Puranjay I got a question on this old commit, see below. > >> >> This will catch NULL, valid or invalid userspace pointers. Only bad >> kernel pointer will be handled by BPF exception table. >> >> [Alexei suggested for x86] >> Suggested-by: Alexei Starovoitov <ast@kernel.org> >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >> --- >> >> Changes in v4: >> * Adjusted the emit code to avoid using temporary reg. >> >> >> arch/powerpc/net/bpf_jit_comp32.c | 34 +++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c >> index 6ee13a09c70d..2ac81563c78d 100644 >> --- a/arch/powerpc/net/bpf_jit_comp32.c >> +++ b/arch/powerpc/net/bpf_jit_comp32.c >> @@ -818,6 +818,40 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> case BPF_LDX | BPF_PROBE_MEM | BPF_W: >> case BPF_LDX | BPF_MEM | BPF_DW: /* dst = *(u64 *)(ul) (src + off) */ >> case BPF_LDX | BPF_PROBE_MEM | BPF_DW: >> + /* >> + * As PTR_TO_BTF_ID that uses BPF_PROBE_MEM mode could either be a valid >> + * kernel pointer or NULL but not a userspace address, execute BPF_PROBE_MEM >> + * load only if addr is kernel address (see is_kernel_addr()), otherwise >> + * set dst_reg=0 and move on. >> + */ >> + if (BPF_MODE(code) == BPF_PROBE_MEM) { >> + PPC_LI32(_R0, TASK_SIZE - off); >> + EMIT(PPC_RAW_CMPLW(src_reg, _R0)); >> + PPC_BCC(COND_GT, (ctx->idx + 5) * 4); >> + EMIT(PPC_RAW_LI(dst_reg, 0)); >> + /* >> + * For BPF_DW case, "li reg_h,0" would be needed when >> + * !fp->aux->verifier_zext. Emit NOP otherwise. >> + * >> + * Note that "li reg_h,0" is emitted for BPF_B/H/W case, >> + * if necessary. So, jump there insted of emitting an >> + * additional "li reg_h,0" instruction. >> + */ >> + if (size == BPF_DW && !fp->aux->verifier_zext) >> + EMIT(PPC_RAW_LI(dst_reg_h, 0)); >> + else >> + EMIT(PPC_RAW_NOP()); > > While do you need a NOP in the else case ? Can't we just emit no > instruction in that case ? Yeah but used the same offset for all cases in the conditional branch above. To drop the NOP, the conditional branch offset can be calculated based on the above if condition, I guess.. Thanks, Hari
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c index 6ee13a09c70d..2ac81563c78d 100644 --- a/arch/powerpc/net/bpf_jit_comp32.c +++ b/arch/powerpc/net/bpf_jit_comp32.c @@ -818,6 +818,40 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * case BPF_LDX | BPF_PROBE_MEM | BPF_W: case BPF_LDX | BPF_MEM | BPF_DW: /* dst = *(u64 *)(ul) (src + off) */ case BPF_LDX | BPF_PROBE_MEM | BPF_DW: + /* + * As PTR_TO_BTF_ID that uses BPF_PROBE_MEM mode could either be a valid + * kernel pointer or NULL but not a userspace address, execute BPF_PROBE_MEM + * load only if addr is kernel address (see is_kernel_addr()), otherwise + * set dst_reg=0 and move on. + */ + if (BPF_MODE(code) == BPF_PROBE_MEM) { + PPC_LI32(_R0, TASK_SIZE - off); + EMIT(PPC_RAW_CMPLW(src_reg, _R0)); + PPC_BCC(COND_GT, (ctx->idx + 5) * 4); + EMIT(PPC_RAW_LI(dst_reg, 0)); + /* + * For BPF_DW case, "li reg_h,0" would be needed when + * !fp->aux->verifier_zext. Emit NOP otherwise. + * + * Note that "li reg_h,0" is emitted for BPF_B/H/W case, + * if necessary. So, jump there insted of emitting an + * additional "li reg_h,0" instruction. + */ + if (size == BPF_DW && !fp->aux->verifier_zext) + EMIT(PPC_RAW_LI(dst_reg_h, 0)); + else + EMIT(PPC_RAW_NOP()); + /* + * Need to jump two instructions instead of one for BPF_DW case + * as there are two load instructions for dst_reg_h & dst_reg + * respectively. + */ + if (size == BPF_DW) + PPC_JMP((ctx->idx + 3) * 4); + else + PPC_JMP((ctx->idx + 2) * 4); + } + switch (size) { case BPF_B: EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
With KUAP enabled, any kernel code which wants to access userspace needs to be surrounded by disable-enable KUAP. But that is not happening for BPF_PROBE_MEM load instruction. Though PPC32 does not support read protection, considering the fact that PTR_TO_BTF_ID (which uses BPF_PROBE_MEM mode) could either be a valid kernel pointer or NULL but should never be a pointer to userspace address, execute BPF_PROBE_MEM load only if addr is kernel address, otherwise set dst_reg=0 and move on. This will catch NULL, valid or invalid userspace pointers. Only bad kernel pointer will be handled by BPF exception table. [Alexei suggested for x86] Suggested-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> --- Changes in v4: * Adjusted the emit code to avoid using temporary reg. arch/powerpc/net/bpf_jit_comp32.c | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)