[SRU][F][PATCH v2 1/1] bpf: Allow delete from sockmap/sockhash only if update is allowed

[Magali Lemes]

On 27/11/2024 12:13, Massimiliano Pellizzer wrote:
> From: Jakub Sitnicki <jakub at cloudflare.com>
> 
> We have seen an influx of syzkaller reports where a BPF program attached to
> a tracepoint triggers a locking rule violation by performing a map_delete
> on a sockmap/sockhash.
> 
> We don't intend to support this artificial use scenario. Extend the
> existing verifier allowed-program-type check for updating sockmap/sockhash
> to also cover deleting from a map.
> 
>  From now on only BPF programs which were previously allowed to update
> sockmap/sockhash can delete from these map types.
> 
> Fixes: ff9105993240 ("bpf, sockmap: Prevent lock inversion deadlock in map delete elem")
> Reported-by: Tetsuo Handa <penguin-kernel at i-love.sakura.ne.jp>
> Reported-by: syzbot+ec941d6e24f633a59172 at syzkaller.appspotmail.com
> Signed-off-by: Jakub Sitnicki <jakub at cloudflare.com>
> Signed-off-by: Daniel Borkmann <daniel at iogearbox.net>
> Tested-by: syzbot+ec941d6e24f633a59172 at syzkaller.appspotmail.com
> Acked-by: John Fastabend <john.fastabend at gmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=ec941d6e24f633a59172
> Link: https://lore.kernel.org/bpf/20240527-sockmap-verify-deletes-v1-1-944b372f2101@cloudflare.com
> (backported from commit 98e948fb60d41447fd8d2d0c3b8637fc6b6dc26d)
> [mpellizzer: the fix commit depends on the function may_update_sockmap(),
> which was introduced by 0126240f448d in mainline in v5.10. Other than
> may_update_sockmap(), commit 0126240f448d introduces also a new bpf
> feature which is not affecting the patch and which is not worth
> backporting. Therefore, while backporting the fix commit I also backpoerted
> the function may_update_sockmap() considering the differences in
> context.]
> CVE-2024-38662
> Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> ---
>   kernel/bpf/verifier.c | 40 ++++++++++++++++++++++++++++++++++++----
>   1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7776b1a6a24c..318f9c267ff4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3535,6 +3535,38 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
>   	return -EACCES;
>   }
>   
> +static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
> +{
> +	enum bpf_attach_type eatype = env->prog->expected_attach_type;
> +	enum bpf_prog_type type = env->prog->type;
> +
> +	if (func_id != BPF_FUNC_map_delete_elem)
> +		return false;
> +
> +	/* It's not possible to get access to a locked struct sock in these
> +	 * contexts, so updating is safe.
> +	 */
> +	switch (type) {
> +	case BPF_PROG_TYPE_SOCK_OPS:
> +		/* map_update allowed only via dedicated helpers with event type checks */
> +		if (func_id == BPF_FUNC_map_delete_elem)

This check here is not really necessary, right? Because we have `if 
(func_id != BPF_FUNC_map_delete_elem)` in the beginning already. I guess 
it'd make sense if we had `BPF_FUNC_map_update_elem` as well, but since 
we don't maybe, in this context, it'd be cleaner to just not have this 
here. Even the comment mentioning map_update could be discarded because 
it doesn't apply. Thoughts?

> +			return true;
> +		break;
> +	case BPF_PROG_TYPE_SOCKET_FILTER:
> +	case BPF_PROG_TYPE_SCHED_CLS:
> +	case BPF_PROG_TYPE_SCHED_ACT:
> +	case BPF_PROG_TYPE_XDP:
> +	case BPF_PROG_TYPE_SK_REUSEPORT:
> +	case BPF_PROG_TYPE_FLOW_DISSECTOR:
> +		return true;
> +	default:
> +		break;
> +	}
> +
> +	verbose(env, "cannot update sockmap in this context\n");
> +	return false;
> +}
> +
>   static int check_map_func_compatibility(struct bpf_verifier_env *env,
>   					struct bpf_map *map, int func_id)
>   {
> @@ -3593,15 +3625,15 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>   	case BPF_MAP_TYPE_SOCKMAP:
>   		if (func_id != BPF_FUNC_sk_redirect_map &&
>   		    func_id != BPF_FUNC_sock_map_update &&
> -		    func_id != BPF_FUNC_map_delete_elem &&
> -		    func_id != BPF_FUNC_msg_redirect_map)
> +		    func_id != BPF_FUNC_msg_redirect_map &&
> +		    !may_update_sockmap(env, func_id))
>   			goto error;
>   		break;
>   	case BPF_MAP_TYPE_SOCKHASH:
>   		if (func_id != BPF_FUNC_sk_redirect_hash &&
>   		    func_id != BPF_FUNC_sock_hash_update &&
> -		    func_id != BPF_FUNC_map_delete_elem &&
> -		    func_id != BPF_FUNC_msg_redirect_hash)
> +		    func_id != BPF_FUNC_msg_redirect_hash &&
> +		    !may_update_sockmap(env, func_id))
>   			goto error;
>   		break;
>   	case BPF_MAP_TYPE_REUSEPORT_SOCKARRAY: