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

[Massimiliano Pellizzer]

On Thu, 5 Dec 2024 at 22:16, Magali Lemes <magali.lemes at canonical.com> wrote:
>
> 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?
>

The check is not necessary in this case, you are right.
However, I think that by keeping the check in place we will stay
closer to upstream,
and we may prevent future bugs in cases where we have to backport
other operations
that are checked using may_update_sockmap().

Thanks for the review
-- 
Massimiliano Pellizzer


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