[PATCH net 1/1] llc: conn: drop out-of-service state in llc_conn_service

[PATCH net 1/1] llc: conn: drop out-of-service state in llc_conn_service

[Ren Wei]

From: Zhengchuan Liang <zcliangcn@gmail.com>

llc_conn_service() only rejects states above NBR_CONN_STATES, but
LLC_CONN_OUT_OF_SVC is also invalid for connection state processing.

The connection state tables are indexed by llc->state - 1, so only
states in the range 1..NBR_CONN_STATES are valid here. If a socket in
LLC_CONN_OUT_OF_SVC is still reachable and another frame is delivered
to it, llc_qualify_conn_ev() will walk the state tables with an
invalid index.

Drop LLC_CONN_OUT_OF_SVC before looking up the current state
transition. Return success for that state so repeated packets to a
stale reachable socket are ignored quietly instead of being turned
into llc_conn_service() failures.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Zhengchuan Liang <zcliangcn@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/llc/llc_conn.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 5c0ac243b248..715d5a314071 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -360,6 +360,10 @@ static int llc_conn_service(struct sock *sk, struct sk_buff *skb)
 	struct llc_sock *llc = llc_sk(sk);
 	int rc = 1;
 
+	if (llc->state == LLC_CONN_OUT_OF_SVC) {
+		rc = 0;
+		goto out;
+	}
 	if (llc->state > NBR_CONN_STATES)
 		goto out;
 	rc = 0;
-- 
2.34.1

Re: [PATCH net 1/1] llc: conn: drop out-of-service state in llc_conn_service

[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
---
llc: conn: drop out-of-service state in llc_conn_service

The commit adds a check for LLC_CONN_OUT_OF_SVC in llc_conn_service() to
avoid indexing state tables with an invalid state. It returns success for
this state to ignore repeated packets to a stale reachable socket.

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 5c0ac243b248f..715d5a3140716 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -360,6 +360,10 @@ static int llc_conn_service(struct sock *sk, struct sk_buff *skb)
>  	struct llc_sock *llc = llc_sk(sk);
>  	int rc = 1;
>  
> +	if (llc->state == LLC_CONN_OUT_OF_SVC) {
> +		rc = 0;
> +		goto out;
> +	}

Can this check still race with llc_sk_free() setting llc->state locklessly?

llc_ui_release() drops the socket lock via release_sock(sk) and calls
llc_sk_free(sk), which sets llc->state = LLC_CONN_OUT_OF_SVC (0) without
holding a lock.

Concurrently, a packet processed by llc_conn_handler() can acquire
bh_lock_sock(sk) and call llc_conn_service(). Since llc->state is checked
without READ_ONCE(), could the compiler emit multiple loads?

If llc->state is > 0 during this check, it passes. If CPU 1 then locklessly
changes llc->state to 0, the subsequent call to llc_qualify_conn_ev() could
read llc->state again from memory as 0.

This would evaluate llc->state - 1 as 255, causing an out-of-bounds array
access on llc_conn_state_table[255]. The garbage data could be interpreted
as curr_state->transitions, leading to unconditional dereferences and a
kernel panic. Does this lockless concurrency need to be addressed to
properly fix the issue?

Additionally, this highlights a potential timer race during socket teardown.
llc_sk_free() stops timers via llc_sk_stop_all_timers(sk, true).

CPU 1 starts socket destruction in llc_ui_release(), calling release_sock(sk)
followed by llc_sk_free(sk).

Concurrently, CPU 2 (finding the socket under RCU via __llc_lookup) receives
a packet in llc_conn_handler(), acquires bh_lock_sock(sk), and since
!sock_owned_by_user(sk) is now true, directly processes the packet.

If CPU 2 processes state transitions and triggers an action (like
llc_conn_ac_start_ack_timer) that calls mod_timer(), it could re-arm the
timer AFTER CPU 1 has stopped it, because CPU 1's timer_delete_sync() does
not wait for the packet handler.

When CPU 1 drops its reference, and CPU 2 finishes processing and calls
sock_put(sk), the socket memory is scheduled to be freed. Will the timer
re-armed by CPU 2 fire later on the freed socket memory, causing a
use-after-free regression?

>  	if (llc->state > NBR_CONN_STATES)
>  		goto out;
>  	rc = 0;