Re: [PATCH v2 bpf-next 1/2] bpf: Fix Null-Pointer Dereference in kernel_clone() via BPF fmod_ret on security_task_alloc

[Leon Hwang]

On 10/4/26 16:03, Feng Yang wrote:
> [...]
>> +static int modify_return_get_retval_range(const struct bpf_prog *prog,
>> +					  struct bpf_retval_range *retval_range)

How about 'range' instead of 'retval_range' like its caller?

>> +{
>> +	unsigned long addr = (unsigned long)prog->aux->dst_trampoline->func.addr;
>> +
>> +	if (within_error_injection_list(addr)) {
>> +		switch (get_injectable_error_type(addr)) {
>> +		case EI_ETYPE_NULL:
>> +			retval_range->minval = 0;
>> +			retval_range->maxval = 0;
>> +			break;
>> +		case EI_ETYPE_ERRNO:
>> +			retval_range->minval = -MAX_ERRNO;
>> +			retval_range->maxval = -1;
>> +			break;
> 
> This refers to the documentation in fault-injection.rst:
> 
> Each error injectable functions will have the error type specified by the
> ALLOW_ERROR_INJECTION() macro. You have to choose it carefully if you add
> a new error injectable function. If the wrong error type is chosen, the
> kernel may crash because it may not be able to handle the error.
> There are 4 types of errors defined in include/asm-generic/error-injection.h
> 
> EI_ETYPE_NULL
>   This function will return `NULL` if it fails. e.g. return an allocated
>   object address.
> 
> EI_ETYPE_ERRNO
>   This function will return an `-errno` error code if it fails. e.g. return
>   -EINVAL if the input is wrong. This will include the functions which will
>   return an address which encodes `-errno` by ERR_PTR() macro.
> 
> EI_ETYPE_ERRNO_NULL
>   This function will return an `-errno` or `NULL` if it fails. If the caller
>   of this function checks the return value with IS_ERR_OR_NULL() macro, this
>   type will be appropriate.
> 
> EI_ETYPE_TRUE
>   This function will return `true` (non-zero positive value) if it fails.
> 
> Restrict EI_ETYPE_ERRNO to only return error codes.
> However, it was noticed that the self-test bpf_testmod_test_read uses 
> ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO); and returns 0, which causes a failure.
> So should returning 0 be considered valid for the EI_ETYPE_ERRNO type, or should the self-test be modified instead?
> 
I think it would be better to add such explanation in code comment.

Thanks,
Leon