Re: [syzbot] [net?] UBSAN: array-index-out-of-bounds in llc_conn_state_process (2)

[Kees Cook <kees@kernel.org>]

On Mon, Jun 03, 2024 at 08:59:20AM -0700, syzbot wrote:
> HEAD commit:    6d7ddd805123 Merge tag 'soc-fixes-6.9-3' of git://git.kern..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=12596604980000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=7144b4fe7fbf5900
> dashboard link: https://syzkaller.appspot.com/bug?extid=628f93722c08dc5aabe0
> compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> 
> Unfortunately, I don't have any reproducer for this issue yet.
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/4d60cb47fbb1/disk-6d7ddd80.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/f3ff90de7db5/vmlinux-6d7ddd80.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/d452970444cd/bzImage-6d7ddd80.xz
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+628f93722c08dc5aabe0@syzkaller.appspotmail.com
> 
> ------------[ cut here ]------------
> UBSAN: array-index-out-of-bounds in net/llc/llc_conn.c:694:24
> index -1 is out of range for type 'int [12][5]'
> CPU: 0 PID: 15346 Comm: syz-executor.4 Not tainted 6.9.0-rc7-syzkaller-00023-g6d7ddd805123 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> Call Trace:
>  <IRQ>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x16c/0x1f0 lib/dump_stack.c:114
>  ubsan_epilogue lib/ubsan.c:231 [inline]
>  __ubsan_handle_out_of_bounds+0x110/0x150 lib/ubsan.c:429
>  llc_find_offset net/llc/llc_conn.c:694 [inline]

static int llc_find_offset(int state, int ev_type)
...
                rc = llc_offset_table[state][3]; break;

But this seems to be racing against:

>  llc_qualify_conn_ev net/llc/llc_conn.c:401 [inline]

static const struct llc_conn_state_trans *llc_qualify_conn_ev(struct sock *sk,
                                                              struct sk_buff *skb)
...
        struct llc_conn_state *curr_state =
                                        &llc_conn_state_table[llc->state - 1]; // <<<<<<
...
        for (next_trans = curr_state->transitions +
                llc_find_offset(llc->state - 1, ev->type);

Otherwise the first one would have crashed too (a -1 array index). Is
something racing:

void llc_sk_free(struct sock *sk)
...
        llc->state = LLC_CONN_OUT_OF_SVC;	// = 0
        /* Stop all (possibly) running timers */
        llc_sk_stop_all_timers(sk, true);


>  llc_conn_service net/llc/llc_conn.c:366 [inline]
>  llc_conn_state_process+0x1381/0x14e0 net/llc/llc_conn.c:72
>  llc_process_tmr_ev net/llc/llc_c_ac.c:1445 [inline]
>  llc_conn_tmr_common_cb+0x450/0x8e0 net/llc/llc_c_ac.c:1331
>  call_timer_fn+0x1a0/0x610 kernel/time/timer.c:1793
>  expire_timers kernel/time/timer.c:1844 [inline]

Given this is in a timer, it seems likely, especially given the above
"llc_sk_stop_all_timer()" call. And llc_conn_tmr_common_cb() is reachable
from several timers:

void llc_conn_pf_cycle_tmr_cb(struct timer_list *t)
{
        struct llc_sock *llc = from_timer(llc, t, pf_cycle_timer.timer);

        llc_conn_tmr_common_cb(&llc->sk, LLC_CONN_EV_TYPE_P_TMR);
}

void llc_conn_busy_tmr_cb(struct timer_list *t)
{
        struct llc_sock *llc = from_timer(llc, t, busy_state_timer.timer);

        llc_conn_tmr_common_cb(&llc->sk, LLC_CONN_EV_TYPE_BUSY_TMR);
}

void llc_conn_ack_tmr_cb(struct timer_list *t)
{
        struct llc_sock *llc = from_timer(llc, t, ack_timer.timer);

        llc_conn_tmr_common_cb(&llc->sk, LLC_CONN_EV_TYPE_ACK_TMR);
}

void llc_conn_rej_tmr_cb(struct timer_list *t)
{
        struct llc_sock *llc = from_timer(llc, t, rej_sent_timer.timer);

        llc_conn_tmr_common_cb(&llc->sk, LLC_CONN_EV_TYPE_REJ_TMR);
}

llc_ui_release() does:

        sock_put(sk);
        sock_orphan(sk);
        sock->sk = NULL;
        llc_sk_free(sk);

And I see llc_sk_free() also does:

	sock_put(sk);

What holds locking on llc? The timer callback is locking itself, but I
don't see any locks in llc_sk_free(), but in theory there should be no
locks left?

What's supposed to be happening here? Moving the state assignment later
doesn't look right, given the explicit check here:

static void llc_process_tmr_ev(struct sock *sk, struct sk_buff *skb)
{
        if (llc_sk(sk)->state == LLC_CONN_OUT_OF_SVC) {
                printk(KERN_WARNING "%s: timer called on closed connection\n",
                       __func__);
                kfree_skb(skb);

Is it just that a lock is missing in llc_sk_free?

diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 5c0ac243b248..99c4f06477eb 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -974,7 +974,9 @@ void llc_sk_free(struct sock *sk)
 {
 	struct llc_sock *llc = llc_sk(sk);
 
+	bh_lock_sock(sk);
 	llc->state = LLC_CONN_OUT_OF_SVC;
+	bh_unlock_sock(sk);
 	/* Stop all (possibly) running timers */
 	llc_sk_stop_all_timers(sk, true);
 #ifdef DEBUG_LLC_CONN_ALLOC

-- 
Kees Cook