Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

A memory leak vulnerability in mptcp #475

Open
eackkk opened this issue Apr 7, 2022 · 5 comments
Open

A memory leak vulnerability in mptcp #475

eackkk opened this issue Apr 7, 2022 · 5 comments
Labels

Comments

@eackkk
Copy link

eackkk commented Apr 7, 2022

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
Copy link
Member

matttbe commented Apr 7, 2022

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
Copy link
Author

eackkk commented Apr 8, 2022

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
Copy link
Member

matttbe commented Apr 8, 2022

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

mptcp/net/mptcp/mptcp_ctrl.c

Lines 1515 to 1521 in 08c571d

void mptcp_cleanup_rbuf(struct sock *meta_sk, int copied)
{
struct tcp_sock *meta_tp = tcp_sk(meta_sk);
struct sock *sk;
bool recheck_rcv_window = false;
__u32 rcv_window_now = 0;

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
Copy link
Author

eackkk commented Apr 11, 2022

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
Copy link
Member

matttbe commented Apr 19, 2022

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants