[PATCH 0/3] net: fix three security bugs in NET/ROM and ROSE stacks

[PATCH 0/3] net: fix three security bugs in NET/ROM and ROSE stacks

[Mashiro Chen]

This series fixes three bugs in the Linux kernel AX.25 protocol stack,
submitted on top of Greg Kroah-Hartman's frame-size validation series
(https://lore.kernel.org/r/2026040730-untagged-groin-bbb7@gregkh).

Patch 1 fixes an integer overflow in nr_queue_rx_frame(): nr_sock.fraglen
is declared as unsigned short and accumulates received fragment lengths
without overflow protection.  When total received data exceeds 65535 bytes,
fraglen wraps to a small value, causing alloc_skb() to allocate a tiny
buffer followed by a full-length copy -- a heap out-of-bounds write.

Patch 2 fixes nr_find_socket() dispatching incoming NR_INFO frames by
matching only the circuit index/id bytes without validating the source
callsign against the socket's dest_addr.  Any node can inject frames into
an existing STATE_3 connection by guessing the circuit ID (the value space
is only 65025 non-zero pairs and IDs are assigned sequentially).  Combined
with the fraglen overflow in patch 1, this gives an unauthenticated
attacker a complete heap corruption primitive.

Patch 3 fixes an out-of-bounds read in rose_parse_ccitt(): the function
validates 10 <= l <= 20 but never checks that the remaining buffer is at
least l+2 bytes before calling memcpy().  rose_parse_national() already
performs the equivalent check; this patch adds the same guard.

Mashiro Chen (3):
  net: netrom: fix integer overflow in nr_queue_rx_frame()
  net: netrom: validate source address in nr_find_socket()
  net: rose: fix out-of-bounds read in rose_parse_ccitt()

 net/netrom/af_netrom.c | 11 +++++++----
 net/netrom/nr_in.c     | 10 ++++++++++
 net/rose/rose_subr.c   |  3 +++
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.53.0

[PATCH 1/3] net: netrom: fix integer overflow in nr_queue_rx_frame()

[Mashiro Chen]

nr_sock.fraglen is declared as unsigned short, so accumulating
received fragment lengths via

    nr->fraglen += skb->len;

can silently wrap around to a small value once the total exceeds
65535 bytes. When the final fragment arrives (NR_MORE_FLAG clear),
the wrapped fraglen is passed to alloc_skb(), which allocates an
undersized buffer. The subsequent skb_put() and skb_copy_from_linear_data()
loop then writes the actual full data into it, resulting in a heap
buffer overflow.

An attacker with NR_STATE_3 access (i.e. after completing a NET/ROM
connection handshake, which open BBS/node services allow to any
callsign) can trigger this by sending a stream of NR_INFO frames
with the MORE flag set until fraglen wraps, followed by a final
NR_INFO frame.

Fix by checking whether adding the incoming skb's length to the
accumulated fraglen would exceed USHRT_MAX before each accumulation.
If so, purge the fragment queue, reset fraglen, and return an error
to signal receive-busy to the caller.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 net/netrom/nr_in.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/netrom/nr_in.c b/net/netrom/nr_in.c
index 97944db6b5ac6..0b7cdb99ae501 100644
--- a/net/netrom/nr_in.c
+++ b/net/netrom/nr_in.c
@@ -36,12 +36,22 @@ static int nr_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
 	nr_start_idletimer(sk);
 
 	if (more) {
+		if ((unsigned int)nr->fraglen + skb->len > USHRT_MAX) {
+			skb_queue_purge(&nr->frag_queue);
+			nr->fraglen = 0;
+			return 1;
+		}
 		nr->fraglen += skb->len;
 		skb_queue_tail(&nr->frag_queue, skb);
 		return 0;
 	}
 
 	if (!more && nr->fraglen > 0) {	/* End of fragment */
+		if ((unsigned int)nr->fraglen + skb->len > USHRT_MAX) {
+			skb_queue_purge(&nr->frag_queue);
+			nr->fraglen = 0;
+			return 1;
+		}
 		nr->fraglen += skb->len;
 		skb_queue_tail(&nr->frag_queue, skb);
 
-- 
2.53.0

Re: [PATCH 1/3] net: netrom: fix integer overflow in nr_queue_rx_frame()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says: This review combines a review of the patch with a report
of an existing problem. And you will see a similar pattern in
the AI generated reviews I am forwarding for the other patches in this
series.

I would suggest not growing the scope of this series too much.
But rather noting (other) existing problems as areas for further work.

Frankly, I suspect fixing all such problems is a significant body of work.

---
    net: netrom: fix integer overflow in nr_queue_rx_frame()

This commit addresses an integer overflow in nr_queue_rx_frame() where
accumulating fragment lengths could wrap around a 16-bit integer. It attempts
to fix this by checking if adding the incoming skb length to the accumulated
fraglen exceeds USHRT_MAX, purging the queue and returning an error if so.

> diff --git a/net/netrom/nr_in.c b/net/netrom/nr_in.c
> index 97944db6b5ac6..0b7cdb99ae501 100644
> --- a/net/netrom/nr_in.c
> +++ b/net/netrom/nr_in.c
> @@ -36,12 +36,22 @@ static int nr_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
>  	nr_start_idletimer(sk);
>  
>  	if (more) {
> +		if ((unsigned int)nr->fraglen + skb->len > USHRT_MAX) {
> +			skb_queue_purge(&nr->frag_queue);
> +			nr->fraglen = 0;
> +			return 1;
> +		}

If we return 1 here, does the caller (nr_state3_machine) treat it as a
transient busy condition?

If the caller retries the skb later, the skb will have already been modified
by the unconditional skb_pull() at the start of nr_queue_rx_frame(). Does
this mean the next time it is processed, it will read from the wrong offset
and potentially process attacker-controlled payload?

Furthermore, since nr->fraglen is reset to 0, wouldn't the oversized fragment
now pass the USHRT_MAX check on retry, bypassing the overflow mitigation
entirely?

When we purge the fragment queue here, are we also silently dropping
fragments that the protocol stack has already acknowledged to the sender?

Since nr_queue_rx_frame() previously returned 0 for those earlier fragments,
the expected sequence number has already advanced. If the sender never
retransmits them and fraglen is reset, will the receiver accept the next
fragment as the start of a new sequence, leading to a truncated message being
delivered to the application?

[ ... ]

While reviewing this patch, I also noticed a few pre-existing issues in
nr_queue_rx_frame() that might be worth addressing.

Looking at the start of the function:

static int nr_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
{
	struct sk_buff *skbo, *skbn = skb;
	struct nr_sock *nr = nr_sk(sk);

	skb_pull(skb, NR_NETWORK_LEN + NR_TRANSPORT_LEN);

Is it possible for skb_pull() to be called on a malformed packet where the
total length is sufficient but the linear region is too small? If so, would
this trigger a BUG_ON() panic inside __skb_pull()? Should there be a length
validation check using pskb_may_pull() beforehand?

Further down, during fragment reassembly:

	while ((skbo = skb_dequeue(&nr->frag_queue)) != NULL) {
		skb_copy_from_linear_data(skbo,
					  skb_put(skbn, skbo->len),
					  skbo->len);
		kfree_skb(skbo);
	}

If skbo is a non-linear (paged) skb, skbo->len includes the paged data length.
Since skb_copy_from_linear_data() acts as a direct memory copy, does this
result in reading past the end of the linear allocation (skb_headlen()) into
adjacent kernel memory?

Finally, looking at the error paths:

	if ((skbn = alloc_skb(nr->fraglen, GFP_ATOMIC)) == NULL)
		return 1;
	...
	return sock_queue_rcv_skb(sk, skbn);

If sock_queue_rcv_skb() fails, it returns a non-zero error which propagates
to the caller. Does the caller then try to re-enqueue the original fragment
pointer into a retry queue?

Since the original fragments were already dequeued from frag_queue and freed
via kfree_skb() in the loop above, could enqueuing the freed skb lead to a
use-after-free?

Additionally, if alloc_skb() fails and returns 1, does the caller enqueue the
original skb while it is still linked in frag_queue, potentially corrupting
the doubly-linked list?

[PATCH 2/3] net: netrom: validate source address in nr_find_socket()

[Mashiro Chen]

nr_find_socket() dispatches incoming NR_INFO frames into a connected
socket by matching the frame's circuit index/id pair (bytes[15-16])
against the socket's my_index/my_id.  It performs no validation of
the frame's source callsign against the socket's dest_addr.

This means any node on the network can craft an NR_INFO frame with
a guessed or brute-forced circuit index/id pair and have it accepted
into an arbitrary STATE_3 connection as if it came from the legitimate
peer.  Circuit IDs are assigned sequentially starting at (1,1), making
them predictable in practice.

This is exploited in concert with CVE-XXXX-XXXXX (nr_queue_rx_frame
fraglen overflow): an attacker can inject NR_INFO | NR_MORE_FLAG frames
into an existing connection without owning a connection themselves,
driving the victim socket's fraglen to wrap and triggering the heap
overflow entirely unauthenticated (CVSS PR:N).

Fix by adding a source address parameter to nr_find_socket() and
requiring it to match the socket's recorded dest_addr for all
frame-dispatch lookups.  The internal nr_find_next_circuit() caller,
which only checks for circuit ID availability, passes NULL to skip
the source check.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 net/netrom/af_netrom.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index b605891bf86e4..73742cc9e9e42 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -162,7 +162,8 @@ static struct sock *nr_find_listener(ax25_address *addr)
 /*
  *	Find a connected NET/ROM socket given my circuit IDs.
  */
-static struct sock *nr_find_socket(unsigned char index, unsigned char id)
+static struct sock *nr_find_socket(unsigned char index, unsigned char id,
+				   const ax25_address *src)
 {
 	struct sock *s;
 
@@ -170,7 +171,8 @@ static struct sock *nr_find_socket(unsigned char index, unsigned char id)
 	sk_for_each(s, &nr_list) {
 		struct nr_sock *nr = nr_sk(s);
 
-		if (nr->my_index == index && nr->my_id == id) {
+		if (nr->my_index == index && nr->my_id == id &&
+		    (!src || !ax25cmp(&nr->dest_addr, src))) {
 			sock_hold(s);
 			goto found;
 		}
@@ -219,7 +221,8 @@ static unsigned short nr_find_next_circuit(void)
 		j = id % 256;
 
 		if (i != 0 && j != 0) {
-			if ((sk=nr_find_socket(i, j)) == NULL)
+			sk = nr_find_socket(i, j, NULL);
+			if (!sk)
 				break;
 			sock_put(sk);
 		}
@@ -926,7 +929,7 @@ int nr_rx_frame(struct sk_buff *skb, struct net_device *dev)
 		if (frametype == NR_CONNREQ)
 			sk = nr_find_peer(circuit_index, circuit_id, src);
 		else
-			sk = nr_find_socket(circuit_index, circuit_id);
+			sk = nr_find_socket(circuit_index, circuit_id, src);
 	}
 
 	if (sk != NULL) {
-- 
2.53.0

Re: [PATCH 2/3] net: netrom: validate source address in nr_find_socket()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: netrom: validate source address in nr_find_socket()

This commit adds a source address parameter to nr_find_socket() to validate
incoming frames against the socket's dest_addr. This prevents unauthenticated
injection of crafted NR_INFO frames into existing connections.

> diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
> index b816c56124ab8..1da32c52a1931 100644
> --- a/net/netrom/af_netrom.c
> +++ b/net/netrom/af_netrom.c

[ ... ]

> @@ -219,7 +221,8 @@ static unsigned short nr_find_next_circuit(void)
>  		j = id % 256;
>  
>  		if (i != 0 && j != 0) {
> -			if ((sk=nr_find_socket(i, j)) == NULL)
> +			sk = nr_find_socket(i, j, NULL);
> +			if (!sk)
>  				break;
>  			sock_put(sk);
>  		}

This isn't a bug introduced by this patch, but does the loop in
nr_find_next_circuit() lack a termination condition?

If all 65,025 valid (i, j) circuit IDs are in use, nr_find_socket() will
always return a socket. The loop will increment id indefinitely, wrap around
65535, and never break, potentially triggering a CPU lockup in softirq
context.

Additionally, since the circuit ID is managed globally, is there a race
condition during its allocation? Multiple threads could concurrently execute
nr_find_next_circuit() and circuit++, leading to identical circuit IDs being
assigned to different connections.

[ ... ]

> @@ -923,7 +926,7 @@ int nr_rx_frame(struct sk_buff *skb, struct net_device *dev)
>  		if (frametype == NR_CONNREQ)
>  			sk = nr_find_peer(circuit_index, circuit_id, src);
>  		else
> -			sk = nr_find_socket(circuit_index, circuit_id);
> +			sk = nr_find_socket(circuit_index, circuit_id, src);
>  	}
>  
>  	if (sk != NULL) {

This isn't a bug introduced by this patch, but does nr_rx_frame() safely
access the socket buffer data?

It unconditionally accesses data up to skb->data[19], and for CONNREQ frames,
it copies 7 bytes from skb->data + 21. If the packet is shorter than 28 bytes,
could this cause an out-of-bounds read and leak adjacent kernel memory?

Furthermore, pskb_may_pull() is not called before these accesses.

Additionally, if an IP-over-NET/ROM packet is smaller than 20 bytes,
skb_pull() fails and is ignored, erroneously passing the unmodified packet
to the IP stack.

[PATCH 3/3] net: rose: fix out-of-bounds read in rose_parse_ccitt()

[Mashiro Chen]

rose_parse_ccitt() handles 0xC0-class facilities by reading l = p[1]
and validating 10 <= l <= 20, but never checks whether the remaining
buffer actually contains l + 2 bytes before accessing p + 7 and
p + 12 via memcpy().

An attacker can send a ROSE_CALL_REQUEST frame with a crafted CCITT
facility whose declared length fits the 10-20 range but whose actual
data is truncated. This causes the kernel to read up to l + 2 bytes
beyond the end of the facilities field, leaking adjacent skb data.

By contrast, rose_parse_national() already performs the equivalent
check (if (len < 2 + l) return -1) for all its 0xC0-class cases.

Add the same check to rose_parse_ccitt() before any data access.

Fixes: e0bccd315db0 ("rose: Add length checks to CALL_REQUEST parsing")
Cc: stable@vger.kernel.org
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 net/rose/rose_subr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
index 4dbc437a9e229..a902ddeddc5bd 100644
--- a/net/rose/rose_subr.c
+++ b/net/rose/rose_subr.c
@@ -370,6 +370,9 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
 			if (l < 10 || l > 20)
 				return -1;
 
+			if (len < 2 + l)
+				return -1;
+
 			if (*p == FAC_CCITT_DEST_NSAP) {
 				memcpy(&facilities->source_addr, p + 7, ROSE_ADDR_LEN);
 				memcpy(callsign, p + 12,   l - 10);
-- 
2.53.0

Re: [PATCH 3/3] net: rose: fix out-of-bounds read in rose_parse_ccitt()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: rose: fix out-of-bounds read in rose_parse_ccitt()

This commit adds bounds checking to rose_parse_ccitt() for 0xC0-class
facilities to prevent an out-of-bounds read when parsing call request
frames. It ensures the remaining buffer contains enough bytes before
accessing the facility data.

Cc: stable@vger.kernel.org

> diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
> index 4dbc437a9e229..a902ddeddc5bd 100644
> --- a/net/rose/rose_subr.c
> +++ b/net/rose/rose_subr.c
> @@ -370,6 +370,9 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
>  			if (l < 10 || l > 20)
>  				return -1;
>  
> +			if (len < 2 + l)
> +				return -1;
> +
>  			if (*p == FAC_CCITT_DEST_NSAP) {
>  				memcpy(&facilities->source_addr, p + 7, ROSE_ADDR_LEN);
>  				memcpy(callsign, p + 12,   l - 10);

Can asc2ax() read uninitialized stack memory here?

If a facility length of l = 17 is provided, memcpy() copies 7 bytes to the
callsign array, and the 8th byte is set to null, leaving the remaining
bytes uninitialized.

When asc2ax() processes this, it consumes the first 6 bytes in its loop.
The pointer then advances past the null byte, and the check if (*s != '\0')
will read the 9th byte, which contains uninitialized stack garbage.

Would zero-initializing the callsign array prevent this?

This regression wasn't introduced by this patch, but exists in the modified
function's loop termination logic:

net/rose/rose_subr.c:rose_parse_ccitt() {
    ...
    } while (*p != 0x00 && len > 0);
}

Due to short-circuit evaluation, if a facility consumes the exact remaining
bytes so that len becomes 0, will the loop dereference the pointer p before
checking if len > 0?

This would cause an out-of-bounds read past the valid facility data.

Should the condition be swapped to while (len > 0 && *p != 0x00)?

This regression wasn't introduced by this patch, but resides in the same
parsing sequence:

net/rose/rose_subr.c:rose_parse_facilities() {
    ...
    facilities_len = *p++;
    if (facilities_len == 0 || (unsigned int)facilities_len > packet_len)
    ...
}

Since packet_len represents the total remaining bytes including the 1-byte
length prefix itself, and the length byte has already been consumed, only
packet_len - 1 bytes actually remain for the facility data.

If an attacker crafts a packet where facilities_len == packet_len, will
this validation incorrectly pass?

The parsing loops will then expect packet_len bytes but only have
packet_len - 1 available, leading to an out-of-bounds read.

Could this check be updated to use facilities_len >= packet_len?

This regression wasn't introduced by this patch, but violates networking
subsystem safety rules regarding socket buffers.

Does the rose subsystem safely linearize socket buffers before dereferencing
packet headers?

Throughout the subsystem, including the parsing functions and routing logic,
packet offsets are accessed directly from skb->data based only on skb->len.

Since skb->len includes paged fragments, if a packet is fragmented such
that skb_headlen(skb) is smaller than the accessed offset, could
dereferencing skb->data trigger a page fault or read garbage memory?

Would adding pskb_may_pull() checks before accessing headers resolve this?