Re: [PATCH V2] Bluetooth: rfcomm: Fix null-ptr-deref in rfcomm_check_security

[Yuxuan-Hu]

Hi Paul,


Thank you for your kind reply.

I have made the changes you mentioned and submitted a new version of the 
patch.

https://patchwork.kernel.org/project/bluetooth/patch/20240103091043.3379363-1-20373622@xxxxxxxxxxx/

On 2024/1/3 16:44, Paul Menzel wrote:
> Dear Yuxuan,
>
>
> Thank you for your patch.
>
> Am 03.01.24 um 04:14 schrieb Yuxuan Hu:
> > During our fuzz testing of the connection and disconnection process 
> > at the
> > RFCOMM layer,we discovered this bug.By comparing the packetsfrom a 
> > normal
>
> 1.  Please add a space after punctuation like full stops/periods and 
> commas. (There are more occurrences below.)
> 2.  packets from
>
> > connection and disconnection process with the testcase that triggered a
> > KASAN report, we analyzed the cause of this bug as follows:
>
> s/, we/. We/
>
> > 1. In the packets captured during a normal connection, the host sends a
> > `Read Encryption Key Size` type of `HCI_CMD` packet(Command Opcode: 
> > 0x1408)
>
> Please add a space before (.
>
> > 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 = 
> > l2cap_pi(sk)->chan->conn;`
> > 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.
> >
> > To fix this bug, check if `sk->sk_state` is BT_CLOSED before calling
> > rfcomm_recv_frame in rfcomm_process_rx.
>
> Is this bug covered by a test case already?


Yes, I have submitted this bug in bugzilla include KASAN details and a 
coverable test case.

Here is the link of it:

https://bugzilla.kernel.org/show_bug.cgi?id=218323


> > Signed-off-by: Yuxuan Hu <20373622@xxxxxxxxxxx>
> > ---
> > V1 -> V2: Removed the direct check for `conn` being null in 
> > rfcomm_check_security
> >
> >   net/bluetooth/rfcomm/core.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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 = skb_dequeue(&sk->sk_receive_queue))) {
> >           skb_orphan(skb);
> > -        if (!skb_linearize(skb)) {
> > +        if (!skb_linearize(skb) && sk->sk_state != BT_CLOSED) {
> >               s = rfcomm_recv_frame(s, skb);
> >               if (!s)
> >                   break;
>
> Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
>
>
> Kind regards,
>
> Paul

Sincerely,

Yuxuan