Re: [PATCH net] tipc: fix memory leaks in bundle and fragment paths

[xiaoshoukui@gmail.com]

Hi Tung,

Thanks for the review and feedback.

On Thu, Sep 17, 2026 at 03:51:16AM +0000, Tung Quang Nguyen wrote:
> This cannot occur because the sending side already does sanity check to
> make sure that no invalid protocol type exists in bundled and fragmented
> messages.
> This only occurs when you create fake TIPC messages or tampering with
> existing messages. This is an invalid use case because TIPC is being used
> in an insecure environment. In such environment, IPSec or TIPC encryption
> must be used as mentioned here
> https://datatracker.ietf.org/doc/html/draft-maloy-tipc-01.txt#section-6

I agree that the normal transmit path performs checks that prevent the
relevant internal protocol users from being bundled. In particular,
tipc_msg_try_bundle() rejects MSG_FRAGMENTER, TUNNEL_PROTOCOL, and
BCAST_PROTOCOL.

However, I think this is separate from the receive-side skb ownership
issue addressed by this patch. The patch does not assume that these
message types are normally generated by the transmit path. It handles
the case where an skb has reached tipc_data_input() and that function
explicitly reports that it did not consume the skb.

The relevant point here is that an skb that is not consumed by
tipc_data_input() must still have a defined owner and cleanup path.

> This check is redundant because bundled messages do not contain message
> types that can make tipc_data_input() return false. The only exception
> is tampering with TIPC messages.
> If that is the case, a fake valid message type can be inserted into the
> bundle message and tipc_data_input() returns true but user applications
> are broken by corrupt messages.

I agree that a tampered message can be changed to a valid user type.
In that case, tipc_data_input() returns true and the message is queued
for further receive processing.

However, that case does not exercise the check added by this patch.
The added kfree_skb_reason() is only executed when tipc_data_input()
returns false.

The two cases therefore have different ownership behavior:

valid user type
    -> tipc_data_input() returns true
    -> skb is queued to inputq
    -> receive processing continues

unhandled protocol user
    -> tipc_data_input() returns false
    -> skb is not queued or consumed by tipc_data_input()

For the latter case, the bundle path currently ignores the return value:

while (tipc_msg_extract(skb, &iskb, &pos))
    tipc_data_input(l, iskb, &tmpq);

tipc_msg_extract() has already allocated and validated iskb before
returning it. For the protocol users listed above, tipc_data_input()
then returns false without consuming the skb. Since the caller ignores
that return value, the extracted skb has no subsequent cleanup path.

The proposed check only closes this ownership gap; it does not attempt
to solve message-integrity or anti-tampering problems.

> >+				kfree_skb_reason(iskb,
> >SKB_DROP_REASON_UNHANDLED_PROTO);
> 
> Compiling warnings:
> https://netdev-ctrl.bots.linux.dev/logview.html?f=/logs/build/1166117/14820384/checkpatch/stdout

Thanks for pointing this out. I will fix the checkpatch formatting issue
in v2.

> This check is redundant because fragmented messages do not contain message
> types that can make tipc_data_input() return false. The only exception
> is tampering with TIPC messages.
> If that is the case:
> - dropping the invalid fragmented message will cause the reassembled message
>   corrupt (For example: sending a 65KB message but dropping/truncating 1500
>   bytes). As a result, user applications receive corrupt messages.
> - a fake fragmented message can ben sent and tipc_data_input() returns true
>   but user applications are broken by corrupt messages.

Regarding the fragment path, I believe the proposed change does not drop
an individual fragment during reassembly.

tipc_buf_append() is documented to return 1 only when reassembly is
complete. For the first and intermediate fragments it returns 0. When
the last fragment is received, it validates the reassembled message,
assigns the complete reassembled skb to *buf, clears the reassembly
state, and returns 1.

Therefore, the relevant sequence is:

  first fragment
      |
  intermediate fragments
      |
  last fragment
      |
  complete reassembly
      |
  tipc_msg_validate()
      |
  tipc_data_input()

The added kfree_skb_reason() is reached only after this complete
reassembly step:

  if (tipc_buf_append(reasm_skb, &skb)) {
      l->stats.recv_fragmented++;
      if (!tipc_data_input(l, skb, inputq))
          kfree_skb_reason(skb,
                           SKB_DROP_REASON_UNHANDLED_PROTO);
  }

At that point, skb refers to the complete reassembled message, not to an
individual 1500-byte fragment.

If tipc_data_input() returns true, the reassembled skb is queued to
inputq as before. If it returns false, the complete reassembled skb is
not consumed by tipc_data_input(). Without an explicit cleanup at this
point, the reassembled skb and its associated fragment data become
unreachable.

Thus, the proposed change does not truncate a 65KB message by dropping
one 1500-byte fragment. It only releases the complete reassembled skb
when the receive-side dispatcher reports that it did not consume it.

I agree that a forged valid user type is a separate message-integrity
issue. This patch is not intended to provide authentication or
protection against message tampering; its purpose is limited to ensuring
that skbs rejected by tipc_data_input() are properly released.

Thanks again for the review.

Best regards,
xiaoshoukui