Hi Dan,
On Wed, Feb 12, 2025 at 10:19 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Luiz Augusto von Dentz,
>
> Commit dd6367916d2d ("Bluetooth: L2CAP: Fix corrupted list in
> hci_chan_del") from Feb 6, 2025 (linux-next), leads to the following
> Smatch static checker warning:
>
> net/bluetooth/l2cap_core.c:7555 l2cap_recv_acldata()
> error: we previously assumed 'conn' could be null (see line 7459)
>
> net/bluetooth/l2cap_core.c
> 7442 void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> 7443 {
> 7444 struct l2cap_conn *conn;
> 7445 int len;
> 7446
> 7447 /* Lock hdev to access l2cap_data to avoid race with l2cap_conn_del */
> 7448 hci_dev_lock(hcon->hdev);
> 7449
> 7450 conn = hcon->l2cap_data;
> 7451
> 7452 if (!conn)
> 7453 conn = l2cap_conn_add(hcon);
> 7454
> 7455 conn = l2cap_conn_hold_unless_zero(conn);
> 7456
> 7457 hci_dev_unlock(hcon->hdev);
> 7458
> 7459 if (!conn)
> 7460 goto drop;
>
> Do you want to move drop after the unlock: or do you want to make this
> a direct return? Choices, choices...
>
> if (!conn) {
> kfree_skb(skb);
> return;
> }
Ouch, yeah it seems like we should be doing this instead of drop, do
you care to spin a patch for it or shall I do it myself?
> 7461
> 7462 BT_DBG("conn %p len %u flags 0x%x", conn, skb->len, flags);
> 7463
> 7464 mutex_lock(&conn->lock);
> 7465
> 7466 switch (flags) {
> 7467 case ACL_START:
> 7468 case ACL_START_NO_FLUSH:
> 7469 case ACL_COMPLETE:
> 7470 if (conn->rx_skb) {
> 7471 BT_ERR("Unexpected start frame (len %d)", skb->len);
> 7472 l2cap_recv_reset(conn);
> 7473 l2cap_conn_unreliable(conn, ECOMM);
> 7474 }
> 7475
> 7476 /* Start fragment may not contain the L2CAP length so just
> 7477 * copy the initial byte when that happens and use conn->mtu as
> 7478 * expected length.
> 7479 */
> 7480 if (skb->len < L2CAP_LEN_SIZE) {
> 7481 l2cap_recv_frag(conn, skb, conn->mtu);
> 7482 break;
> 7483 }
> 7484
> 7485 len = get_unaligned_le16(skb->data) + L2CAP_HDR_SIZE;
> 7486
> 7487 if (len == skb->len) {
> 7488 /* Complete frame received */
> 7489 l2cap_recv_frame(conn, skb);
> 7490 goto unlock;
> 7491 }
> 7492
> 7493 BT_DBG("Start: total len %d, frag len %u", len, skb->len);
> 7494
> 7495 if (skb->len > len) {
> 7496 BT_ERR("Frame is too long (len %u, expected len %d)",
> 7497 skb->len, len);
> 7498 l2cap_conn_unreliable(conn, ECOMM);
> 7499 goto drop;
> 7500 }
> 7501
> 7502 /* Append fragment into frame (with header) */
> 7503 if (l2cap_recv_frag(conn, skb, len) < 0)
> 7504 goto drop;
> 7505
> 7506 break;
> 7507
> 7508 case ACL_CONT:
> 7509 BT_DBG("Cont: frag len %u (expecting %u)", skb->len, conn->rx_len);
> 7510
> 7511 if (!conn->rx_skb) {
> 7512 BT_ERR("Unexpected continuation frame (len %d)", skb->len);
> 7513 l2cap_conn_unreliable(conn, ECOMM);
> 7514 goto drop;
> 7515 }
> 7516
> 7517 /* Complete the L2CAP length if it has not been read */
> 7518 if (conn->rx_skb->len < L2CAP_LEN_SIZE) {
> 7519 if (l2cap_recv_len(conn, skb) < 0) {
> 7520 l2cap_conn_unreliable(conn, ECOMM);
> 7521 goto drop;
> 7522 }
> 7523
> 7524 /* Header still could not be read just continue */
> 7525 if (conn->rx_skb->len < L2CAP_LEN_SIZE)
> 7526 break;
> 7527 }
> 7528
> 7529 if (skb->len > conn->rx_len) {
> 7530 BT_ERR("Fragment is too long (len %u, expected %u)",
> 7531 skb->len, conn->rx_len);
> 7532 l2cap_recv_reset(conn);
> 7533 l2cap_conn_unreliable(conn, ECOMM);
> 7534 goto drop;
> 7535 }
> 7536
> 7537 /* Append fragment into frame (with header) */
> 7538 l2cap_recv_frag(conn, skb, skb->len);
> 7539
> 7540 if (!conn->rx_len) {
> 7541 /* Complete frame received. l2cap_recv_frame
> 7542 * takes ownership of the skb so set the global
> 7543 * rx_skb pointer to NULL first.
> 7544 */
> 7545 struct sk_buff *rx_skb = conn->rx_skb;
> 7546 conn->rx_skb = NULL;
> 7547 l2cap_recv_frame(conn, rx_skb);
> 7548 }
> 7549 break;
> 7550 }
> 7551
> 7552 drop:
> 7553 kfree_skb(skb);
> 7554 unlock:
> --> 7555 mutex_unlock(&conn->lock);
> 7556 l2cap_conn_put(conn);
> 7557 }
>
> regards,
> dan carpenter
>
--
Luiz Augusto von Dentz
