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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <874k6epbd0.fsf@cloudflare.com>
Date:   Sat, 08 Jan 2022 15:10:03 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
        bpf@...r.kernel.org, joamaki@...il.com
Subject: Re: [PATCH bpf-next] bpf, sockmap: fix double bpf_prog_put on error
 case in map_link

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@...kaller.appspotmail.com
> Fixes: 38207a5e8123 ("bpf, sockmap: Attach map progs to psock early for feature probes")
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---

FWIW, late :thumbup:

Reviewed-by: Jakub Sitnicki <jakub@...udflare.com>