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

From: 'Simon Horman' <[email protected]>

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.