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

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

Hi Tung,

Thanks for the review and feedback.

On Thu, Sep 17, 2026 at 03:51:16AM +0000, Tung Quang Nguyen wrote:

quoted

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.

This patch tries to address an unreal use case. I am waiting for your real reproducer and explanation on how data part of bundling and fragmented messages can contain invalid types/users.
Of course, tipc_data_input() drops invalid types/users as designed.

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.

quoted

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 check is redundant because tipc_data_input() never returns false after calling tipc_msg_extract() or tipc_buf_append().

The two cases therefore have different ownership behavior:

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

This is correct but there is more valid use case:
-> tipc_data_input() returns false
-> skb is passed to tipc_link_input()
->  tipc_data_input() returns true
-> skb is queued to inputq

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

This is not correct. If tipc_data_input() returns false, the skb will be dropped.

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.

This is not correct as explained above.

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

quoted

quoted

+				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.

quoted

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

quoted

  corrupt (For example: sending a 65KB message but dropping/truncating

1500

quoted

  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.

You are right. I misread tipc_buf_append().

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.

tipc_data_input() releases invalid skbs as expected.
Note that we do not want to add check for unreal cases because it causes performance regression.
I tested your patch and it showed regression as below:

Setup: disable NAGLE to generate bundling messages. Two netperf threads with different message sizes are executed at the same time.

[BEFORE PATCH]:
node1 ~ # taskset -c 0 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 64
TIPC STREAM TEST to <1.0.561:547203127>
Throughput 
521.49

node1 ~ # taskset -c 1 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 65536
TIPC STREAM TEST to <1.0.561:309928041>
Throughput 
9342.63    

[AFTER PATCH]:
node1 ~ # taskset -c 0 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 64
TIPC STREAM TEST to <1.0.561:1589461199>
Throughput 
440.38      

node1 ~ # taskset -c 1 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 65536
TIPC STREAM TEST to <1.0.561:2903145288>
Throughput 
8417.67