syzbot reported an out of bounds access of llc_conn_state_table[] and
llc_offset_table[] reached from the LLC receive path:
BUG: KASAN: global-out-of-bounds in llc_qualify_conn_ev net/llc/llc_conn.c:394
Read of size 8 at addr ffff800089e50078 by task syz.0.17/4975
The buggy address belongs to the variable:
llc_temp_state_transitions+0x58/0x60
UBSAN: array-index-out-of-bounds in net/llc/llc_conn.c:681:8
index -1 is out of range for type 'int[12][5]'
Both tables are indexed with "llc->state - 1" because connection states
are 1-based, and LLC_CONN_OUT_OF_SVC is 0, so a connection in that
pseudo state indexes them with -1.
LLC_CONN_OUT_OF_SVC is not a state of the state machine: it means the
connection component does not exist, either because it has not been
brought up or because it has been torn down, and it has no row in
llc_conn_state_table[]. A socket can nevertheless be left in it while
remaining reachable:
llc_adm_state_trans_5 (LLC_CONN_STATE_ADM, "receive any frame")
.next_state = LLC_CONN_OUT_OF_SVC
.ev_actions = { llc_conn_disc }
llc_conn_disc() is documented as "removes connection from SAP list and
frees it", but it has been a stub returning 0 since 1da177e4c3f4
("Linux-2.6.12-rc2"). So the first unexpected frame for a socket sitting
in LLC_CONN_STATE_ADM - a bound socket, or a passive open child created
by llc_create_incoming_sock() - moves it to LLC_CONN_OUT_OF_SVC and
leaves it hashed in its SAP. Every following frame is found again by
__llc_lookup_established() and dispatched into the state machine with
state 0. That is what the reproducer does: bind a PF_LLC socket and
inject the same 802.2 frame twice.
"An out of service connection must not be given events" is already the
rule, but it is enforced at only some of the entry points:
llc_backlog_rcv() tests "llc->state > 1", and llc_process_tmr_ev() and
llc_send_disc() test LLC_CONN_OUT_OF_SVC. The direct receive path
(llc_conn_handler() -> llc_conn_rcv()) and the upper layer primitive
path (llc_establish_connection()) do not, and llc_conn_service()'s own
sanity check covers the upper bound only.
Enforce the rule once in llc_conn_state_process(), which every event
source funnels through, and drop the event there rather than let it
reach the tables. Complete llc_conn_service()'s range check as well so
that the code doing the indexing cannot underflow either.
The socket is still left bricked in LLC_CONN_OUT_OF_SVC, which is the
pre-existing behaviour of the state table, but it is no longer a memory
safety problem.
Build tested ARCH=x86_64 net/llc/ with GCC 14.2.0, CONFIG_LLC2=y and =m.
Reported-by: [email protected]
Closes: https://lore.kernel.org/all/[email protected]
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Assisted-by: Claude:claude-opus-5[1m]
Signed-off-by: Kees Cook <[email protected]>
---
include/net/llc_c_st.h | 2 +-
net/llc/llc_conn.c | 38 +++++++++++++++++++++++++++++++++++++-
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/include/net/llc_c_st.h b/include/net/llc_c_st.h
index f52a4cc4880d..24577e0b17df 100644
--- a/include/net/llc_c_st.h
+++ b/include/net/llc_c_st.h
@@ -42,5 +42,5 @@ struct llc_conn_state {
const struct llc_conn_state_trans **transitions;
};
-extern struct llc_conn_state llc_conn_state_table[];
+extern struct llc_conn_state llc_conn_state_table[NBR_CONN_STATES];
#endif /* LLC_C_ST_H */
diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 260460d50f54..a4ae29b42300 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -36,11 +36,38 @@ static const struct llc_conn_state_trans
*llc_qualify_conn_ev(struct sock *sk,
/* Offset table on connection states transition diagram */
static int llc_offset_table[NBR_CONN_STATES][NBR_CONN_EV];
+/* Both tables are walked together with the same "state - 1" index. */
+static_assert(ARRAY_SIZE(llc_offset_table) ==
ARRAY_SIZE(llc_conn_state_table));
+
int sysctl_llc2_ack_timeout = LLC2_ACK_TIME * HZ;
int sysctl_llc2_p_timeout = LLC2_P_TIME * HZ;
int sysctl_llc2_rej_timeout = LLC2_REJ_TIME * HZ;
int sysctl_llc2_busy_timeout = LLC2_BUSY_TIME * HZ;
+/**
+ * llc_conn_state_in_service - can this state drive the state machine?
+ * @state: state of connection
+ *
+ * Connection states are 1-based indexes into llc_conn_state_table[] and
+ * llc_offset_table[]. LLC_CONN_OUT_OF_SVC is not a state of the state
+ * machine at all: it marks a connection that has no transition table,
+ * either because it has not been brought up yet or because it has been
+ * torn down. Returns true if @state has a row in those tables.
+ */
+static bool llc_conn_state_in_service(u8 state)
+{
+ /*
+ * The tables are indexed with "state - 1", so the numbering has to be
+ * dense, start right after the LLC_CONN_OUT_OF_SVC sentinel, and end
+ * at NBR_CONN_STATES for the bounds below to be the real ones.
+ */
+ static_assert(LLC_CONN_OUT_OF_SVC == 0);
+ static_assert(LLC_CONN_STATE_ADM == LLC_CONN_OUT_OF_SVC + 1);
+ static_assert(LLC_CONN_STATE_TEMP == NBR_CONN_STATES);
+
+ return state > LLC_CONN_OUT_OF_SVC && state <= NBR_CONN_STATES;
+}
+
/**
* llc_conn_state_process - sends event to connection state machine
* @sk: connection
@@ -59,6 +86,15 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff
*skb)
struct llc_sock *llc = llc_sk(skb->sk);
struct llc_conn_state_ev *ev = llc_conn_ev(skb);
+ /*
+ * An out of service connection has no row in llc_conn_state_table[],
+ * so it cannot be driven by any event.
+ */
+ if (unlikely(!llc_conn_state_in_service(llc->state))) {
+ kfree_skb(skb);
+ return 1;
+ }
+
ev->ind_prim = ev->cfm_prim = 0;
/*
* Send event to state machine
@@ -354,7 +390,7 @@ 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 > NBR_CONN_STATES)
+ if (!llc_conn_state_in_service(llc->state))
goto out;
rc = 0;
trans = llc_qualify_conn_ev(sk, skb);
--
2.34.1
