Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-810-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231226073957.328124-1-20373622@buaa.edu.cn>
In-Reply-To: <20231226073957.328124-1-20373622@buaa.edu.cn>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 2 Jan 2024 12:02:35 -0500
Message-ID: <CABBYNZJoH9WZ8cadJG9H63T5FvG0KS0uSNKrZFP4XvOPWy+APw@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: rfcomm: Fix null-ptr-deref in
To: Yuxuan Hu <20373622@buaa.edu.cn>
Cc: marcel@holtmann.org, johan.hedberg@gmail.com, 
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org, 
	baijiaju1990@gmail.com, sy2239101@buaa.edu.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Yuxuan,

On Tue, Dec 26, 2023 at 2:40=E2=80=AFAM Yuxuan Hu <20373622@buaa.edu.cn> wr=
ote:
>
> During our fuzz testing of the connection and disconnection process at th=
e
> RFCOMM layer,we discovered this bug.By comparing the packetsfrom a normal
> connection and disconnection process with the testcase that triggered a
> KASAN report, we analyzed the cause of this bug as follows:
>
> 1. In the packets captured during a normal connection, the host sends a
> `Read Encryption Key Size` type of `HCI_CMD` packet(Command Opcode: 0x140=
8)
> to the controller to inquire the length of encryption key.After receiving
> this packet, the controller immediately replies with a Command Complete
> packet (Event Code: 0x0e) to return the Encryption Key Size.
>
> 2. In our fuzz test case, the timing of the controller's response to this
> packet was delayed to an unexpected point: after the RFCOMM and L2CAP
> layers had disconnected but before the HCI layer had disconnected.
>
> 3. After receiving the Encryption Key Size Response at the time described
> in point 2, the host still called the rfcomm_check_security function.
> However, by this time `struct l2cap_conn *conn =3D l2cap_pi(sk)->chan->co=
nn;`
> had already been released, and when the function executed
> `return hci_conn_security(conn->hcon, d->sec_level, auth_type, d->out);`,
> specifically when accessing `conn->hcon`, a null-ptr-deref error occurred=
.
>
> Adding a check for whether `conn` is a null pointer to fix this bug.
>
> Signed-off-by: Yuxuan Hu <20373622@buaa.edu.cn>
> ---
>  net/bluetooth/rfcomm/core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 053ef8f25fae..5ffa197fdb48 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -228,6 +228,9 @@ static int rfcomm_check_security(struct rfcomm_dlc *d=
)
>                 auth_type =3D HCI_AT_NO_BONDING;
>                 break;
>         }
> +
> +       if (!conn)
> +               return 0;

This shall probably be handled earlier, perhaps on rfcomm_process_rx
if we can check that had been disconnected, that way we drop packets
that we know don't have any effect since later we do call
rfcomm_session_close e.g:

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 053ef8f25fae..1d34d8497033 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -1941,7 +1941,7 @@ static struct rfcomm_session
*rfcomm_process_rx(struct rfcomm_session *s)
        /* Get data directly from socket receive queue without copying it. =
*/
        while ((skb =3D skb_dequeue(&sk->sk_receive_queue))) {
                skb_orphan(skb);
-               if (!skb_linearize(skb)) {
+               if (!skb_linearize(skb) && sk->sk_state !=3D BT_CLOSED) {
                        s =3D rfcomm_recv_frame(s, skb);
                        if (!s)
                                break;

--=20
Luiz Augusto von Dentz