| Message ID | 20220104214645.290900-1-john.fastabend@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 218d747a4142f281a256687bb513a135c905867b |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf, sockmap: fix double bpf_prog_put on error case in map_link | expand |
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 4 Jan 2022 13:46:45 -0800 you wrote: > 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.) > > [...] Here is the summary with links: - [bpf-next] bpf, sockmap: fix double bpf_prog_put on error case in map_link https://git.kernel.org/bpf/bpf-next/c/218d747a4142 You are awesome, thank you!
On Tue, Jan 04, 2022 at 10:46 PM CET, John Fastabend wrote: > 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 assignemnt > > 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. > > Reported-by: syzbot+bb73e71cf4b8fd376a4f@syzkaller.appspotmail.com > Fixes: 38207a5e8123 ("bpf, sockmap: Attach map progs to psock early for feature probes") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- FWIW, late :thumbup: Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 9618ab6d7cc9..1827669eedd6 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 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk) out_put_stream_verdict: if (stream_verdict) bpf_prog_put(stream_verdict); +out: return ret; }
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 assignemnt 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. Reported-by: syzbot+bb73e71cf4b8fd376a4f@syzkaller.appspotmail.com Fixes: 38207a5e8123 ("bpf, sockmap: Attach map progs to psock early for feature probes") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- Applies against both bpf and bpf-next but labeled for bpf-next given we are in rc8 now. net/core/sock_map.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)