* There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c
@ 2021-09-24 8:44 butt3rflyh4ck
2021-09-24 9:19 ` Arnd Bergmann
0 siblings, 1 reply; 4+ messages in thread
From: butt3rflyh4ck @ 2021-09-24 8:44 UTC (permalink / raw)
To: isdn, arnd, David S. Miller; +Cc: netdev, LKML
Hi, there is an array-index-out-bounds bug in detach_capi_ctr in
drivers/isdn/capi/kcapi.c and I reproduce it on 5.15.0-rc2+.
###Analyze
we can call CMTPCONNADD ioctl and it would invoke
do_cmtp_sock_ioctl(), it would call cmtp_add_connection().
the chain of call is as follows.
ioctl(CMTPCONNADD)
->cmtp_sock_ioctl()
-->do_cmtp_sock_ioctl()
--->cmtp_add_connection()
---->kthread_run()
---->cmtp_attach_device()
the function would add a cmtp session to a controller. Let us see the code.
```
int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
{
u32 valid_flags = BIT(CMTP_LOOPBACK);
struct cmtp_session *session, *s;
int i, err;
BT_DBG("");
if (!l2cap_is_socket(sock))
return -EBADFD;
if (req->flags & ~valid_flags)
return -EINVAL;
session = kzalloc(sizeof(struct cmtp_session), GFP_KERNEL); ///
alloc and clear struct cmtp_session 'session' by kzalloc().
if (!session)
return -ENOMEM;
[...]
session->task = kthread_run(cmtp_session, session,
"kcmtpd_ctr_%d",session->num); /// create and run a kernel thread
invoke cmtp_session() and the args is 'session'
[...]
if (!(session->flags & BIT(CMTP_LOOPBACK))) {
err = cmtp_attach_device(session); /// invoke cmtp_attach_device()
to attach a session with a controller.
if (err < 0) {
/* Caller will call fput in case of failure, and so
* will cmtp_session kthread.
*/
get_file(session->sock->file);
atomic_inc(&session->terminate);
wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(&cmtp_session_sem);
return err;
}
}
[...]
```
the struct cmtp_session have a member named struct capi_ctr 'ctrl'.
struct capi_ctr have a member named 'cnr', it is controller number.
the kernel thread would invoke cmtp_session(), it would call
cmtp_detach_device() to detach a session and the registration of a
controller.
cmtp_session()
->cmtp_detach_device()
->detach_capi_ctr()
let us see detach_capi_ctr() function implement.
```
int detach_capi_ctr(struct capi_ctr *ctr)
{
int err = 0;
mutex_lock(&capi_controller_lock);
ctr_down(ctr, CAPI_CTR_DETACHED);
if (capi_controller[ctr->cnr - 1] != ctr) { /// use
cmtp_session->capi_ctr->cnr
err = -EINVAL;
goto unlock_out;
}
capi_controller[ctr->cnr - 1] = NULL;
ncontrollers--;
[...]
```
If the cmtp_add_connection() call cmtp_attach_device() not yet, the
cmtp_session->capi_ctr->cnr just is an ZERO.
The capi_controller[-1] make no sense. so should check that the
cmtp_session->capi_ctr->cnr is not an ZERO.
###Crash
root@syzkaller:/home/user# ./detach_capi_ctr
[ 41.829295][ C1] random: crng init done
[ 41.829769][ C1] random: 7 urandom warning(s) missed due to ratelimiting
[ 43.904027][ T2627] Bluetooth: hci0: command 0x0409 tx timeout
[ 45.984105][ T2911] Bluetooth: hci0: command 0x041b tx timeout
[ 46.863815][ T6447] Bluetooth: Found 0 CAPI controller(s) on device
10:aa:aa:aa:aa:aa
[ 46.864775][ T6479]
================================================================================
[ 46.866069][ T6479] UBSAN: array-index-out-of-bounds in
drivers/isdn/capi/kcapi.c:483:21
[ 46.867196][ T6479] index -1 is out of range for type 'capi_ctr *[32]'
[ 46.867982][ T6479] CPU: 1 PID: 6479 Comm: kcmtpd_ctr_0 Not tainted
5.15.0-rc2+ #8
[ 46.869002][ T6479] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.14.0-2 04/01/2014
[ 46.870107][ T6479] Call Trace:
[ 46.870473][ T6479] dump_stack_lvl+0x57/0x7d
[ 46.870974][ T6479] ubsan_epilogue+0x5/0x40
[ 46.871458][ T6479] __ubsan_handle_out_of_bounds.cold+0x43/0x48
[ 46.872135][ T6479] detach_capi_ctr+0x64/0xc0
[ 46.872639][ T6479] cmtp_session+0x5c8/0x5d0
[ 46.873131][ T6479] ? __init_waitqueue_head+0x60/0x60
[ 46.873712][ T6479] ? cmtp_add_msgpart+0x120/0x120
[ 46.874256][ T6479] kthread+0x147/0x170
[ 46.874709][ T6479] ? set_kthread_struct+0x40/0x40
[ 46.875248][ T6479] ret_from_fork+0x1f/0x30
[ 46.875773][ T6479]
================================================================================
[ 46.876799][ T6479] Kernel panic - not syncing: panic_on_warn set ...
[ 46.877541][ T6479] CPU: 1 PID: 6479 Comm: kcmtpd_ctr_0 Not tainted
5.15.0-rc2+ #8
[ 46.878384][ T6479] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.14.0-2 04/01/2014
[ 46.879377][ T6479] Call Trace:
[ 46.879742][ T6479] dump_stack_lvl+0x57/0x7d
[ 46.880251][ T6479] panic+0x139/0x302
[ 46.880699][ T6479] ubsan_epilogue+0x3f/0x40
[ 46.881199][ T6479] __ubsan_handle_out_of_bounds.cold+0x43/0x48
[ 46.881890][ T6479] detach_capi_ctr+0x64/0xc0
[ 46.882389][ T6479] cmtp_session+0x5c8/0x5d0
[ 46.882881][ T6479] ? __init_waitqueue_head+0x60/0x60
[ 46.883448][ T6479] ? cmtp_add_msgpart+0x120/0x120
[ 46.883989][ T6479] kthread+0x147/0x170
[ 46.884435][ T6479] ? set_kthread_struct+0x40/0x40
[ 46.884989][ T6479] ret_from_fork+0x1f/0x30
[ 46.885561][ T6479] Kernel Offset: disabled
[ 46.886043][ T6479] Rebooting in 86400 seconds..
###Patch
Should check the cmtp_session->capi_ctr->cnr is not an ZERO.
```
diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
index cb0afe897162..38502a955f13 100644
--- a/drivers/isdn/capi/kcapi.c
+++ b/drivers/isdn/capi/kcapi.c
@@ -480,7 +480,7 @@ int detach_capi_ctr(struct capi_ctr *ctr)
ctr_down(ctr, CAPI_CTR_DETACHED);
- if (capi_controller[ctr->cnr - 1] != ctr) {
+ if (!ctr->cnr || capi_controller[ctr->cnr - 1] != ctr) {
err = -EINVAL;
goto unlock_out;
}
```
Regards,
butt3rflyh4ck.
--
Active Defense Lab of Venustech
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c
2021-09-24 8:44 There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c butt3rflyh4ck
@ 2021-09-24 9:19 ` Arnd Bergmann
2021-09-24 10:02 ` butt3rflyh4ck
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2021-09-24 9:19 UTC (permalink / raw)
To: butt3rflyh4ck
Cc: Karsten Keil, Arnd Bergmann, David S. Miller, Networking, LKML,
Bluez mailing list, Marcel Holtmann, Johan Hedberg,
Luiz Augusto von Dentz
On Fri, Sep 24, 2021 at 10:44 AM butt3rflyh4ck
<butterflyhuangxx@gmail.com> wrote:
>
> Hi, there is an array-index-out-bounds bug in detach_capi_ctr in
> drivers/isdn/capi/kcapi.c and I reproduce it on 5.15.0-rc2+.
Thank you for the detailed report!
> ###Analyze
> we can call CMTPCONNADD ioctl and it would invoke
> do_cmtp_sock_ioctl(), it would call cmtp_add_connection().
> the chain of call is as follows.
> ioctl(CMTPCONNADD)
> ->cmtp_sock_ioctl()
> -->do_cmtp_sock_ioctl()
> --->cmtp_add_connection()
> ---->kthread_run()
> ---->cmtp_attach_device()
> the function would add a cmtp session to a controller. Let us see the code.
When I last touched the capi code, I tried to remove it all, but we then
left it in the kernel because the bluetooth cmtp code can still theoretically
use it.
May I ask how you managed to run into this? Did you find the bug through
inspection first and then produce it using cmtp, or did you actually use
cmtp?
If the only purpose of cmtp is now to be a target for exploits, then I
would suggest we consider removing both cmtp and capi for
good after backporting your fix to stable kernels. Obviously
if it turns out that someone actually uses cmtp and/or capi, we
should not remove it.
> If the cmtp_add_connection() call cmtp_attach_device() not yet, the
> cmtp_session->capi_ctr->cnr just is an ZERO.
>
> The capi_controller[-1] make no sense. so should check that the
> cmtp_session->capi_ctr->cnr is not an ZERO
I would consider that a problem in cmtp, not in capi, though making
capi more robust as in your patch does address the immediate issue.
> diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
> index cb0afe897162..38502a955f13 100644
> --- a/drivers/isdn/capi/kcapi.c
> +++ b/drivers/isdn/capi/kcapi.c
> @@ -480,7 +480,7 @@ int detach_capi_ctr(struct capi_ctr *ctr)
>
> ctr_down(ctr, CAPI_CTR_DETACHED);
>
> - if (capi_controller[ctr->cnr - 1] != ctr) {
> + if (!ctr->cnr || capi_controller[ctr->cnr - 1] != ctr) {
> err = -EINVAL;
> goto unlock_out;
> }
I think the API that is meant to be used here is
get_capi_ctr_by_nr(), which has that check.
Arnd
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c
2021-09-24 9:19 ` Arnd Bergmann
@ 2021-09-24 10:02 ` butt3rflyh4ck
2021-09-28 8:35 ` butt3rflyh4ck
0 siblings, 1 reply; 4+ messages in thread
From: butt3rflyh4ck @ 2021-09-24 10:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Karsten Keil, David S. Miller, Networking, LKML,
Bluez mailing list, Marcel Holtmann, Johan Hedberg,
Luiz Augusto von Dentz
> When I last touched the capi code, I tried to remove it all, but we then
> left it in the kernel because the bluetooth cmtp code can still theoretically
> use it.
>
> May I ask how you managed to run into this? Did you find the bug through
> inspection first and then produce it using cmtp, or did you actually use
> cmtp?
I fuzz the bluez system and find a crash to analyze it and reproduce it.
> If the only purpose of cmtp is now to be a target for exploits, then I
> would suggest we consider removing both cmtp and capi for
> good after backporting your fix to stable kernels. Obviously
> if it turns out that someone actually uses cmtp and/or capi, we
> should not remove it.
>
Yes, I think this should be feasible.
Regards
butt3rflyh4ck.
--
Active Defense Lab of Venustech
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c
2021-09-24 10:02 ` butt3rflyh4ck
@ 2021-09-28 8:35 ` butt3rflyh4ck
0 siblings, 0 replies; 4+ messages in thread
From: butt3rflyh4ck @ 2021-09-28 8:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Karsten Keil, David S. Miller, Networking, LKML,
Bluez mailing list, Marcel Holtmann, Johan Hedberg,
Luiz Augusto von Dentz
[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]
Hi, I make a patch for this issue.
Regards,
butt3rflyh4ck.
On Fri, Sep 24, 2021 at 6:02 PM butt3rflyh4ck
<butterflyhuangxx@gmail.com> wrote:
>
> > When I last touched the capi code, I tried to remove it all, but we then
> > left it in the kernel because the bluetooth cmtp code can still theoretically
> > use it.
> >
> > May I ask how you managed to run into this? Did you find the bug through
> > inspection first and then produce it using cmtp, or did you actually use
> > cmtp?
>
> I fuzz the bluez system and find a crash to analyze it and reproduce it.
>
> > If the only purpose of cmtp is now to be a target for exploits, then I
> > would suggest we consider removing both cmtp and capi for
> > good after backporting your fix to stable kernels. Obviously
> > if it turns out that someone actually uses cmtp and/or capi, we
> > should not remove it.
> >
> Yes, I think this should be feasible.
>
> Regards
> butt3rflyh4ck.
>
>
> --
> Active Defense Lab of Venustech
--
Active Defense Lab of Venustech
[-- Attachment #2: 0001-isdn-cpai-check-ctr-cnr-to-avoid-array-index-out-of-.patch --]
[-- Type: text/x-patch, Size: 2214 bytes --]
From 15449bb5aac1f828dc82054651fe8f4dca37bef7 Mon Sep 17 00:00:00 2001
From: Xiaolong Huang <butterflyhuangxx@gmail.com>
Date: Tue, 28 Sep 2021 13:03:41 +0800
Subject: [PATCH] isdn: cpai: check ctr->cnr to avoid array index out of bound
The cmtp_add_connection() would add a cmtp session to a controller and run a kernel
thread to process cmtp.
__module_get(THIS_MODULE);
session->task = kthread_run(cmtp_session, session, "kcmtpd_ctr_%d",
session->num);
During this process, the kernel thread would call detach_capi_ctr()
to detach a register controller. if the controller was not attached yet, detach_capi_ctr()
would trigger an array-index-out-bounds bug.
[ 46.866069][ T6479] UBSAN: array-index-out-of-bounds in
drivers/isdn/capi/kcapi.c:483:21
[ 46.867196][ T6479] index -1 is out of range for type 'capi_ctr *[32]'
[ 46.867982][ T6479] CPU: 1 PID: 6479 Comm: kcmtpd_ctr_0 Not tainted
5.15.0-rc2+ #8
[ 46.869002][ T6479] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.14.0-2 04/01/2014
[ 46.870107][ T6479] Call Trace:
[ 46.870473][ T6479] dump_stack_lvl+0x57/0x7d
[ 46.870974][ T6479] ubsan_epilogue+0x5/0x40
[ 46.871458][ T6479] __ubsan_handle_out_of_bounds.cold+0x43/0x48
[ 46.872135][ T6479] detach_capi_ctr+0x64/0xc0
[ 46.872639][ T6479] cmtp_session+0x5c8/0x5d0
[ 46.873131][ T6479] ? __init_waitqueue_head+0x60/0x60
[ 46.873712][ T6479] ? cmtp_add_msgpart+0x120/0x120
[ 46.874256][ T6479] kthread+0x147/0x170
[ 46.874709][ T6479] ? set_kthread_struct+0x40/0x40
[ 46.875248][ T6479] ret_from_fork+0x1f/0x30
[ 46.875773][ T6479]
Signed-off-by: Xiaolong Huang <butterflyhuangxx@gmail.com>
---
drivers/isdn/capi/kcapi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
index cb0afe897162..7313454e403a 100644
--- a/drivers/isdn/capi/kcapi.c
+++ b/drivers/isdn/capi/kcapi.c
@@ -480,6 +480,11 @@ int detach_capi_ctr(struct capi_ctr *ctr)
ctr_down(ctr, CAPI_CTR_DETACHED);
+ if (ctr->cnr < 1 || ctr->cnr - 1 >= CAPI_MAXCONTR) {
+ err = -EINVAL;
+ goto unlock_out;
+ }
+
if (capi_controller[ctr->cnr - 1] != ctr) {
err = -EINVAL;
goto unlock_out;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-28 8:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-24 8:44 There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c butt3rflyh4ck
2021-09-24 9:19 ` Arnd Bergmann
2021-09-24 10:02 ` butt3rflyh4ck
2021-09-28 8:35 ` butt3rflyh4ck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).