[PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Verify if the pointer obtained from bpf_xdp_pointer() is either an error or
NULL before returning it.

The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. Instead of
solely checking for NULL, it should also verify if the pointer returned by
bpf_xdp_pointer() is an error or NULL.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/bpf/d1360219-85c3-4a03-9449-253ea905f9d1@moroto.mountain/
Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 56ce5008aedd..eb91cae0612a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2270,7 +2270,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
 	case BPF_DYNPTR_TYPE_XDP:
 	{
 		void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
-		if (xdp_ptr)
+		if (!IS_ERR_OR_NULL(xdp_ptr))
 			return xdp_ptr;
 
 		if (!buffer__opt)
-- 
2.34.1

Re: [PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[Yonghong Song]

On 8/3/23 4:12 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Verify if the pointer obtained from bpf_xdp_pointer() is either an error or
> NULL before returning it.
> 
> The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. Instead of
> solely checking for NULL, it should also verify if the pointer returned by
> bpf_xdp_pointer() is an error or NULL.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/bpf/d1360219-85c3-4a03-9449-253ea905f9d1@moroto.mountain/
> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[Kui-Feng Lee]

On 8/3/23 18:32, Yonghong Song wrote:
> 
> 
> On 8/3/23 4:12 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Verify if the pointer obtained from bpf_xdp_pointer() is either an 
>> error or
>> NULL before returning it.
>>
>> The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. 
>> Instead of
>> solely checking for NULL, it should also verify if the pointer 
>> returned by
>> bpf_xdp_pointer() is an error or NULL.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: 
>> https://lore.kernel.org/bpf/d1360219-85c3-4a03-9449-253ea905f9d1@moroto.mountain/
>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and 
>> bpf_dynptr_slice_rdwr")
>> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
Thanks!

Re: [PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Thu,  3 Aug 2023 16:12:06 -0700 you wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Verify if the pointer obtained from bpf_xdp_pointer() is either an error or
> NULL before returning it.
> 
> The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. Instead of
> solely checking for NULL, it should also verify if the pointer returned by
> bpf_xdp_pointer() is an error or NULL.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.
    https://git.kernel.org/bpf/bpf-next/c/5426700e6841

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[Martin KaFai Lau]

On 8/3/23 4:12 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Verify if the pointer obtained from bpf_xdp_pointer() is either an error or
> NULL before returning it.
> 
> The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. Instead of
> solely checking for NULL, it should also verify if the pointer returned by
> bpf_xdp_pointer() is an error or NULL.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/bpf/d1360219-85c3-4a03-9449-253ea905f9d1@moroto.mountain/
> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/helpers.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 56ce5008aedd..eb91cae0612a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2270,7 +2270,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
>   	case BPF_DYNPTR_TYPE_XDP:
>   	{
>   		void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
> -		if (xdp_ptr)
> +		if (!IS_ERR_OR_NULL(xdp_ptr))

Considering the earlier bpf_dynptr_check_off_len() should have avoided the 
IS_ERR() case here, I think targeting bpf-next makes sense. Applied.

Re: [PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[Kui-Feng Lee]

On 8/4/23 15:26, Martin KaFai Lau wrote:
> On 8/3/23 4:12 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Verify if the pointer obtained from bpf_xdp_pointer() is either an 
>> error or
>> NULL before returning it.
>>
>> The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. 
>> Instead of
>> solely checking for NULL, it should also verify if the pointer 
>> returned by
>> bpf_xdp_pointer() is an error or NULL.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: 
>> https://lore.kernel.org/bpf/d1360219-85c3-4a03-9449-253ea905f9d1@moroto.mountain/
>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and 
>> bpf_dynptr_slice_rdwr")
>> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/helpers.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 56ce5008aedd..eb91cae0612a 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -2270,7 +2270,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct 
>> bpf_dynptr_kern *ptr, u32 offset
>>       case BPF_DYNPTR_TYPE_XDP:
>>       {
>>           void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + 
>> offset, len);
>> -        if (xdp_ptr)
>> +        if (!IS_ERR_OR_NULL(xdp_ptr))
> 
> Considering the earlier bpf_dynptr_check_off_len() should have avoided 
> the IS_ERR() case here, I think targeting bpf-next makes sense. Applied.

It is a good point. I think the bpf_dynptr_check_off_len() check is
wrong as well. According to the behavior of the rest of the function,
it should be

     err = bpf_dynptr_check_off_len(ptr, ptr->offset + offset, len);

How do you think?

Re: [PATCH bpf-next] bpf: fix bpf_dynptr_slice() to stop return an ERR_PTR.

[Martin KaFai Lau]

On 8/7/23 10:07 AM, Kui-Feng Lee wrote:
> 
> 
> On 8/4/23 15:26, Martin KaFai Lau wrote:
>> On 8/3/23 4:12 PM, thinker.li@gmail.com wrote:
>>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>>
>>> Verify if the pointer obtained from bpf_xdp_pointer() is either an error or
>>> NULL before returning it.
>>>
>>> The function bpf_dynptr_slice() mistakenly returned an ERR_PTR. Instead of
>>> solely checking for NULL, it should also verify if the pointer returned by
>>> bpf_xdp_pointer() is an error or NULL.
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: 
>>> https://lore.kernel.org/bpf/d1360219-85c3-4a03-9449-253ea905f9d1@moroto.mountain/
>>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
>>> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>> ---
>>>   kernel/bpf/helpers.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>> index 56ce5008aedd..eb91cae0612a 100644
>>> --- a/kernel/bpf/helpers.c
>>> +++ b/kernel/bpf/helpers.c
>>> @@ -2270,7 +2270,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct 
>>> bpf_dynptr_kern *ptr, u32 offset
>>>       case BPF_DYNPTR_TYPE_XDP:
>>>       {
>>>           void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
>>> -        if (xdp_ptr)
>>> +        if (!IS_ERR_OR_NULL(xdp_ptr))
>>
>> Considering the earlier bpf_dynptr_check_off_len() should have avoided the 
>> IS_ERR() case here, I think targeting bpf-next makes sense. Applied.
> 
> It is a good point. I think the bpf_dynptr_check_off_len() check is
> wrong as well. According to the behavior of the rest of the function,
> it should be
> 
>      err = bpf_dynptr_check_off_len(ptr, ptr->offset + offset, len);

Not sure why it is needed either.
The bpf_dynptr_adjust() has updated the size after updating the offset.
Did I missing other offset update places?