Commit f5d83cf0 authored by Matt Johnston's avatar Matt Johnston Committed by Paolo Abeni
Browse files

net: mctp: unshare packets when reassembling 



Ensure that the frag_list used for reassembly isn't shared with other
packets. This avoids incorrect reassembly when packets are cloned, and
prevents a memory leak due to circular references between fragments and
their skb_shared_info.

The upcoming MCTP-over-USB driver uses skb_clone which can trigger the
problem - other MCTP drivers don't share SKBs.

A kunit test is added to reproduce the issue.

Signed-off-by: default avatarMatt Johnston <matt@codeconstruct.com.au>
Fixes: 4a992bbd ("mctp: Implement message fragmentation & reassembly")
Reviewed-by: default avatarSimon Horman <horms@kernel.org>
Link: https://patch.msgid.link/20250306-matt-mctp-usb-v1-1-085502b3dd28@codeconstruct.com.au


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parent 62531a1e

net/mctp/route.c

+8 −2
Original line number Diff line number Diff line
@@ -332,8 +332,14 @@ static int mctp_frag_queue(struct mctp_sk_key *key, struct sk_buff *skb) 
		& MCTP_HDR_SEQ_MASK;



	if (!key->reasm_head) {

		key->reasm_head = skb;

		key->reasm_tailp = &(skb_shinfo(skb)->frag_list);

		/* Since we're manipulating the shared frag_list, ensure it isn't

		 * shared with any other SKBs.

		 */

		key->reasm_head = skb_unshare(skb, GFP_ATOMIC);

		if (!key->reasm_head)

			return -ENOMEM;



		key->reasm_tailp = &(skb_shinfo(key->reasm_head)->frag_list);

		key->last_seq = this_seq;

		return 0;

	}

net/mctp/test/route-test.c

+109 −0
Original line number Diff line number Diff line
@@ -921,6 +921,114 @@ static void mctp_test_route_input_sk_fail_frag(struct kunit *test) 
	__mctp_route_test_fini(test, dev, rt, sock);

}



/* Input route to socket, using a fragmented message created from clones.

 */

static void mctp_test_route_input_cloned_frag(struct kunit *test)

{

	/* 5 packet fragments, forming 2 complete messages */

	const struct mctp_hdr hdrs[5] = {

		RX_FRAG(FL_S, 0),

		RX_FRAG(0, 1),

		RX_FRAG(FL_E, 2),

		RX_FRAG(FL_S, 0),

		RX_FRAG(FL_E, 1),

	};

	struct mctp_test_route *rt;

	struct mctp_test_dev *dev;

	struct sk_buff *skb[5];

	struct sk_buff *rx_skb;

	struct socket *sock;

	size_t data_len;

	u8 compare[100];

	u8 flat[100];

	size_t total;

	void *p;

	int rc;



	/* Arbitrary length */

	data_len = 3;

	total = data_len + sizeof(struct mctp_hdr);



	__mctp_route_test_init(test, &dev, &rt, &sock, MCTP_NET_ANY);



	/* Create a single skb initially with concatenated packets */

	skb[0] = mctp_test_create_skb(&hdrs[0], 5 * total);

	mctp_test_skb_set_dev(skb[0], dev);

	memset(skb[0]->data, 0 * 0x11, skb[0]->len);

	memcpy(skb[0]->data, &hdrs[0], sizeof(struct mctp_hdr));



	/* Extract and populate packets */

	for (int i = 1; i < 5; i++) {

		skb[i] = skb_clone(skb[i - 1], GFP_ATOMIC);

		KUNIT_ASSERT_TRUE(test, skb[i]);

		p = skb_pull(skb[i], total);

		KUNIT_ASSERT_TRUE(test, p);

		skb_reset_network_header(skb[i]);

		memcpy(skb[i]->data, &hdrs[i], sizeof(struct mctp_hdr));

		memset(&skb[i]->data[sizeof(struct mctp_hdr)], i * 0x11, data_len);

	}

	for (int i = 0; i < 5; i++)

		skb_trim(skb[i], total);



	/* SOM packets have a type byte to match the socket */

	skb[0]->data[4] = 0;

	skb[3]->data[4] = 0;



	skb_dump("pkt1 ", skb[0], false);

	skb_dump("pkt2 ", skb[1], false);

	skb_dump("pkt3 ", skb[2], false);

	skb_dump("pkt4 ", skb[3], false);

	skb_dump("pkt5 ", skb[4], false);



	for (int i = 0; i < 5; i++) {

		KUNIT_EXPECT_EQ(test, refcount_read(&skb[i]->users), 1);

		/* Take a reference so we can check refcounts at the end */

		skb_get(skb[i]);

	}



	/* Feed the fragments into MCTP core */

	for (int i = 0; i < 5; i++) {

		rc = mctp_route_input(&rt->rt, skb[i]);

		KUNIT_EXPECT_EQ(test, rc, 0);

	}



	/* Receive first reassembled message */

	rx_skb = skb_recv_datagram(sock->sk, MSG_DONTWAIT, &rc);

	KUNIT_EXPECT_EQ(test, rc, 0);

	KUNIT_EXPECT_EQ(test, rx_skb->len, 3 * data_len);

	rc = skb_copy_bits(rx_skb, 0, flat, rx_skb->len);

	for (int i = 0; i < rx_skb->len; i++)

		compare[i] = (i / data_len) * 0x11;

	/* Set type byte */

	compare[0] = 0;



	KUNIT_EXPECT_MEMEQ(test, flat, compare, rx_skb->len);

	KUNIT_EXPECT_EQ(test, refcount_read(&rx_skb->users), 1);

	kfree_skb(rx_skb);



	/* Receive second reassembled message */

	rx_skb = skb_recv_datagram(sock->sk, MSG_DONTWAIT, &rc);

	KUNIT_EXPECT_EQ(test, rc, 0);

	KUNIT_EXPECT_EQ(test, rx_skb->len, 2 * data_len);

	rc = skb_copy_bits(rx_skb, 0, flat, rx_skb->len);

	for (int i = 0; i < rx_skb->len; i++)

		compare[i] = (i / data_len + 3) * 0x11;

	/* Set type byte */

	compare[0] = 0;



	KUNIT_EXPECT_MEMEQ(test, flat, compare, rx_skb->len);

	KUNIT_EXPECT_EQ(test, refcount_read(&rx_skb->users), 1);

	kfree_skb(rx_skb);



	/* Check input skb refcounts */

	for (int i = 0; i < 5; i++) {

		KUNIT_EXPECT_EQ(test, refcount_read(&skb[i]->users), 1);

		kfree_skb(skb[i]);

	}



	__mctp_route_test_fini(test, dev, rt, sock);

}



#if IS_ENABLED(CONFIG_MCTP_FLOWS)



static void mctp_test_flow_init(struct kunit *test,
@@ -1144,6 +1252,7 @@ static struct kunit_case mctp_test_cases[] = { 
	KUNIT_CASE(mctp_test_packet_flow),

	KUNIT_CASE(mctp_test_fragment_flow),

	KUNIT_CASE(mctp_test_route_output_key_create),

	KUNIT_CASE(mctp_test_route_input_cloned_frag),

	{}

};