[PATCH net] net: mctp: free reassembly head skb when key_add fails on SOM

[PATCH net] net: mctp: free reassembly head skb when key_add fails on SOM

[Joas Antonio dos Santos]

When a start-of-message fragment creates a new reassembly key on the
broadcast/bind path, mctp_frag_queue() stores the skb as
key->reasm_head before the key is made observable with mctp_key_add().

If mctp_key_add() fails (a race with another SOM packet for the same
source/dest/tag, or the receiving socket going away), the error path
only drops the key reference via mctp_key_unref().  The final key
destructor frees the key itself but not key->reasm_head, and since the
key was never added to any list, neither the expiry worker nor socket
teardown can reach it to release the queued fragment.  The skb is
leaked.

Free key->reasm_head on the key_add failure path before releasing the
key.  Other teardown paths (__mctp_key_done_in(), EOM delivery) already
clear reasm_head before the final unref, so this only affects the
failure path.

Fixes: 4a992bbd3650 ("mctp: Implement message fragmentation & reassembly")
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
Found by code review.  The reassembly/key lifetime was traced in
net/mctp/route.c: mctp_key_unref()'s destructor frees the key but not
key->reasm_head, and only __mctp_key_done_in() and the EOM delivery
path clear reasm_head, neither of which runs on the mctp_key_add()
failure path.  Not yet build- or runtime-tested (build environment
temporarily unavailable); the change is a one-line kfree_skb() on an
error path.

 net/mctp/route.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/mctp/route.c b/net/mctp/route.c
index e7c95eeac..3d79c2031 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -552,8 +552,16 @@ static int mctp_dst_input(struct mctp_dst *dst, struct sk_buff *skb)
 			 * can do is drop.
 			 */
 			rc = mctp_key_add(key, msk);
-			if (!rc)
+			if (!rc) {
 				trace_mctp_key_acquire(key);
+			} else {
+				/* key_add failed: the key is not observable by
+				 * any cleanup path, so free the fragment we
+				 * just queued on it to avoid leaking the skb.
+				 */
+				kfree_skb(key->reasm_head);
+				key->reasm_head = NULL;
+			}
 
 			/* we don't need to release key->lock on exit, so
 			 * clean up here and suppress the unlock via

Re: [PATCH net] net: mctp: free reassembly head skb when key_add fails on SOM

[Jeremy Kerr]

Hi Joas & Guikin,

[merging two threads here; Guikin reported the same issue, around 12
hours earlier]

> When a start-of-message fragment creates a new reassembly key on the
> broadcast/bind path, mctp_frag_queue() stores the skb as
> key->reasm_head before the key is made observable with mctp_key_add().
> 
> If mctp_key_add() fails (a race with another SOM packet for the same
> source/dest/tag, or the receiving socket going away), the error path
> only drops the key reference via mctp_key_unref().  The final key
> destructor frees the key itself but not key->reasm_head, and since the
> key was never added to any list, neither the expiry worker nor socket
> teardown can reach it to release the queued fragment.  The skb is
> leaked.

OK, good catch.

> diff --git a/net/mctp/route.c b/net/mctp/route.c
> index e7c95eeac..3d79c2031 100644
> --- a/net/mctp/route.c
> +++ b/net/mctp/route.c
> @@ -552,8 +552,16 @@ static int mctp_dst_input(struct mctp_dst *dst, struct sk_buff *skb)
>  			 * can do is drop.
>  			 */
>  			rc = mctp_key_add(key, msk);
> -			if (!rc)
> +			if (!rc) {
>  				trace_mctp_key_acquire(key);
> +			} else {
> +				/* key_add failed: the key is not observable by
> +				 * any cleanup path, so free the fragment we
> +				 * just queued on it to avoid leaking the skb.
> +				 */
> +				kfree_skb(key->reasm_head);
> +				key->reasm_head = NULL;
> +			}

We could just defer setting skb to NULL until the mctp_key_add() has
succeeded; there's already the common error path that cleans up any
remaining skb.

Alternatively, we could change the semantics mctp_key_unref() to also
free any pending reassembly, but I'm okay with the current approach of
being explicit about the state handling.

Cheers,


Jeremy

Re: [PATCH net] net: mctp: free reassembly head skb when key_add fails on SOM

[Joas Antonio dos Santos]

Hi Jeremy,

> [merging two threads here; Guikin reported the same issue, around 12
> hours earlier]

Thanks, I hadn't seen Guikun's report. If a v2 is wanted I'll add:

Reported-by: Guikun Yang <yangguikun19@qq.com>
Closes: https://lore.kernel.org/all/tencent_43A65D51143E18518EA968DF9323694EFD08@qq.com/

> We could just defer setting skb to NULL until the mctp_key_add() has
> succeeded; there's already the common error path that cleans up any
> remaining skb.

I think that one needs care. For the first fragment, mctp_frag_queue()
stores skb_unclone_full(skb) in key->reasm_head, and if skb is a clone,
skb_unclone_full() frees it and returns a copy. The kfree_skb(skb) at
out: would then free the original a second time, and the copy in
reasm_head would still leak. Taking skb back from key->reasm_head on
failure would work, but the explicit free looked simpler to me.

> Alternatively, we could change the semantics mctp_key_unref() to also
> free any pending reassembly, but I'm okay with the current approach of
> being explicit about the state handling.

Then I'll keep it as is. Let me know if you want a v2 with the tags
above.

Thanks,
Joas