[PATCH bpf-next] bpf, sockmap: fix double bpf_prog_put on error case in map_link

[PATCH bpf-next] bpf, sockmap: fix double bpf_prog_put on error case in map_link

[John Fastabend]

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(-)

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;
 }
 
-- 
2.33.0

Re: [PATCH bpf-next] bpf, sockmap: fix double bpf_prog_put on error case in map_link

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpf, sockmap: fix double bpf_prog_put on error case in map_link

[Jakub Sitnicki]

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>