multipath-tcp / mptcp

⚠️⚠️⚠️ Deprecated 🚫 Out-of-tree Linux Kernel implementation of MultiPath TCP. 👉 Use https://github.com/multipath-tcp/mptcp_net-next repo instead ⚠️⚠️⚠️
https://github.com/multipath-tcp/mptcp_net-next
Other
889 stars 336 forks source link

A memory leak vulnerability in mptcp #475

Open eackkk opened 2 years ago

eackkk commented 2 years ago

Hi, I found a memory leak in mptcp that causes a kernel panic, my test version is v0.94 and the kernel architecture is aarch64. See the attachment for report.txt. The details of the vulnerability are as follows:

void mptcp_del_sock(struct sock *sk)
{
    struct tcp_sock *tp = tcp_sk(sk), *tp_prev;
    struct mptcp_cb *mpcb;
    struct mptcp_deleted_flow_info *flow_info = NULL;
    struct dst_entry *dst = NULL;
    ...
    if (!sock_flag(tp->meta_sk, SOCK_DEAD)) {
        flow_info = kmalloc(sizeof(*flow_info), GFP_ATOMIC);          // <--- here!
        if (flow_info) {
            flow_info->loc_id = tp->mptcp->loc_id;
            dst = sk_dst_get(sk);
            if (dst) {
                (void)strncpy(flow_info->ifname, dst->dev->name, IFNAMSIZ);
                flow_info->ifname[IFNAMSIZ - 1] = '\0';
                dst_release(dst);
            } else {
                flow_info->ifname[0] = '\0';
            }

            flow_info->tcpi_bytes_acked = tp->bytes_acked;
            flow_info->tcpi_bytes_received = tp->bytes_received;
            list_add(&flow_info->node, &mpcb->deleted_flow_info);
        }
    }
        ...
}

If you have any questions, please contact me, best wishes!

matttbe commented 2 years ago

Hi @eackkk,

Thank you for the bug report.

Here! if mpcb->master_info == NULL, not kfree

I'm not sure to understand your comment: if master_info is NULL, no need to free anything. Do you mean the opposite? i.e. if it is not NULL, it is not freed? It is freed when the mpcb is freed so it should be OK. Except if there is a bug and the mpcb is not freed but that would be strange.

Can you easily reproduce this issue? Are you sure the MPTCP connection linked to the subflow that was closed was also closed when kmemleak emitted its report?

I wonder if kmemleak is not confused here: we are in a situation where the initial subflow is being closed while the MPTCP connection is still alive. Some memory is reserved: not in the subflow data but in the MPTCP connection data. Maybe kmemleak thinks it is attached to the subflow level and not to the MPTCP level?

eackkk commented 2 years ago

Hi @matttbe,

Thanks for your prompt reply.

Yes, I made a small mistake, I posted the wrong code, you can see the backtrace in report.txt, the vulnerability is in mptcp_del_sock+0x264/0x628 net/mptcp/mptcp_ctrl.c:1519.

flow_info is not released, which eventually leads to a memory leak.

Apologize again for my mistake.

matttbe commented 2 years ago

Which version are you using? Here is what we can see on mptcp_v0.94 branch around line 1519:

https://github.com/multipath-tcp/mptcp/blob/08c571d0bb7622c1b6f23e7b3eb42cd6f85ad86d/net/mptcp/mptcp_ctrl.c#L1515-L1521

flow_info is not released, which eventually leads to a memory leak.

Are you sure it is not released when all MPTCP connections are closed? It is maybe not released when kmemleak is checking memory associated to a certain subflow/slab but it will be released later when the linked MPTCP connection will be closed, no?

eackkk commented 2 years ago

Hey! My version is 0.94.7, In the mptcp_init function, it shows that it is v0.94.7:

image

and the Makefile is:

# SPDX-License-Identifier: GPL-2.0
VERSION = 4
PATCHLEVEL = 14
SUBLEVEL = 265
EXTRAVERSION =
NAME = Petit Gorille

As I can see in the code it is only used in mptcp_ctrl.c but not kfree:

Screenshot_20220411_100633

matttbe commented 2 years ago

I cannot find this code in mptcp_ctrl.c in v0.94.7 version: https://github.com/multipath-tcp/mptcp/blob/v0.94.7/net/mptcp/mptcp_ctrl.c