lkml.org 
[lkml]   [2022]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.15 310/846] bpf, sockmap: Fix double bpf_prog_put on error case in map_link
    Date
    From: John Fastabend <john.fastabend@gmail.com>

    [ Upstream commit 218d747a4142f281a256687bb513a135c905867b ]

    sock_map_link() is called to update a sockmap entry with a sk. But, if the
    sock_map_init_proto() call fails then we return an error to the map_update
    op against the sockmap. In the error path though we need to cleanup psock
    and dec the refcnt on any programs associated with the map, because we
    refcnt them early in the update process to ensure they are pinned for the
    psock. (This avoids a race where user deletes programs while also updating
    the map with new socks.)

    In current code we do the prog refcnt dec explicitely by calling
    bpf_prog_put() when the program was found in the map. But, after commit
    '38207a5e81230' in this error path we've already done the prog to psock
    assignment so the programs have a reference from the psock as well. This
    then causes the psock tear down logic, invoked by sk_psock_put() in the
    error path, to similarly call bpf_prog_put on the programs there.

    To be explicit this logic does the prog->psock assignment:

    if (msg_*)
    psock_set_prog(...)

    Then the error path under the out_progs label does a similar check and
    dec with:

    if (msg_*)
    bpf_prog_put(...)

    And the teardown logic sk_psock_put() does ...

    psock_set_prog(msg_*, NULL)

    ... triggering another bpf_prog_put(...). Then KASAN gives us this splat,
    found by syzbot because we've created an inbalance between bpf_prog_inc and
    bpf_prog_put calling put twice on the program.

    BUG: KASAN: vmalloc-out-of-bounds in __bpf_prog_put kernel/bpf/syscall.c:1812 [inline]
    BUG: KASAN: vmalloc-out-of-bounds in __bpf_prog_put kernel/bpf/syscall.c:1812 [inline] kernel/bpf/syscall.c:1829
    BUG: KASAN: vmalloc-out-of-bounds in bpf_prog_put+0x8c/0x4f0 kernel/bpf/syscall.c:1829 kernel/bpf/syscall.c:1829
    Read of size 8 at addr ffffc90000e76038 by task syz-executor020/3641

    To fix clean up error path so it doesn't try to do the bpf_prog_put in the
    error path once progs are assigned then it relies on the normal psock
    tear down logic to do complete cleanup.

    For completness we also cover the case whereh sk_psock_init_strp() fails,
    but this is not expected because it indicates an incorrect socket type
    and should be caught earlier.

    Fixes: 38207a5e8123 ("bpf, sockmap: Attach map progs to psock early for feature probes")
    Reported-by: syzbot+bb73e71cf4b8fd376a4f@syzkaller.appspotmail.com
    Signed-off-by: John Fastabend <john.fastabend@gmail.com>
    Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
    Link: https://lore.kernel.org/bpf/20220104214645.290900-1-john.fastabend@gmail.com
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    net/core/sock_map.c | 21 +++++++++++++--------
    1 file changed, 13 insertions(+), 8 deletions(-)

    diff --git a/net/core/sock_map.c b/net/core/sock_map.c
    index c89f527411e84..8288b5382f08d 100644
    --- a/net/core/sock_map.c
    +++ b/net/core/sock_map.c
    @@ -292,15 +292,23 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
    if (skb_verdict)
    psock_set_prog(&psock->progs.skb_verdict, skb_verdict);

    + /* msg_* and stream_* programs references tracked in psock after this
    + * point. Reference dec and cleanup will occur through psock destructor
    + */
    ret = sock_map_init_proto(sk, psock);
    - if (ret < 0)
    - goto out_drop;
    + if (ret < 0) {
    + sk_psock_put(sk, psock);
    + goto out;
    + }

    write_lock_bh(&sk->sk_callback_lock);
    if (stream_parser && stream_verdict && !psock->saved_data_ready) {
    ret = sk_psock_init_strp(sk, psock);
    - if (ret)
    - goto out_unlock_drop;
    + if (ret) {
    + write_unlock_bh(&sk->sk_callback_lock);
    + sk_psock_put(sk, psock);
    + goto out;
    + }
    sk_psock_start_strp(sk, psock);
    } else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
    sk_psock_start_verdict(sk,psock);
    @@ -309,10 +317,6 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
    }
    write_unlock_bh(&sk->sk_callback_lock);
    return 0;
    -out_unlock_drop:
    - write_unlock_bh(&sk->sk_callback_lock);
    -out_drop:
    - sk_psock_put(sk, psock);
    out_progs:
    if (skb_verdict)
    bpf_prog_put(skb_verdict);
    @@ -325,6 +329,7 @@ out_put_stream_parser:
    out_put_stream_verdict:
    if (stream_verdict)
    bpf_prog_put(stream_verdict);
    +out:
    return ret;
    }

    --
    2.34.1


    \
     
     \ /
      Last update: 2022-01-25 00:11    [W:2.354 / U:0.176 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site