| Message ID | 4FB9932B.9070101@samsung.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Minho, * Minho Ban <mhban@samsung.com> [2012-05-21 09:58:19 +0900]: > If l2cap_chan_send is called will null conn it will cause kernel Oops. > This patch checks if conn is valid before entering l2cap_chan_send. chan->conn should be always valid, and if not we have a bug somewhere else in the code and not in l2cap_chan_send(). It could be a locking problem maybe. Also check if you can reproduce this with latest bluetooth-next. Gustavo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/22/2012 01:17 AM, Gustavo Padovan wrote: > Hi Minho, > > * Minho Ban <mhban@samsung.com> [2012-05-21 09:58:19 +0900]: > >> If l2cap_chan_send is called will null conn it will cause kernel Oops. >> This patch checks if conn is valid before entering l2cap_chan_send. > > chan->conn should be always valid, and if not we have a bug somewhere else in > the code and not in l2cap_chan_send(). It could be a locking problem maybe. > Also check if you can reproduce this with latest bluetooth-next. > > Gustavo > Thanks for comment. I'm using bluetooth-next backporting to kernel 3.0 I wonder how do we guarantee chan->conn is valid if other thread release chan->lock just after exit l2cap_chan_del. It seem l2cap_chan_del is well protected with various mutex (eg, sk, conn, chan) but that may not enough to prevent lock waiters from accessing object. Regards, Minho Ban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 2012년 05월 21일 09:58, Minho Ban wrote: > If l2cap_chan_send is called will null conn it will cause kernel Oops. > This patch checks if conn is valid before entering l2cap_chan_send. > > 2871 [ 177.271861] Unable to handle kernel NULL pointer dereference at virtual address 00000010 > 2872 [ 177.271867] pgd = eadc0000 > 2873 [ 177.271870] [00000010] *pgd=6ad03831, *pte=00000000, *ppte=00000000 > 2874 [ 177.271884] Internal error: Oops: 17 [#1] PREEMPT SMP > 2875 [ 177.271891] Modules linked in: > 2876 [ 177.271900] CPU: 3 Not tainted (3.0.15-00019-g097836e #36) > 2877 [ 177.271925] PC is at l2cap_chan_send+0x8c/0x6f8 > 2878 [ 177.271933] LR is at 0xc089d59c > 2879 [ 177.271940] pc : [<c0531514>] lr : [<c089d59c>] psr: 80000013 > 2880 [ 177.271943] sp : ddda1d50 ip : 0000035c fp : ddda1dac > 2881 [ 177.271948] r10: 0000035c r9 : 00000000 r8 : ddda1f3c > 2882 [ 177.271954] r7 : ed67ec00 r6 : 00000006 r5 : ed67ec00 r4 : 00000003 > 2883 [ 177.271959] r3 : ddda1d7c r2 : 0000035c r1 : 00000000 r0 : ed67ec00 > 2884 [ 177.271967] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > 2885 [ 177.271973] Control: 10c5387d Table: 6adc004a DAC: 00000015 > ~~~ snip ~~~~~ > 3000 [ 177.272989] Backtrace: > 3001 [ 177.273001] [<c0531488>] (l2cap_chan_send+0x0/0x6f8) from [<c053700c>] (l2cap_sock_sendmsg+0xc0/0x15c) > 3002 [ 177.273023] [<c0536f4c>] (l2cap_sock_sendmsg+0x0/0x15c) from [<c0458afc>] (sock_sendmsg+0xcc/0xd4) > 3003 [ 177.273035] [<c0458a30>] (sock_sendmsg+0x0/0xd4) from [<c0459558>] (sys_sendto+0xb8/0xdc) > 3004 [ 177.273041] r9:ddda0000 r8:00004040 r7:00000000 r6:ebdf9d40 r5:0004fb00 > 3005 [ 177.273050] r4:0000035c > 3006 [ 177.273059] [<c04594a0>] (sys_sendto+0x0/0xdc) from [<c045959c>] (sys_send+0x20/0x28) > 3007 [ 177.273077] [<c045957c>] (sys_send+0x0/0x28) from [<c00431c0>] (ret_fast_syscall+0x0/0x30) > 3008 [ 177.273087] Code: e5901004 e24b3030 e51ba050 e590e21c (e5914010) > 3009 [ 177.273096] ---[ end trace 29a418305c9cffba ]--- > > Signed-off-by: Minho Ban<mhban@samsung.com> > --- > net/bluetooth/l2cap_sock.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index 3bb1611..98d4541 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -727,10 +727,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms > if (msg->msg_flags& MSG_OOB) > return -EOPNOTSUPP; > > - if (sk->sk_state != BT_CONNECTED) > + l2cap_chan_lock(chan); > + if (sk->sk_state != BT_CONNECTED || !chan->conn) { > + l2cap_chan_unlock(chan); > return -ENOTCONN; > + } > > - l2cap_chan_lock(chan); > err = l2cap_chan_send(chan, msg, len, sk->sk_priority); > l2cap_chan_unlock(chan); > Beside !chan->conn condition,I think it makes sense that sk_state check should be moved after l2cap_chan_lock() because sk_state could be changed due to l2cap_conn_del(). I think sk->state could be protected by l2cap_chan_lock() in the below procedure. l2cap_conn_del() ->l2cap_chan_lock() ->l2cap_chan_del() -> lock_sock() __l2cap_sock_state_change() * -> release_sock() ->l2cap_chan_unlock() BR Chanyeol Park. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/22/2012 09:35 PM, Chanyeol Park wrote: > Hi > On 2012년 05월 21일 09:58, Minho Ban wrote: >> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c >> index 3bb1611..98d4541 100644 >> --- a/net/bluetooth/l2cap_sock.c >> +++ b/net/bluetooth/l2cap_sock.c >> @@ -727,10 +727,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms >> if (msg->msg_flags& MSG_OOB) >> return -EOPNOTSUPP; >> >> - if (sk->sk_state != BT_CONNECTED) >> + l2cap_chan_lock(chan); >> + if (sk->sk_state != BT_CONNECTED || !chan->conn) { >> + l2cap_chan_unlock(chan); >> return -ENOTCONN; >> + } >> >> - l2cap_chan_lock(chan); >> err = l2cap_chan_send(chan, msg, len, sk->sk_priority); >> l2cap_chan_unlock(chan); >> > Beside !chan->conn condition,I think it makes sense that sk_state check should be moved after l2cap_chan_lock() > because sk_state could be changed due to l2cap_conn_del(). > Thanks, chan->conn condition is not necessary, move sk->sk_state != BT_CONNECTED behind chan_lock is enough. I'll amend this patch. Regards Minho Ban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 3bb1611..98d4541 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -727,10 +727,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms if (msg->msg_flags & MSG_OOB) return -EOPNOTSUPP; - if (sk->sk_state != BT_CONNECTED) + l2cap_chan_lock(chan); + if (sk->sk_state != BT_CONNECTED || !chan->conn) { + l2cap_chan_unlock(chan); return -ENOTCONN; + } - l2cap_chan_lock(chan); err = l2cap_chan_send(chan, msg, len, sk->sk_priority); l2cap_chan_unlock(chan);
If l2cap_chan_send is called will null conn it will cause kernel Oops. This patch checks if conn is valid before entering l2cap_chan_send. 2871 [ 177.271861] Unable to handle kernel NULL pointer dereference at virtual address 00000010 2872 [ 177.271867] pgd = eadc0000 2873 [ 177.271870] [00000010] *pgd=6ad03831, *pte=00000000, *ppte=00000000 2874 [ 177.271884] Internal error: Oops: 17 [#1] PREEMPT SMP 2875 [ 177.271891] Modules linked in: 2876 [ 177.271900] CPU: 3 Not tainted (3.0.15-00019-g097836e #36) 2877 [ 177.271925] PC is at l2cap_chan_send+0x8c/0x6f8 2878 [ 177.271933] LR is at 0xc089d59c 2879 [ 177.271940] pc : [<c0531514>] lr : [<c089d59c>] psr: 80000013 2880 [ 177.271943] sp : ddda1d50 ip : 0000035c fp : ddda1dac 2881 [ 177.271948] r10: 0000035c r9 : 00000000 r8 : ddda1f3c 2882 [ 177.271954] r7 : ed67ec00 r6 : 00000006 r5 : ed67ec00 r4 : 00000003 2883 [ 177.271959] r3 : ddda1d7c r2 : 0000035c r1 : 00000000 r0 : ed67ec00 2884 [ 177.271967] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user 2885 [ 177.271973] Control: 10c5387d Table: 6adc004a DAC: 00000015 ~~~ snip ~~~~~ 3000 [ 177.272989] Backtrace: 3001 [ 177.273001] [<c0531488>] (l2cap_chan_send+0x0/0x6f8) from [<c053700c>] (l2cap_sock_sendmsg+0xc0/0x15c) 3002 [ 177.273023] [<c0536f4c>] (l2cap_sock_sendmsg+0x0/0x15c) from [<c0458afc>] (sock_sendmsg+0xcc/0xd4) 3003 [ 177.273035] [<c0458a30>] (sock_sendmsg+0x0/0xd4) from [<c0459558>] (sys_sendto+0xb8/0xdc) 3004 [ 177.273041] r9:ddda0000 r8:00004040 r7:00000000 r6:ebdf9d40 r5:0004fb00 3005 [ 177.273050] r4:0000035c 3006 [ 177.273059] [<c04594a0>] (sys_sendto+0x0/0xdc) from [<c045959c>] (sys_send+0x20/0x28) 3007 [ 177.273077] [<c045957c>] (sys_send+0x0/0x28) from [<c00431c0>] (ret_fast_syscall+0x0/0x30) 3008 [ 177.273087] Code: e5901004 e24b3030 e51ba050 e590e21c (e5914010) 3009 [ 177.273096] ---[ end trace 29a418305c9cffba ]--- Signed-off-by: Minho Ban <mhban@samsung.com> --- net/bluetooth/l2cap_sock.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)