Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1708-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <0000000000007cea730610e083e8@google.com> <216c95d9-db1f-487a-bf3d-17a496422485@v0yd.nl>
In-Reply-To: <216c95d9-db1f-487a-bf3d-17a496422485@v0yd.nl>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Fri, 9 Feb 2024 08:36:42 -0500
Message-ID: <CABBYNZKPaMLK5+HnsRWR9jwpdZWvbbai6p9XbePhMYdKSYUPPg@mail.gmail.com>
Subject: Re: [syzbot] [bluetooth?] KASAN: slab-use-after-free Write in __hci_acl_create_connection_sync
To: =?UTF-8?Q?Jonas_Dre=C3=9Fler?= <verdre@v0yd.nl>
Cc: syzbot <syzbot+3f0a39be7a2035700868@syzkaller.appspotmail.com>, 
	davem@davemloft.net, edumazet@google.com, johan.hedberg@gmail.com, 
	kuba@kernel.org, linux-bluetooth@vger.kernel.org, 
	linux-kernel@vger.kernel.org, luiz.von.dentz@intel.com, marcel@holtmann.org, 
	netdev@vger.kernel.org, pabeni@redhat.com, syzkaller-bugs@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Jonas,

On Fri, Feb 9, 2024 at 7:37=E2=80=AFAM Jonas Dre=C3=9Fler <verdre@v0yd.nl> =
wrote:
>
> Hi everyone!
>
> On 08.02.24 16:32, syzbot wrote:
> > syzbot has bisected this issue to:
> >
> > commit 456561ba8e495e9320c1f304bf1cd3d1043cbe7b
> > Author: Jonas Dre=C3=9Fler <verdre@v0yd.nl>
> > Date:   Tue Feb 6 11:08:13 2024 +0000
> >
> >      Bluetooth: hci_conn: Only do ACL connections sequentially
> >
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=3D154f8550=
180000
> > start commit:   b1d3a0e70c38 Add linux-next specific files for 20240208
> > git tree:       linux-next
> > final oops:     https://syzkaller.appspot.com/x/report.txt?x=3D174f8550=
180000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=3D134f8550180=
000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=3Dbb693ba1956=
62a06
> > dashboard link: https://syzkaller.appspot.com/bug?extid=3D3f0a39be7a203=
5700868
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=3D11d95147e=
80000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=3D107c2d8fe80=
000
> >
> > Reported-by: syzbot+3f0a39be7a2035700868@syzkaller.appspotmail.com
> > Fixes: 456561ba8e49 ("Bluetooth: hci_conn: Only do ACL connections sequ=
entially")
> >
> > For information about bisection process see: https://goo.gl/tpsmEJ#bise=
ction
>
> Hmm, looking at the backtraces, I think the issue that the introduction o=
f the
> sequential connect has introduced another async case: In hci_connect_acl(=
), when
> we call hci_acl_create_connection_sync(), the conn state no longer immedi=
ately
> gets set to BT_CONNECT, but remains in BT_OPEN or BT_CLOSED until the hci=
_sync
> queue actually executes __hci_acl_create_connection_sync().

Need to double check but I think we do set BT_CONNECT in case of LE
when it is queued so which shall prevent it to be queued multiple
times.

> This means that now hci_connect_acl() is happy to do multiple
> hci_acl_create_connection_sync calls, and the hci_sync machinery will hap=
pily
> execute them right after each other. Then the newly introduced hci_abort_=
conn_sync()
> in __hci_acl_create_connection_sync() calls hci_conn_del() and frees the =
conn
> object, so the second time we enter __hci_acl_create_connection_sync(),
> things blow up.
>
> It looks to me like in theory the hci_connect_le_sync() logic is prone to=
 a
> similar issue, but in practice that's prohibited because in hci_connect_l=
e_sync()
> we lookup whether the conn object still exists and bail out if it doesn't=
.
>
> Even for LE though I think we can queue multiple hci_connect_le_sync() ca=
lls
> and those will happily send HCI_OP_LE_CREATE_CONN no matter what the conn=
ection
> state actually is?
>
> So assuming this analysis is correct, what do we do to fix this? It seems=
 to me
> that
>
> 1) we want a BT_CONNECT_QUEUED state for connections, so that the state
> machine covers this additional stage that we have for ACL and LE connecti=
ons now.

BT_CONNECT already indicates that connection procedure is in progress.

> 2) the conn object can still disappear while the __hci_acl_create_connect=
ion_sync()
> is queued, so we need something like the "if conn doesn't exist anymore, =
bail out"
> check from hci_connect_le_sync() in __hci_acl_create_connection_sync(), t=
oo.

Btw, I'd probably clean up the connect function and create something
like hci_connect/hci_connect_sync which takes care of the details
internally like it was done to abort.

> That said, the current check in hci_connect_le_sync() that's using the co=
nnection
> handle to lookup the conn does not seem great, aren't these handles re-us=
ed
> after connections are torn down?

Well we could perhaps do a lookup by pointer to see if the connection
hasn't been removed in the meantime, that said to force a clash on the
handles it need to happen in between abort, which frees the handle,
and connect, anyway the real culprit here is that we should be able to
abort the cmd_sync callback like we do in LE:

https://github.com/bluez/bluetooth-next/blob/master/net/bluetooth/hci_conn.=
c#L2943

That way we stop the connect callback to run and don't have to worry
about handle re-use.

> Cheers,
> Jonas


--=20
Luiz Augusto von Dentz