[PATCH net-next 0/3] llc: do not run the state machine on out of service connections

[PATCH net-next 0/3] llc: do not run the state machine on out of service connections

[Kees Cook]

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/6a95888b.4d659fcc.734b4.0051.GAE@google.com

  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

[PATCH net-next 1/3] llc: do not run the state machine on out of service connections

[Kees Cook]

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: syzbot+628f93722c08dc5aabe0@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6a95888b.4d659fcc.734b4.0051.GAE@google.com
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Assisted-by: Claude:claude-opus-5[1m]
Signed-off-by: Kees Cook <kees@kernel.org>
---
 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

[PATCH net-next 2/3] llc: report a closed connection for out of service sockets

[Kees Cook]

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.

Assisted-by: Claude:claude-opus-5[1m]
Signed-off-by: Kees Cook <kees@kernel.org>
---
 net/llc/llc_if.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/llc/llc_if.c b/net/llc/llc_if.c
index 1514362e613d..f1a3f3372c4f 100644
--- a/net/llc/llc_if.c
+++ b/net/llc/llc_if.c
@@ -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

[PATCH net-next 3/3] llc: add KUnit tests for the connection state machine bounds

[Kees Cook]

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.

Assisted-by: Claude:claude-opus-5[1m]
Signed-off-by: Kees Cook <kees@kernel.org>
---
 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

