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

[Juerg Haefliger]

On Wed, 27 Nov 2024 16:13:27 +0100
Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com> 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.]

This looks incorrect. The code is mostly from 0126240f448d but you're calling
it a backport of 98e948fb60d41447fd8d2d0c3b8637fc6b6dc26d. You need to add a
dedicated patch (partial backport of 0126240f448d) as a prereq. It's easier
to review/read and it also puts the sha 0126240f448d in the git log so that
we'll notice if there's a follow up fix for 0126240f448d.

With your current patch we'll never notice if there's fixup for
may_update_sockmap().

...Juerg


> 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)
> +			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:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20250205/e2a90275/attachment.sig>