[PATCH net v2] net/x25: Fix overflow when accumulating packets

[PATCH net v2] net/x25: Fix overflow when accumulating packets

[Martin Schiller]

Add a check to ensure that `x25_sock.fraglen` does not overflow.

The `fraglen` also needs to be resetted when purging `fragment_queue` in
`x25_clear_queues()`.

Reported-by: Yiming Qian <yimingqian591@gmail.com>
Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
Changes in v2:
- Use USHRT_MAX instead of sizeof(fraglen) nonsense
- Link to v1: https://lore.kernel.org/r/20260327-x25_fraglen-v1-1-9fc751d4f754@dev.tdt.de
---
 net/x25/x25_in.c   | 6 ++++++
 net/x25/x25_subr.c | 1 +
 2 files changed, 7 insertions(+)

diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
index b981a4828d08c2e6676749a06035910eab01e6cd..cb84c683d249d6078f3673835bb2f80eb487f253 100644
--- a/net/x25/x25_in.c
+++ b/net/x25/x25_in.c
@@ -34,6 +34,12 @@ static int x25_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
 	struct sk_buff *skbo, *skbn = skb;
 	struct x25_sock *x25 = x25_sk(sk);
 
+	/* make sure we don't overflow */
+	if (x25->fraglen + skb->len > USHRT_MAX) {
+		kfree_skb(skb);
+		return 1;
+	}
+
 	if (more) {
 		x25->fraglen += skb->len;
 		skb_queue_tail(&x25->fragment_queue, skb);
diff --git a/net/x25/x25_subr.c b/net/x25/x25_subr.c
index 0285aaa1e93c17233748d38eef6d8b5c6059b67a..159708d9ad20cb2e6db24ead67daf1e9d6258f64 100644
--- a/net/x25/x25_subr.c
+++ b/net/x25/x25_subr.c
@@ -40,6 +40,7 @@ void x25_clear_queues(struct sock *sk)
 	skb_queue_purge(&x25->interrupt_in_queue);
 	skb_queue_purge(&x25->interrupt_out_queue);
 	skb_queue_purge(&x25->fragment_queue);
+	x25->fraglen = 0;
 }
 
 

---
base-commit: 4ae97cae07e15d41e5c0ebabba64c6eefdeb0bbe
change-id: 20260325-x25_fraglen-8fc240d1edd3

Best regards,
-- 
Martin Schiller <ms@dev.tdt.de>

Re: [PATCH net v2] net/x25: Fix overflow when accumulating packets

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/x25: Fix overflow when accumulating packets

This commit adds a check to prevent x25_sock.fraglen from overflowing
during packet accumulation. It also resets the fragment length when
purging the fragment queue.

> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
> index b981a4828d08..cb84c683d249 100644
> --- a/net/x25/x25_in.c
> +++ b/net/x25/x25_in.c
> @@ -34,6 +34,12 @@ static int x25_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
>  	struct sk_buff *skbo, *skbn = skb;
>  	struct x25_sock *x25 = x25_sk(sk);
>  
> +	/* make sure we don't overflow */
> +	if (x25->fraglen + skb->len > USHRT_MAX) {
> +		kfree_skb(skb);
> +		return 1;
> +	}

Does this cause the skb to be freed twice?

When x25_queue_rx_frame() frees the skb and returns 1, the caller
x25_state3_machine() leaves its local queued variable as 0 and returns 0.

This 0 value propagates up through x25_process_rx_frame() to callers
like x25_backlog_rcv() or x25_lapb_receive_frame(). Since the return
value indicates the packet was not consumed by the protocol stack, these
upper-level functions will call kfree_skb(skb) again on the already
freed skb.

Would it be safer to return 1 without calling kfree_skb(skb) here so
the callers can handle freeing the unconsumed packet?
-- 
pw-bot: cr

Re: [PATCH net v2] net/x25: Fix overflow when accumulating packets

[Martin Schiller]

On 2026-04-02 04:48, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/x25: Fix overflow when accumulating packets
> 
> This commit adds a check to prevent x25_sock.fraglen from overflowing
> during packet accumulation. It also resets the fragment length when
> purging the fragment queue.
> 
>> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
>> index b981a4828d08..cb84c683d249 100644
>> --- a/net/x25/x25_in.c
>> +++ b/net/x25/x25_in.c
>> @@ -34,6 +34,12 @@ static int x25_queue_rx_frame(struct sock *sk, 
>> struct sk_buff *skb, int more)
>>  	struct sk_buff *skbo, *skbn = skb;
>>  	struct x25_sock *x25 = x25_sk(sk);
>> 
>> +	/* make sure we don't overflow */
>> +	if (x25->fraglen + skb->len > USHRT_MAX) {
>> +		kfree_skb(skb);
>> +		return 1;
>> +	}
> 
> Does this cause the skb to be freed twice?
> 
> When x25_queue_rx_frame() frees the skb and returns 1, the caller
> x25_state3_machine() leaves its local queued variable as 0 and returns 
> 0.
> 
> This 0 value propagates up through x25_process_rx_frame() to callers
> like x25_backlog_rcv() or x25_lapb_receive_frame(). Since the return
> value indicates the packet was not consumed by the protocol stack, 
> these
> upper-level functions will call kfree_skb(skb) again on the already
> freed skb.
> 
> Would it be safer to return 1 without calling kfree_skb(skb) here so
> the callers can handle freeing the unconsumed packet?

This is already fixed / changed in the v4 patch set.