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: 4a992bbd3650 ("mctp: Implement message fragmentation & reassembly")
Cc: stable@vger.kernel.org
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Link: https://patch.msgid.link/20241218-mctp-next-v2-1-1c1729645eaa@codeconstruct.com.au
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/net/mctp/route.c b/net/mctp/route.c
index 597e9cf5aa64445474287a3fee02ba760db15796..3f2bd65ff5e3c940c8204dca14327a15a15ba26d 100644 (file)
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -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,12 +513,19 @@ 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);
-                       key->reasm_head = NULL;
+               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,8 +544,7 @@ out_unlock:
        if (any_key)
                mctp_key_unref(any_key);
 out:
-       if (rc)
-               kfree_skb(skb);
+       kfree_skb(skb);
        return rc;
 }
 

diff --git a/net/mctp/test/route-test.c b/net/mctp/test/route-test.c
index 8551dab1d1e69836c84f68509bc9dab43a96cc67..17165b86ce22d48b10793a82cc10192b8749e7e6 100644 (file)
--- a/net/mctp/test/route-test.c
+++ b/net/mctp/test/route-test.c
@@ -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),