[PATCH v4 8/8] bpf ppc32: Access only if addr is kernel address

[Hari Bathini]

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 at kernel.org>
>> Signed-off-by: Hari Bathini <hbathini at 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