Re: [PATCH net 1/1] llc: fix listener child socket leak on non-SABME frames

[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://netdev-ai.bots.linux.dev/sashiko/#/patchset/631a34bdb0c1e1ab47ba75a4be94b5129b7a122f.1784725007.git.zihanx@nebusec.ai?part=1
---
llc: fix listener child socket leak on non-SABME frames

This patch restricts child socket creation in llc_conn_handler() to
frames that are actual SABME commands, by gating the TCP_LISTEN branch
on llc_conn_ev_rx_sabme_cmd_pbit_set_x(). Non-SABME traffic then falls
through to the else branch, which attaches skb->sk to the listener and
runs the LLC connection state machine on the listener socket itself.

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 260460d50f54..7bf3f7f517b3 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -794,7 +794,8 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
>  	 * the originator of the new connection, and this state has to be
>  	 * in the newly created struct sock private area. -acme
>  	 */
> -	if (unlikely(sk->sk_state == TCP_LISTEN)) {
> +	if (unlikely(sk->sk_state == TCP_LISTEN) &&
> +	    !llc_conn_ev_rx_sabme_cmd_pbit_set_x(sk, skb)) {
>  		struct sock *newsk = llc_create_incoming_sock(sk, skb->dev,
>  							      &saddr, &daddr);
>  		if (!newsk)

[Critical]
When the new condition is false (non-SABME frame arriving at a listener),
control now falls into the else branch and drives the listener itself
through llc_conn_rcv() -> llc_conn_state_process() -> llc_conn_service().
Is that intentional?

The listener's llc_sock state is set by llc_sk_init() to
LLC_CONN_STATE_ADM and is not updated by llc_ui_listen():

  net/llc/llc_c_ac.c / llc_sk_init():
    llc->state = LLC_CONN_STATE_ADM;

In net/llc/llc_c_st.c the ADM state table contains llc_adm_state_trans_5
with .ev = llc_conn_ev_rx_any_frame (an unconditional match) and
.next_state = LLC_CONN_OUT_OF_SVC, which equals 0. After a single
non-DISC, non-XXX-with-P=1 frame (for example an I-frame or S-frame with
P=0, or a DM/UA/FRMR response) matches trans_5, llc_conn_service() will
write llc_sk(listener)->state = 0.

On the following non-SABME frame reaching the same listener,
llc_conn_service() only guards:

    if (llc->state > NBR_CONN_STATES)
        goto out;

so state == 0 passes through, and llc_qualify_conn_ev() then computes:

    curr_state = &llc_conn_state_table[llc->state - 1];

which indexes llc_conn_state_table[-1], followed by
llc_find_offset(llc->state - 1, ev->type) indexing llc_offset_table[-1]
[...]. Both are out-of-bounds reads before a 12-entry array. The garbage
curr_state->transitions pointer is then iterated and eventually
(*next_trans)->ev(sk, skb) is called through an attacker-triggerable but
uninfluenced function pointer.

Since llc_lookup_listener() will match any frame carrying the listener's
SAP when no established socket exists, a peer on the local segment can
reach this with two crafted LLC frames.

Should the else branch continue to skip the connection state machine
for a listening socket, or should the ADM tables be prevented from
executing against a TCP_LISTEN sk in the first place?

[Medium]
Is the wire-level behavior for DISC and other pbit=1 commands against a
listener intended to change here?

Prior to this patch, llc_create_incoming_sock() ran first and did:

    memcpy(&newllc->daddr, saddr, sizeof(newllc->daddr));

so DM responses generated by the ADM state transitions were addressed to
the peer that sent the DISC. After the patch, the ADM transitions run on
the listener itself.

In net/llc/llc_c_st.c the ADM table includes llc_adm_state_trans_3 for
DISC commands, whose action is llc_conn_ac_send_dm_rsp_f_set_p() in
net/llc/llc_c_ac.c. That builds the DM response using llc->daddr.lsap
(via llc_pdu_header_init) and llc->daddr.mac (via llc_mac_hdr_init).

For a socket()+bind()+listen() socket, llc->daddr is never populated:
llc_ui_bind() only writes llc->laddr and llc->dev, and llc_sk_init()
zero-initializes daddr. The DM response therefore goes to MAC
00:00:00:00:00:00 with destination LSAP 0. The same concern applies to
llc_adm_state_trans_4 (rx_xxx_cmd_pbit_set_1 ->
llc_conn_ac_send_dm_rsp_f_set_1).

Should the commit message call out this on-wire behavioral change for
legitimate DISC traffic against a listening socket, or should the else
branch avoid running these transitions on a listener whose daddr is
unset?