Re: There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c
From: Arnd Bergmann <arnd@arndb.de>
Date: 2021-09-24 09:20:26
Also in:
lkml, netdev
On Fri, Sep 24, 2021 at 10:44 AM butt3rflyh4ck [off-list ref] 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.
quoted hunk
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