diff --git a/net/llc/Kconfig b/net/llc/Kconfig
index 7f79f5e134f9..19bd101d6329 100644
--- a/net/llc/Kconfig
+++ b/net/llc/Kconfig
@@ -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.
diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index a4ae29b42300..07f077a30c30 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -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
diff --git a/net/llc/tests/conn_kunit.c b/net/llc/tests/conn_kunit.c
new file mode 100644
index 000000000000..86d3f122df0a
--- /dev/null
+++ b/net/llc/tests/conn_kunit.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for the LLC type 2 connection state machine.
+ *
+ * This file is #included by llc_conn.c so that the tests can reach the
+ * static helpers of the state machine.
+ */
+#include <kunit/test.h>
+#include <linux/net.h>
+#include <net/net_namespace.h>
+
+/*
+ * Build the smallest event that reaches the LLC_CONN_STATE_ADM catch-all
+ * transition: an I format command PDU with the P bit clear. It matches
+ * neither llc_conn_ev_rx_sabme_cmd_pbit_set_x(),
+ * llc_conn_ev_rx_disc_cmd_pbit_set_x() nor
+ * llc_conn_ev_rx_xxx_cmd_pbit_set_1(), so llc_adm_state_trans_5 wins.
+ */
+static struct sk_buff *llc_conn_test_rx_pdu(struct kunit *test, struct sock *sk)
+{
+	struct llc_conn_state_ev *ev;
+	struct llc_pdu_sn *pdu;
+	struct sk_buff *skb;
+
+	skb = alloc_skb(sizeof(*pdu), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, skb);
+
+	skb_reset_network_header(skb);
+	pdu = skb_put(skb, sizeof(*pdu));
+	pdu->dsap   = 0x42;
+	pdu->ssap   = LLC_PDU_CMD;
+	pdu->ctrl_1 = LLC_PDU_TYPE_I;
+	pdu->ctrl_2 = 0;
+
+	skb->sk = sk;
+	ev = llc_conn_ev(skb);
+	memset(ev, 0, sizeof(*ev));
+	ev->type = LLC_CONN_EV_TYPE_PDU;
+
+	return skb;
+}
+
+static struct socket *llc_conn_test_socket(struct kunit *test)
+{
+	struct socket *sock;
+	int rc;
+
+	rc = sock_create_kern(&init_net, PF_LLC, SOCK_DGRAM, 0, &sock);
+	if (rc)
+		kunit_skip(test, "cannot create a PF_LLC socket: %d", rc);
+
+	return sock;
+}
+
+/*
+ * llc_conn_state_table[] and llc_offset_table[] are indexed with "state - 1",
+ * which only works while every state is its own 1-based index.
+ */
+static void llc_conn_state_table_is_one_based(struct kunit *test)
+{
+	u8 state;
+
+	for (state = LLC_CONN_STATE_ADM; state <= LLC_CONN_STATE_TEMP; state++)
+		KUNIT_EXPECT_EQ(test, llc_conn_state_table[state - 1].current_state,
+				state);
+}
+
+static void llc_conn_state_in_service_bounds(struct kunit *test)
+{
+	KUNIT_EXPECT_FALSE(test, llc_conn_state_in_service(LLC_CONN_OUT_OF_SVC));
+	KUNIT_EXPECT_TRUE(test, llc_conn_state_in_service(LLC_CONN_STATE_ADM));
+	KUNIT_EXPECT_TRUE(test, llc_conn_state_in_service(LLC_CONN_STATE_TEMP));
+	KUNIT_EXPECT_FALSE(test, llc_conn_state_in_service(LLC_CONN_STATE_TEMP + 1));
+	KUNIT_EXPECT_FALSE(test, llc_conn_state_in_service(U8_MAX));
+}
+
+/*
+ * Regression test for the syzbot report below: an unsolicited frame moves a
+ * socket sitting in LLC_CONN_STATE_ADM to LLC_CONN_OUT_OF_SVC, and the next
+ * frame for the same socket used to index llc_conn_state_table[-1] and
+ * llc_offset_table[-1][] before it was dropped.
+ *
+ * Link: https://lore.kernel.org/all/6a95888b.4d659fcc.734b4.0051.GAE@google.com
+ */
+static void llc_conn_state_process_out_of_svc(struct kunit *test)
+{
+	struct sk_buff *first, *second;
+	struct socket *sock;
+	struct sock *sk;
+
+	sock = llc_conn_test_socket(test);
+	sk = sock->sk;
+
+	first = llc_conn_test_rx_pdu(test, sk);
+	second = llc_conn_test_rx_pdu(test, sk);
+
+	lock_sock(sk);
+	KUNIT_EXPECT_EQ(test, llc_sk(sk)->state, LLC_CONN_STATE_ADM);
+
+	/* The catch-all ADM transition parks the socket out of service. */
+	KUNIT_EXPECT_EQ(test, llc_conn_state_process(sk, first), 0);
+	KUNIT_EXPECT_EQ(test, llc_sk(sk)->state, LLC_CONN_OUT_OF_SVC);
+
+	/* The next event must be refused rather than indexed with -1. */
+	KUNIT_EXPECT_NE(test, llc_conn_state_process(sk, second), 0);
+	KUNIT_EXPECT_EQ(test, llc_sk(sk)->state, LLC_CONN_OUT_OF_SVC);
+	release_sock(sk);
+
+	sock_release(sock);
+}
+
+/* The same refusal has to cover states past the end of the state table. */
+static void llc_conn_state_process_bad_state(struct kunit *test)
+{
+	struct socket *sock;
+	struct sk_buff *skb;
+	struct sock *sk;
+
+	sock = llc_conn_test_socket(test);
+	sk = sock->sk;
+
+	skb = llc_conn_test_rx_pdu(test, sk);
+
+	lock_sock(sk);
+	llc_sk(sk)->state = LLC_CONN_STATE_TEMP + 1;
+	KUNIT_EXPECT_NE(test, llc_conn_state_process(sk, skb), 0);
+	llc_sk(sk)->state = LLC_CONN_STATE_ADM;
+	release_sock(sk);
+
+	sock_release(sock);
+}
+
+static struct kunit_case llc_conn_test_cases[] = {
+	KUNIT_CASE(llc_conn_state_table_is_one_based),
+	KUNIT_CASE(llc_conn_state_in_service_bounds),
+	KUNIT_CASE(llc_conn_state_process_out_of_svc),
+	KUNIT_CASE(llc_conn_state_process_bad_state),
+	{}
+};
+
+static struct kunit_suite llc_conn_test_suite = {
+	.name = "llc2_conn",
+	.test_cases = llc_conn_test_cases,
+};
+
+kunit_test_suite(llc_conn_test_suite);
-- 
2.34.1