Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1812-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>
 <CABBYNZKPaMLK5+HnsRWR9jwpdZWvbbai6p9XbePhMYdKSYUPPg@mail.gmail.com> <bbd64fd9-395b-441e-be04-39440359b035@v0yd.nl>
In-Reply-To: <bbd64fd9-395b-441e-be04-39440359b035@v0yd.nl>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 13 Feb 2024 08:53:06 -0500
Message-ID: <CABBYNZJ=SgwgNz7K9j0nK-aV-wgMF6_yZ9psF1ydWG7HAhwFYw@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 Mon, Feb 12, 2024 at 7:29=E2=80=AFPM Jonas Dre=C3=9Fler <verdre@v0yd.nl>=
 wrote:
>
> Hi Luiz,
>
> On 09.02.24 14:36, Luiz Augusto von Dentz wrote:
> > 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=3D154f85=
50180000
> >>> start commit:   b1d3a0e70c38 Add linux-next specific files for 202402=
08
> >>> git tree:       linux-next
> >>> final oops:     https://syzkaller.appspot.com/x/report.txt?x=3D174f85=
50180000
> >>> console output: https://syzkaller.appspot.com/x/log.txt?x=3D134f85501=
80000
> >>> kernel config:  https://syzkaller.appspot.com/x/.config?x=3Dbb693ba19=
5662a06
> >>> dashboard link: https://syzkaller.appspot.com/bug?extid=3D3f0a39be7a2=
035700868
> >>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=3D11d9514=
7e80000
> >>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=3D107c2d8fe=
80000
> >>>
> >>> Reported-by: syzbot+3f0a39be7a2035700868@syzkaller.appspotmail.com
> >>> Fixes: 456561ba8e49 ("Bluetooth: hci_conn: Only do ACL connections se=
quentially")
> >>>
> >>> For information about bisection process see: https://goo.gl/tpsmEJ#bi=
section
> >>
> >> Hmm, looking at the backtraces, I think the issue that the introductio=
n of the
> >> sequential connect has introduced another async case: In hci_connect_a=
cl(), when
> >> we call hci_acl_create_connection_sync(), the conn state no longer imm=
ediately
> >> 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.
>
> Nope, we set it only from within the hci_sync callback, see
> hci_connect_le_sync(). IMO that makes sense, because in
> hci_abort_conn_sync(), we send a  HCI_OP_CREATE_CONN_CANCEL in case
> of conn->state =3D=3D BT_CONNECT, and this OP we wouldn't want to send
> while the command is still waiting in the queue.

Yep, well I did a change which should work regardless of BT_CONNECT
only being set at the callback:

https://patchwork.kernel.org/project/bluetooth/patch/20240209141612.3554051=
-1-luiz.dentz@gmail.com/

> >
> >> This means that now hci_connect_acl() is happy to do multiple
> >> hci_acl_create_connection_sync calls, and the hci_sync machinery will =
happily
> >> execute them right after each other. Then the newly introduced hci_abo=
rt_conn_sync()
> >> in __hci_acl_create_connection_sync() calls hci_conn_del() and frees t=
he 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_connec=
t_le_sync()
> >> we lookup whether the conn object still exists and bail out if it does=
n't.
> >>
> >> Even for LE though I think we can queue multiple hci_connect_le_sync()=
 calls
> >> and those will happily send HCI_OP_LE_CREATE_CONN no matter what the c=
onnection
> >> state actually is?
> >>
> >> So assuming this analysis is correct, what do we do to fix this? It se=
ems to me
> >> that
> >>
> >> 1) we want a BT_CONNECT_QUEUED state for connections, so that the stat=
e
> >> machine covers this additional stage that we have for ACL and LE conne=
ctions now.
> >
> > BT_CONNECT already indicates that connection procedure is in progress.
>
> I still think an additional state is necessary. Alternatively (and maybe
> a lot nicer than the extra state) would be to add some functions to hci_s=
ync
> to check for and remove queued/ongoing commands, I'm thinking of a new
>
> bool hci_cmd_sync_has(struct hci_dev *hdev, hci_cmd_sync_work_func_t func=
, void *data);
> void hci_cmd_sync_cancel(struct hci_dev *hdev, hci_cmd_sync_work_func_t f=
unc, void *data);
>
> I think if we had those, in addition to not needing the additional state,
> we could also simplify the hci_abort_conn() code quite a bit and possibly
> get rid of the passing of connection handles to hci_connect_le_sync().
>
> I'll give that a try and propose a small patch tomorrow.

Yeah, I did something like in the past for hci_cmd_sync_queue_once, I
will resend it since it had the following function as well:

bool hci_cmd_sync_lookup(struct hci_dev *hdev, hci_cmd_sync_work_func_t fun=
c,
             void *data, hci_cmd_sync_work_destroy_t destroy,
             bool cancel);

> Cheers,
> Jonas
>
> >
> >> 2) the conn object can still disappear while the __hci_acl_create_conn=
ection_sync()
> >> is queued, so we need something like the "if conn doesn't exist anymor=
e, bail out"
> >> check from hci_connect_le_sync() in __hci_acl_create_connection_sync()=
, too.
> >
> > 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=
 connection
> >> handle to lookup the conn does not seem great, aren't these handles re=
-used
> >> 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_c=
onn.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