• Daniel Borkmann's avatar
    bpf, sockmap: fix leakage of smap_psock_map_entry · 2262b26d
    Daniel Borkmann authored
    [ Upstream commit d40b0116 ]
    
    While working on sockmap I noticed that we do not always kfree the
    struct smap_psock_map_entry list elements which track psocks attached
    to maps. In the case of sock_hash_ctx_update_elem(), these map entries
    are allocated outside of __sock_map_ctx_update_elem() with their
    linkage to the socket hash table filled. In the case of sock array,
    the map entries are allocated inside of __sock_map_ctx_update_elem()
    and added with their linkage to the psock->maps. Both additions are
    under psock->maps_lock each.
    
    Now, we drop these elements from their psock->maps list in a few
    occasions: i) in sock array via smap_list_map_remove() when an entry
    is either deleted from the map from user space, or updated via
    user space or BPF program where we drop the old socket at that map
    slot, or the sock array is freed via sock_map_free() and drops all
    its elements; ii) for sock hash via smap_list_hash_remove() in exactly
    the same occasions as just described for sock array; iii) in the
    bpf_tcp_close() where we remove the elements from the list via
    psock_map_pop() and iterate over them dropping themselves from either
    sock array or sock hash; and last but not least iv) once again in
    smap_gc_work() which is a callback for deferring the work once the
    psock refcount hit zero and thus the socket is being destroyed.
    
    Problem is that the only case where we kfree() the list entry is
    in case iv), which at that point should have an empty list in
    normal cases. So in cases from i) to iii) we unlink the elements
    without freeing where they go out of reach from us. Hence fix is
    to properly kfree() them as well to stop the leakage. Given these
    are all handled under psock->maps_lock there is no need for deferred
    RCU freeing.
    
    I later also ran with kmemleak detector and it confirmed the finding
    as well where in the state before the fix the object goes unreferenced
    while after the patch no kmemleak report related to BPF showed up.
    
      [...]
      unreferenced object 0xffff880378eadae0 (size 64):
        comm "test_sockmap", pid 2225, jiffies 4294720701 (age 43.504s)
        hex dump (first 32 bytes):
          00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de  ................
          50 4d 75 5d 03 88 ff ff 00 00 00 00 00 00 00 00  PMu]............
        backtrace:
          [<000000005225ac3c>] sock_map_ctx_update_elem.isra.21+0xd8/0x210
          [<0000000045dd6d3c>] bpf_sock_map_update+0x29/0x60
          [<00000000877723aa>] ___bpf_prog_run+0x1e1f/0x4960
          [<000000002ef89e83>] 0xffffffffffffffff
      unreferenced object 0xffff880378ead240 (size 64):
        comm "test_sockmap", pid 2225, jiffies 4294720701 (age 43.504s)
        hex dump (first 32 bytes):
          00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de  ................
          00 44 75 5d 03 88 ff ff 00 00 00 00 00 00 00 00  .Du]............
        backtrace:
          [<000000005225ac3c>] sock_map_ctx_update_elem.isra.21+0xd8/0x210
          [<0000000030e37a3a>] sock_map_update_elem+0x125/0x240
          [<000000002e5ce36e>] map_update_elem+0x4eb/0x7b0
          [<00000000db453cc9>] __x64_sys_bpf+0x1f9/0x360
          [<0000000000763660>] do_syscall_64+0x9a/0x300
          [<00000000422a2bb2>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
          [<000000002ef89e83>] 0xffffffffffffffff
      [...]
    
    Fixes: e9db4ef6 ("bpf: sockhash fix omitted bucket lock in sock_close")
    Fixes: 54fedb42 ("bpf: sockmap, fix smap_list_map_remove when psock is in many maps")
    Fixes: 2f857d04 ("bpf: sockmap, remove STRPARSER map_flags and add multi-map support")
    Signed-off-by: 's avatarDaniel Borkmann <daniel@iogearbox.net>
    Acked-by: 's avatarJohn Fastabend <john.fastabend@gmail.com>
    Acked-by: 's avatarSong Liu <songliubraving@fb.com>
    Signed-off-by: 's avatarAlexei Starovoitov <ast@kernel.org>
    Signed-off-by: 's avatarSasha Levin <alexander.levin@microsoft.com>
    Signed-off-by: 's avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    2262b26d
sockmap.c 59.7 KB