Re: [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call()

[Puranjay Mohan <puranjay@kernel.org>]

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Thu, 2025-11-13 at 10:40 +0000, Puranjay Mohan wrote:
>
> [...]
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 1268fa075d4c..31136f9c418b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -3273,7 +3273,7 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>>  	struct bpf_kfunc_desc *desc;
>>  	const char *func_name;
>>  	struct btf *desc_btf;
>> -	unsigned long addr;
>> +	unsigned long addr, call_imm;
>>  	int err;
>>  
>>  	prog_aux = env->prog->aux;
>> @@ -3369,8 +3369,20 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>>  	if (err)
>>  		return err;
>>  
>> +	if (bpf_jit_supports_far_kfunc_call()) {
>> +		call_imm = func_id;
>> +	} else {
>> +		call_imm = BPF_CALL_IMM(addr);
>> +		/* Check whether the relative offset overflows desc->imm */
>> +		if ((unsigned long)(s32)call_imm != call_imm) {
>> +			verbose(env, "address of kernel func_id %u is out of range\n", func_id);
>> +			return -EINVAL;
>> +		}
>> +	}
>
> Instead of having this logic in two places, how about moving the
> desc->imm setup down to sort_kfunc_descs_by_imm_off()?
> I think it the only consumer of desc->imm in verifier.c.
> E.g. as in the diff attached.

This seems like the best way to move ahead with fixing this. I will send
v3 with your suggested diff.

>> +
>>  	desc = &tab->descs[tab->nr_descs++];
>>  	desc->func_id = func_id;
>> +	desc->imm = call_imm;
>>  	desc->offset = offset;
>>  	desc->addr = addr;
>>  	desc->func_model = func_model;
>> @@ -22353,17 +22365,15 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>>  	}
>>  
>>  set_imm:
>> -	if (bpf_jit_supports_far_kfunc_call()) {
>> -		call_imm = func_id;
>> -	} else {
>> +	if (!bpf_jit_supports_far_kfunc_call()) {
>>  		call_imm = BPF_CALL_IMM(addr);
>>  		/* Check whether the relative offset overflows desc->imm */
>>  		if ((unsigned long)(s32)call_imm != call_imm) {
>>  			verbose(env, "address of kernel func_id %u is out of range\n", func_id);
>>  			return -EINVAL;
>>  		}
>> +		desc->imm = call_imm;
>>  	}
>> -	desc->imm = call_imm;
>>  	desc->addr = addr;
>>  	return 0;
>>  }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1268fa075d4c..7ffe526c34cb 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3391,16 +3391,44 @@ static int kfunc_desc_cmp_by_imm_off(const void *a, const void *b)
>  	return 0;
>  }
>  
> -static void sort_kfunc_descs_by_imm_off(struct bpf_prog *prog)
> +static int set_kfunc_desc_imm(struct bpf_verifier_env *env, struct bpf_kfunc_desc *desc)
> +{
> +	unsigned long call_imm;
> +
> +	if (bpf_jit_supports_far_kfunc_call()) {
> +		call_imm = desc->func_id;
> +		return 0;
> +	} else {
> +		call_imm = BPF_CALL_IMM(desc->addr);
> +		/* Check whether the relative offset overflows desc->imm */
> +		if ((unsigned long)(s32)call_imm != call_imm) {
> +			verbose(env, "address of kernel func_id %u is out of range\n",
> +				desc->func_id);
> +			return -EINVAL;
> +		}
> +	}
> +	desc->imm = call_imm;
> +	return 0;
> +}
> +
> +static int sort_kfunc_descs_by_imm_off(struct bpf_verifier_env *env)
>  {
>  	struct bpf_kfunc_desc_tab *tab;
> +	int i, err;
>  
> -	tab = prog->aux->kfunc_tab;
> +	tab = env->prog->aux->kfunc_tab;
>  	if (!tab)
> -		return;
> +		return 0;
> +
> +	for (i = 0; i < tab->nr_descs; i++) {
> +		err = set_kfunc_desc_imm(env, &tab->descs[i]);
> +		if (err)
> +			return err;
> +	}
>  
>  	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
>  	     kfunc_desc_cmp_by_imm_off, NULL);
> +	return 0;
>  }
>  
>  bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog)
> @@ -22320,10 +22348,10 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>  	bool is_rdonly;
>  	u32 func_id = desc->func_id;
>  	u16 offset = desc->offset;
> -	unsigned long addr = desc->addr, call_imm;
> +	unsigned long addr = desc->addr;
>  
>  	if (offset) /* return if module BTF is used */
> -		goto set_imm;
> +		return 0;
>  
>  	if (bpf_dev_bound_kfunc_id(func_id)) {
>  		xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
> @@ -22351,19 +22379,6 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>  		if (!env->insn_aux_data[insn_idx].non_sleepable)
>  			addr = (unsigned long)bpf_dynptr_from_file_sleepable;
>  	}
> -
> -set_imm:
> -	if (bpf_jit_supports_far_kfunc_call()) {
> -		call_imm = func_id;
> -	} else {
> -		call_imm = BPF_CALL_IMM(addr);
> -		/* Check whether the relative offset overflows desc->imm */
> -		if ((unsigned long)(s32)call_imm != call_imm) {
> -			verbose(env, "address of kernel func_id %u is out of range\n", func_id);
> -			return -EINVAL;
> -		}
> -	}
> -	desc->imm = call_imm;
>  	desc->addr = addr;
>  	return 0;
>  }
> @@ -23441,7 +23456,9 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>  		}
>  	}
>  
> -	sort_kfunc_descs_by_imm_off(env->prog);
> +	ret = sort_kfunc_descs_by_imm_off(env);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }