llc: do not run the state machine on out of service connections

Hi,

syzbot found a KASAN/UBSAN out of bounds report in the LLC connection state machine, and has now bisected a reproducer for it:

https://lore.kernel.org/all/[email protected]

  BUG: KASAN: global-out-of-bounds in llc_qualify_conn_ev net/llc/llc_conn.c:394
  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]'

llc_conn_state_table[] and llc_offset_table[] 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 both tables with -1.

Add bounds checking, regularize connect(2) errno, and add KUnit tests for it all. See the respective patches for way more details. :)

While I did build testing and convinced myself that the KUnit tests (which pass only with the fixes applied) are sensible, I would like more eyes on these changes, as I'm not entirely sure the best way to test them in the real world.

Note that this collides with proposed patch[1] which also noted we needed to do better sanity checking of llc->state in llc_conn_service(). Since this check is needed in llc_conn_state_process() as well, I made it a common function and did other work of keeping the sizes of things double-checked with static asserts.

-Kees

[1] https://lore.kernel.org/all/8fa3c9e6d5dcf328979ed2bdc27817c11a5eff91.1787752861.git.zihanx@nebusec.ai/

Kees Cook (3):
llc: do not run the state machine on out of service connections
llc: report a closed connection for out of service sockets
llc: add KUnit tests for the connection state machine bounds

 net/llc/Kconfig            |  14 ++++
 include/net/llc_c_st.h     |   2 +-
 net/llc/llc_conn.c         |  42 ++++++++++-
 net/llc/llc_if.c           |  12 ++-
 net/llc/tests/conn_kunit.c | 146 +++++++++++++++++++++++++++++++++++++
 5 files changed, 213 insertions(+), 3 deletions(-)
 create mode 100644 net/llc/tests/conn_kunit.c

2.34.1

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.

---
 include/net/llc_c_st.h |  2 +-
 net/llc/llc_conn.c     | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 2 deletions(-)

index f52a4cc4880d..24577e0b17df 100644
@@ -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 */

index 260460d50f54..a4ae29b42300 100644
@@ -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

An unsolicited frame can move a bound PF_LLC socket from LLC_CONN_STATE_ADM to LLC_CONN_OUT_OF_SVC while the socket stays open from userspace's point of view. Both upper layer entry points handle that badly:

- llc_establish_connection() has no state check at all, so with the state machine now refusing the event, connect(2) would return the state machine's "1" failure indication as a positive syscall return value.

- llc_build_and_send_pkt() special cases LLC_CONN_STATE_ADM as -ECONNABORTED but falls through to -EBUSY for LLC_CONN_OUT_OF_SVC. -EBUSY describes a connection that is momentarily unable to send, not one that no longer exists.

Report -ECONNABORTED from both.

There is deliberately no Fixes: tag here. The connect(2) return value only becomes observable once the previous patch makes the state machine refuse the event, and the llc_build_and_send_pkt() change is a long-standing errno inaccuracy with no memory safety impact. Backporting this on its own would fix nothing.

Build tested ARCH=x86_64 net/llc/ with GCC 14.2.0, CONFIG_LLC2=y and =m.

---
 net/llc/llc_if.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index 1514362e613d..f1a3f3372c4f 100644
@@ -41,7 +41,8 @@ int llc_build_and_send_pkt(struct sock *sk, struct sk_buff *skb)
 	int rc = -ECONNABORTED;
 	struct llc_sock *llc = llc_sk(sk);
 
-	if (unlikely(llc->state == LLC_CONN_STATE_ADM))
+	if (unlikely(llc->state == LLC_CONN_STATE_ADM ||
+		     llc->state == LLC_CONN_OUT_OF_SVC))
 		goto out_free;
 	rc = -EBUSY;
 	if (unlikely(llc_data_accept_state(llc->state) || /* data_conn_refuse */

@@ -82,6 +83,15 @@ int llc_establish_connection(struct sock *sk, const u8 *lmac, u8 *dmac, u8 dsap)
 	struct llc_sock *llc = llc_sk(sk);
 	struct sock *existing;
 
+	/*
+	 * A socket parked in LLC_CONN_OUT_OF_SVC has no state machine to run,
+	 * so there is nothing to establish. Report it as a closed connection
+	 * rather than handing llc_conn_state_process() an event it can only
+	 * throw away.
+	 */
+	if (unlikely(llc->state == LLC_CONN_OUT_OF_SVC))
+		return -ECONNABORTED;
+
 	laddr.lsap = llc->sap->laddr.lsap;
 	daddr.lsap = dsap;
 	memcpy(daddr.mac, dmac, sizeof(daddr.mac));

2.34.1

Add a KUnit suite for the LLC type 2 connection state machine. The test file is #included by llc_conn.c so that it can reach the static state machine helpers.

llc_conn_state_process_out_of_svc() is the regression test for the syzbot report: it feeds a PF_LLC socket sitting in LLC_CONN_STATE_ADM an I format command PDU with the P bit clear, which matches only the catch-all llc_adm_state_trans_5 transition and parks the socket in LLC_CONN_OUT_OF_SVC, then feeds it a second one.

Without the preceding fixes, on a CONFIG_UBSAN_BOUNDS=y plus CONFIG_KASAN=y kernel that case prints both halves of the reported splat and then dies:

  UBSAN: array-index-out-of-bounds in net/llc/llc_conn.c:712:24
  index -1 is out of range for type 'int [12][5]'
  BUG: KASAN: global-out-of-bounds in llc_conn_state_process

The other cases cover the 1-based indexing invariant of llc_conn_state_table[], the bounds of llc_conn_state_in_service(), and an event delivered with a state past the end of the table.

  $ ./tools/testing/kunit/kunit.py run --arch=x86_64 \
        --kconfig_add CONFIG_NET=y --kconfig_add CONFIG_LLC2=y llc2_conn
  [PASSED] llc_conn_state_table_is_one_based
  [PASSED] llc_conn_state_in_service_bounds
  [PASSED] llc_conn_state_process_out_of_svc
  [PASSED] llc_conn_state_process_bad_state

Build tested ARCH=x86_64 net/llc/ with GCC 14.2.0 at CONFIG_LLC2=y and CONFIG_LLC2=m (the suite needs CONFIG_LLC2=y). Tests run 4/4 passing on ARCH=um and on ARCH=x86_64 under qemu, and confirmed to fail with the two preceding patches reverted, both with CONFIG_KASAN=y and CONFIG_UBSAN_BOUNDS=y.

---
 net/llc/Kconfig            |  14 ++++
 net/llc/llc_conn.c         |   4 +
 net/llc/tests/conn_kunit.c | 146 +++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 net/llc/tests/conn_kunit.c

index 7f79f5e134f9..19bd101d6329 100644
@@ -8,3 +8,17 @@ config LLC2
 	help
 	  This is a Logical Link Layer type 2, connection oriented support.
 	  Select this if you want to have support for PF_LLC sockets.
+
+config LLC2_CONN_KUNIT_TEST
+	bool "KUnit tests for the LLC type 2 connection state machine" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y && LLC2=y
+	default KUNIT_ALL_TESTS
+	help
+	  This builds the KUnit tests for the LLC type 2 connection state
+	  machine, covering the bounds of the state transition tables and the
+	  handling of events delivered to an out of service connection.
+
+	  For more information on KUnit and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.

index a4ae29b42300..07f077a30c30 100644
@@ -1059,3 +1059,7 @@ void llc_sk_reset(struct sock *sk)
 	llc->failed_data_req	= 0 ;
 	llc->last_nr		= 0;
 }
+
+#if IS_ENABLED(CONFIG_LLC2_CONN_KUNIT_TEST)
+#include "tests/conn_kunit.c"
+#endif

2.34.1

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/#/patchset/[email protected]?part=3
---
llc: add KUnit tests for the connection state machine bounds

This commit adds a KUnit test suite for the LLC type 2 connection state machine. It introduces regression tests for state transitions and array indexing invariants.

[ ... ]