[bpf] sock_map: fix a NULL pointer dereference in sock_map_link_update_prog()

Commit Message

Cong Wang Oct. 26, 2024, 6:55 p.m. UTC

From: Cong Wang <[e-mail]>

The following race condition could trigger a NULL pointer dereference:

sock_map_link_detach():		sock_map_link_update_prog():
   mutex_lock(&sockmap_mutex);
   ...
   sockmap_link->map = NULL;
   mutex_unlock(&sockmap_mutex);
   				   mutex_lock(&sockmap_mutex);
				   ...
				   sock_map_prog_link_lookup(sockmap_link->map);
				   mutex_unlock(&sockmap_mutex);
   <continue>

Fix it by adding a NULL pointer check. In this specific case, it makes
no sense to update a link which is being released.

Reported-by: Ruan Bonan <[e-mail]>
Fixes: 699c23f02c65 ("bpf: Add bpf_link support for sk_msg and sk_skb progs")
Cc: Yonghong Song <[e-mail]>
Cc: John Fastabend <[e-mail]>
Cc: Jakub Sitnicki <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 net/core/sock_map.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Yonghong Song Oct. 28, 2024, 5:58 a.m. UTC | #1

On 10/26/24 11:55 AM, Cong Wang wrote:
> From: Cong Wang <[e-mail]>
>
> The following race condition could trigger a NULL pointer dereference:
>
> sock_map_link_detach():		sock_map_link_update_prog():
>     mutex_lock(&sockmap_mutex);
>     ...
>     sockmap_link->map = NULL;
>     mutex_unlock(&sockmap_mutex);
>     				   mutex_lock(&sockmap_mutex);
> 				   ...
> 				   sock_map_prog_link_lookup(sockmap_link->map);
> 				   mutex_unlock(&sockmap_mutex);
>     <continue>
>
> Fix it by adding a NULL pointer check. In this specific case, it makes
> no sense to update a link which is being released.
>
> Reported-by: Ruan Bonan <[e-mail]>
> Fixes: 699c23f02c65 ("bpf: Add bpf_link support for sk_msg and sk_skb progs")
> Cc: Yonghong Song <[e-mail]>
> Cc: John Fastabend <[e-mail]>
> Cc: Jakub Sitnicki <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>
> ---
>   net/core/sock_map.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 07d6aa4e39ef..9fca4db52f57 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -1760,6 +1760,10 @@ static int sock_map_link_update_prog(struct bpf_link *link,
>   		ret = -EINVAL;
>   		goto out;
>   	}
> +	if (!sockmap_link->map) {
> +		ret = -EINVAL;

Thanks for the fix. Maybe we should use -ENOENT as the return error code?
In this case, update_prog failed due to sockmap_link->map == NULL which is
equivalent to no 'entry' to update.

> +		goto out;
> +	}
>   
>   	ret = sock_map_prog_link_lookup(sockmap_link->map, &pprog, &plink,
>   					sockmap_link->attach_type);

Martin KaFai Lau Oct. 29, 2024, 1:57 a.m. UTC | #2

On 10/27/24 10:58 PM, Yonghong Song wrote:
> 
> On 10/26/24 11:55 AM, Cong Wang wrote:
>> From: Cong Wang <[e-mail]>
>>
>> The following race condition could trigger a NULL pointer dereference:
>>
>> sock_map_link_detach():        sock_map_link_update_prog():
>>     mutex_lock(&sockmap_mutex);
>>     ...
>>     sockmap_link->map = NULL;
>>     mutex_unlock(&sockmap_mutex);
>>                        mutex_lock(&sockmap_mutex);
>>                    ...
>>                    sock_map_prog_link_lookup(sockmap_link->map);
>>                    mutex_unlock(&sockmap_mutex);
>>     <continue>
>>
>> Fix it by adding a NULL pointer check. In this specific case, it makes
>> no sense to update a link which is being released.
>>
>> Reported-by: Ruan Bonan <[e-mail]>
>> Fixes: 699c23f02c65 ("bpf: Add bpf_link support for sk_msg and sk_skb progs")
>> Cc: Yonghong Song <[e-mail]>
>> Cc: John Fastabend <[e-mail]>
>> Cc: Jakub Sitnicki <[e-mail]>
>> Signed-off-by: Cong Wang <[e-mail]>
>> ---
>>   net/core/sock_map.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
>> index 07d6aa4e39ef..9fca4db52f57 100644
>> --- a/net/core/sock_map.c
>> +++ b/net/core/sock_map.c
>> @@ -1760,6 +1760,10 @@ static int sock_map_link_update_prog(struct bpf_link 
>> *link,
>>           ret = -EINVAL;
>>           goto out;
>>       }
>> +    if (!sockmap_link->map) {
>> +        ret = -EINVAL;
> 
> Thanks for the fix. Maybe we should use -ENOENT as the return error code?
> In this case, update_prog failed due to sockmap_link->map == NULL which is
> equivalent to no 'entry' to update.

The fix lgtm. Regarding the error value, the tcx/bpf_struct_ops/cgroup's 
update_prog uses -ENOLINK. I changed it to -ENOLINK for consistency. Applied. 
Thanks.

> 
>> +        goto out;
>> +    }
>>       ret = sock_map_prog_link_lookup(sockmap_link->map, &pprog, &plink,
>>                       sockmap_link->attach_type);
>

[e-mail] Oct. 29, 2024, 2 a.m. UTC | #3

Hello:

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

On Sat, 26 Oct 2024 11:55:22 -0700 you wrote:
> From: Cong Wang <[e-mail]>
> 
> The following race condition could trigger a NULL pointer dereference:
> 
> sock_map_link_detach():		sock_map_link_update_prog():
>    mutex_lock(&sockmap_mutex);
>    ...
>    sockmap_link->map = NULL;
>    mutex_unlock(&sockmap_mutex);
>    				   mutex_lock(&sockmap_mutex);
> 				   ...
> 				   sock_map_prog_link_lookup(sockmap_link->map);
> 				   mutex_unlock(&sockmap_mutex);
>    <continue>
> 
> [...]

Here is the summary with links:
  - [bpf] sock_map: fix a NULL pointer dereference in sock_map_link_update_prog()
    https://git.kernel.org/bpf/bpf/c/740be3b9a6d7

You are awesome, thank you!

Patch

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 07d6aa4e39ef..9fca4db52f57 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1760,6 +1760,10 @@  static int sock_map_link_update_prog(struct bpf_link *link,
 		ret = -EINVAL;
 		goto out;
 	}
+	if (!sockmap_link->map) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = sock_map_prog_link_lookup(sockmap_link->map, &pprog, &plink,
 					sockmap_link->attach_type);