[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <202505091223.3C51585567@keescook>
Date: Fri, 9 May 2025 12:46:22 -0700
From: Kees Cook <kees@...nel.org>
To: syzbot <syzbot+628f93722c08dc5aabe0@...kaller.appspotmail.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
pabeni@...hat.com, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [net?] UBSAN: array-index-out-of-bounds in
llc_conn_state_process (2)
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@...kaller.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
Powered by blists - more mailing lists