Commit ce1219c3 authored by Jeremy Kerr's avatar Jeremy Kerr Committed by Paolo Abeni
Browse files

net: mctp: handle skb cleanup on sock_queue failures 



Currently, we don't use the return value from sock_queue_rcv_skb, which
means we may leak skbs if a message is not successfully queued to a
socket.

Instead, ensure that we're freeing the skb where the sock hasn't
otherwise taken ownership of the skb by adding checks on the
sock_queue_rcv_skb() to invoke a kfree on failure.

In doing so, rather than using the 'rc' value to trigger the
kfree_skb(), use the skb pointer itself, which is more explicit.

Also, add a kunit test for the sock delivery failure cases.

Fixes: 4a992bbd ("mctp: Implement message fragmentation & reassembly")
Cc: stable@vger.kernel.org
Signed-off-by: default avatarJeremy Kerr <jk@codeconstruct.com.au>
Link: https://patch.msgid.link/20241218-mctp-next-v2-1-1c1729645eaa@codeconstruct.com.au


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

net/mctp/route.c

+26 −10
Original line number Diff line number Diff line
@@ -374,8 +374,13 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
	msk = NULL;

	rc = -EINVAL;



	/* we may be receiving a locally-routed packet; drop source sk

	 * accounting

	/* We may be receiving a locally-routed packet; drop source sk

	 * accounting.

	 *

	 * From here, we will either queue the skb - either to a frag_queue, or

	 * to a receiving socket. When that succeeds, we clear the skb pointer;

	 * a non-NULL skb on exit will be otherwise unowned, and hence

	 * kfree_skb()-ed.

	 */

	skb_orphan(skb);
@@ -434,7 +439,9 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
		 * pending key.

		 */

		if (flags & MCTP_HDR_FLAG_EOM) {

			sock_queue_rcv_skb(&msk->sk, skb);

			rc = sock_queue_rcv_skb(&msk->sk, skb);

			if (!rc)

				skb = NULL;

			if (key) {

				/* we've hit a pending reassembly; not much we

				 * can do but drop it
@@ -443,7 +450,6 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
						   MCTP_TRACE_KEY_REPLIED);

				key = NULL;

			}

			rc = 0;

			goto out_unlock;

		}
@@ -470,8 +476,10 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
			 * this function.

			 */

			rc = mctp_key_add(key, msk);

			if (!rc)

			if (!rc) {

				trace_mctp_key_acquire(key);

				skb = NULL;

			}



			/* we don't need to release key->lock on exit, so

			 * clean up here and suppress the unlock via
@@ -489,6 +497,8 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
				key = NULL;

			} else {

				rc = mctp_frag_queue(key, skb);

				if (!rc)

					skb = NULL;

			}

		}
@@ -503,11 +513,18 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
		else

			rc = mctp_frag_queue(key, skb);



		if (rc)

			goto out_unlock;



		/* we've queued; the queue owns the skb now */

		skb = NULL;



		/* end of message? deliver to socket, and we're done with

		 * the reassembly/response key

		 */

		if (!rc && flags & MCTP_HDR_FLAG_EOM) {

			sock_queue_rcv_skb(key->sk, key->reasm_head);

		if (flags & MCTP_HDR_FLAG_EOM) {

			rc = sock_queue_rcv_skb(key->sk, key->reasm_head);

			if (!rc)

				key->reasm_head = NULL;

			__mctp_key_done_in(key, net, f, MCTP_TRACE_KEY_REPLIED);

			key = NULL;
@@ -527,7 +544,6 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb) 
	if (any_key)

		mctp_key_unref(any_key);

out:

	if (rc)

	kfree_skb(skb);

	return rc;

}

net/mctp/test/route-test.c

+86 −0
Original line number Diff line number Diff line
@@ -837,6 +837,90 @@ static void mctp_test_route_input_multiple_nets_key(struct kunit *test) 
	mctp_test_route_input_multiple_nets_key_fini(test, &t2);

}



/* Input route to socket, using a single-packet message, where sock delivery

 * fails. Ensure we're handling the failure appropriately.

 */

static void mctp_test_route_input_sk_fail_single(struct kunit *test)

{

	const struct mctp_hdr hdr = RX_HDR(1, 10, 8, FL_S | FL_E | FL_TO);

	struct mctp_test_route *rt;

	struct mctp_test_dev *dev;

	struct socket *sock;

	struct sk_buff *skb;

	int rc;



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



	/* No rcvbuf space, so delivery should fail. __sock_set_rcvbuf will

	 * clamp the minimum to SOCK_MIN_RCVBUF, so we open-code this.

	 */

	lock_sock(sock->sk);

	WRITE_ONCE(sock->sk->sk_rcvbuf, 0);

	release_sock(sock->sk);



	skb = mctp_test_create_skb(&hdr, 10);

	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, skb);

	skb_get(skb);



	mctp_test_skb_set_dev(skb, dev);



	/* do route input, which should fail */

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

	KUNIT_EXPECT_NE(test, rc, 0);



	/* we should hold the only reference to skb */

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

	kfree_skb(skb);



	__mctp_route_test_fini(test, dev, rt, sock);

}



/* Input route to socket, using a fragmented message, where sock delivery fails.

 */

static void mctp_test_route_input_sk_fail_frag(struct kunit *test)

{

	const struct mctp_hdr hdrs[2] = { RX_FRAG(FL_S, 0), RX_FRAG(FL_E, 1) };

	struct mctp_test_route *rt;

	struct mctp_test_dev *dev;

	struct sk_buff *skbs[2];

	struct socket *sock;

	unsigned int i;

	int rc;



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



	lock_sock(sock->sk);

	WRITE_ONCE(sock->sk->sk_rcvbuf, 0);

	release_sock(sock->sk);



	for (i = 0; i < ARRAY_SIZE(skbs); i++) {

		skbs[i] = mctp_test_create_skb(&hdrs[i], 10);

		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, skbs[i]);

		skb_get(skbs[i]);



		mctp_test_skb_set_dev(skbs[i], dev);

	}



	/* first route input should succeed, we're only queueing to the

	 * frag list

	 */

	rc = mctp_route_input(&rt->rt, skbs[0]);

	KUNIT_EXPECT_EQ(test, rc, 0);



	/* final route input should fail to deliver to the socket */

	rc = mctp_route_input(&rt->rt, skbs[1]);

	KUNIT_EXPECT_NE(test, rc, 0);



	/* we should hold the only reference to both skbs */

	KUNIT_EXPECT_EQ(test, refcount_read(&skbs[0]->users), 1);

	kfree_skb(skbs[0]);



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

	kfree_skb(skbs[1]);



	__mctp_route_test_fini(test, dev, rt, sock);

}



#if IS_ENABLED(CONFIG_MCTP_FLOWS)



static void mctp_test_flow_init(struct kunit *test,
@@ -1053,6 +1137,8 @@ static struct kunit_case mctp_test_cases[] = { 
			 mctp_route_input_sk_reasm_gen_params),

	KUNIT_CASE_PARAM(mctp_test_route_input_sk_keys,

			 mctp_route_input_sk_keys_gen_params),

	KUNIT_CASE(mctp_test_route_input_sk_fail_single),

	KUNIT_CASE(mctp_test_route_input_sk_fail_frag),

	KUNIT_CASE(mctp_test_route_input_multiple_nets_bind),

	KUNIT_CASE(mctp_test_route_input_multiple_nets_key),

	KUNIT_CASE(mctp_test_packet_flow),