Re: There is an array-index-out-bounds bug in detach_capi_ctr in drivers/isdn/capi/kcapi.c

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 10:44 AM butt3rflyh4ck
<butterflyhuangxx@xxxxxxxxx> 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