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

Commit Message

Kui-Feng Lee Aug. 3, 2023, 11:12 p.m. UTC

From: Kui-Feng Lee <[e-mail]>

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 <[e-mail]>
Closes: https://lore.kernel.org/bpf/[e-mail]/
Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
Suggested-by: Alexei Starovoitov <[e-mail]>
Signed-off-by: Kui-Feng Lee <[e-mail]>
---
 kernel/bpf/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yonghong Song Aug. 4, 2023, 1:32 a.m. UTC | #1

On 8/3/23 4:12 PM, [e-mail] wrote:
> From: Kui-Feng Lee <[e-mail]>
> 
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
> Suggested-by: Alexei Starovoitov <[e-mail]>
> Signed-off-by: Kui-Feng Lee <[e-mail]>

Acked-by: Yonghong Song <[e-mail]>

Kui-Feng Lee Aug. 4, 2023, 5:25 p.m. UTC | #2

On 8/3/23 18:32, Yonghong Song wrote:
> 
> 
> On 8/3/23 4:12 PM, [e-mail] wrote:
>> From: Kui-Feng Lee <[e-mail]>
>>
>> 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 <[e-mail]>
>> Closes: 
>> https://lore.kernel.org/bpf/[e-mail]/
>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and 
>> bpf_dynptr_slice_rdwr")
>> Suggested-by: Alexei Starovoitov <[e-mail]>
>> Signed-off-by: Kui-Feng Lee <[e-mail]>
> 
> Acked-by: Yonghong Song <[e-mail]>
Thanks!

[e-mail] Aug. 4, 2023, 10 p.m. UTC | #3

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <[e-mail]>:

On Thu,  3 Aug 2023 16:12:06 -0700 you wrote:
> From: Kui-Feng Lee <[e-mail]>
> 
> 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!

Martin KaFai Lau Aug. 4, 2023, 10:26 p.m. UTC | #4

On 8/3/23 4:12 PM, [e-mail] wrote:
> From: Kui-Feng Lee <[e-mail]>
> 
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
> Suggested-by: Alexei Starovoitov <[e-mail]>
> Signed-off-by: Kui-Feng Lee <[e-mail]>
> ---
>   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.

Kui-Feng Lee Aug. 7, 2023, 5:07 p.m. UTC | #5

On 8/4/23 15:26, Martin KaFai Lau wrote:
> On 8/3/23 4:12 PM, [e-mail] wrote:
>> From: Kui-Feng Lee <[e-mail]>
>>
>> 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 <[e-mail]>
>> Closes: 
>> https://lore.kernel.org/bpf/[e-mail]/
>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and 
>> bpf_dynptr_slice_rdwr")
>> Suggested-by: Alexei Starovoitov <[e-mail]>
>> Signed-off-by: Kui-Feng Lee <[e-mail]>
>> ---
>>   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?

Martin KaFai Lau Aug. 7, 2023, 7:49 p.m. UTC | #6

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, [e-mail] wrote:
>>> From: Kui-Feng Lee <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Closes: 
>>> https://lore.kernel.org/bpf/[e-mail]/
>>> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
>>> Suggested-by: Alexei Starovoitov <[e-mail]>
>>> Signed-off-by: Kui-Feng Lee <[e-mail]>
>>> ---
>>>   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?

Patch

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)