From nobody Thu Dec 18 07:52:47 2025 Received: from smtp-relay-internal-0.canonical.com (smtp-relay-internal-0.canonical.com [185.125.188.122]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ACF2F2391AB for ; Thu, 5 Dec 2024 07:17:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.125.188.122 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733383067; cv=none; b=U6SkYLH0kycdeCiUyV3Zd8uXhDOpguLasDMsQKANuEf2LNrIJL2U4LgEtTKilb9rlbN5mBXzNshgBejPk6EfPK6D4UUr3fr4MwmKDACRU/A7+9uLK9HuIg/5hPdXeTvhlk4ro5zkj3rDAr8JI0mO9WtA2d36VGGXMH2wOwS3iNs= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733383067; c=relaxed/simple; bh=tCAmYwcPUUuakZw/lHI+qHX63POcQUnz7J3kWSWQjRs=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=DpsOe6UGrwOP654Wi6fcgXdg1q1zQZpNNTmzic/0lyOBdXySi3/7JtF1YLiKh1JawP20VEgOMBMqDUwphMns3HbVAay3iX768xhrzmLF/S4nqc4D8L2iy7x3KP39MaYMeX3ozLPchr+L99UMre7gHoq06hV6+p6BQesJY8XQcRE= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=canonical.com; spf=pass smtp.mailfrom=canonical.com; dkim=pass (2048-bit key) header.d=canonical.com header.i=@canonical.com header.b=KlyrK9XI; arc=none smtp.client-ip=185.125.188.122 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=canonical.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=canonical.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=canonical.com header.i=@canonical.com header.b="KlyrK9XI" Received: from mail-oo1-f71.google.com (mail-oo1-f71.google.com [209.85.161.71]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 272FA3FDB1 for ; Thu, 5 Dec 2024 07:17:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1733383057; bh=uafyawx0PXFiUu2BYekHpyweJ4XNWrmQFuMU5DXhLa0=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=KlyrK9XI1n9/TbdXtrrbizxUonwc5sUDl8rRSqn6O6y33+SklOJ39yH8K0NxKYmK9 TxuYwH3SHRuiITKP9Gr9tRO1eDmKGq+UqJJsHtrVni2DFNAxLGPYm2eq3sI07pm5ZH Xr9CHPlDPcB6oB+PBxRDSPlP2cZMPDakt5LdjqiQVIS5gnDIZlm+Tf6nMM7DZf4IdF J0M+pjjbFFpiu+LKZCz+Lfizm5V5r2coWbdB2N7ooBnd4QGusDi4+7qK0ikOrjubbS tSHIqI3F7nIuzNEp85RjNezN4ETVsOWB3N9SBUGwwweux7R8D/lMhwk1YMLAp2RZeh waXA2GX1Nte1A== Received: by mail-oo1-f71.google.com with SMTP id 006d021491bc7-5f07a5621b7so539711eaf.2 for ; Wed, 04 Dec 2024 23:17:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733383055; x=1733987855; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=uafyawx0PXFiUu2BYekHpyweJ4XNWrmQFuMU5DXhLa0=; b=qWTqvLfIHc8/uZdsOwD9bZ0a/sH56/3yjfTIcSr6SkyZAFo8dFUPYlxYixdE7jHOZK 5CQS2BigVqyeRDFCmqM+ployvU02PCUz4SU2jH2/hSNtnaspriwq4sD8f7UU7eKRWq3n kwtXyRkt+wtc3We0tP+Ch4/wp+PNnYOpD6+ID8+O1NUWHKuQaTEKypalLM+nH9CYRZK4 ovJ2sEcxyQbBALlrRf2iKzQxXGY5y6i9soa8H8svecb8ZFWTapdX6j8FITzAJp1PtIKz Woy30tM+8aK7aPt3t7fZPnwrKYfmb4RXRIjlZcPOleKfepFwjwIsLc+fN+2akQCsI/7k 25Fg== X-Forwarded-Encrypted: i=1; AJvYcCUrqnTeCduA4ub5JClBDuclrDKcJqABvZ6I+8pTvtZvR+ASDBswjk5f0FpsNuymbh0UpKUCaRSuNG42Tf8=@vger.kernel.org X-Gm-Message-State: AOJu0YyUrOcfiwMdo1GBcoJKNjAocHzqw74QSDywanP+71NDfkP5X9Lb eM43DkYdYzmHXWvo3wgCu1ic4HQ/fen/KZBYVSt/+UM8hwqy9CORxRTQIrjPxB+sWE27KDx+VGl X3g5rtqRiy0axfmeZy8tYJuUW9TUqP72uxuBE2iuoSaSh9ND1lNQuFF1so1f5D+Amm6G29iNPdq +g4A== X-Gm-Gg: ASbGnctbzoXHHPUInHj2um2vQsoyHxYSknpPqUBQrPuM7w7Y7HUr08QqKoGxfPugLAZ fjdx5EN3Nj0hgeBbgu6ZV/gn59mjTN1WFmIUJHXX0JwTSHWW33cJ5CjYdGlsp8mnQLHJjidfcnI 9hPlmYg6mhbt3IUH0oNuIiYrZJRyPjPAQqTHPtuQOVaW8QvdWa9i96WuS1/SvIlvOvICGOp9R6m J3uLJWAH0thafp23AKKhXLclUfRLl/lWLvYd/Q5XpPf8WEeHHPQDpeaW2vg1oqDd2vHMApCX/Km xyUCN9+qhFRy3Bx4QAMh1vwgqfge6EdsODtsC8pjGyhUYBNVW7nq X-Received: by 2002:a05:6830:264e:b0:71d:5983:8827 with SMTP id 46e09a7af769-71dad6d3bb3mr10216250a34.19.1733383055608; Wed, 04 Dec 2024 23:17:35 -0800 (PST) X-Google-Smtp-Source: AGHT+IFCd0BmpC+PZMTubC54bMDVhvpw3j3A3q/DZBKccq+qL7noJolQS2hLkQ9G0hYcYL2yXgJzxw== X-Received: by 2002:a05:6830:264e:b0:71d:5983:8827 with SMTP id 46e09a7af769-71dad6d3bb3mr10216240a34.19.1733383055300; Wed, 04 Dec 2024 23:17:35 -0800 (PST) Received: from rickywu0421-ThinkPad-X1-Carbon-Gen-11.. (118-163-61-247.hinet-ip.hinet.net. [118.163.61.247]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-7fd156fb8fcsm573909a12.40.2024.12.04.23.17.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Dec 2024 23:17:34 -0800 (PST) From: En-Wei Wu To: marcel@holtmann.org, luiz.dentz@gmail.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org, pmenzel@molgen.mpg.de Cc: quic_tjiang@quicinc.com, kuan-ying.lee@canonical.com, anthony.wong@canonical.com Subject: [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() Date: Thu, 5 Dec 2024 15:17:26 +0800 Message-ID: <20241205071727.36710-2-en-wei.wu@canonical.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20241205071727.36710-1-en-wei.wu@canonical.com> References: <20241205071727.36710-1-en-wei.wu@canonical.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" The WCN7851 (0489:e0f3) Bluetooth controller supports firmware crash dump collection through devcoredump. During this process, the crash dump data is queued to a dump queue as skb for further processing. A NULL pointer dereference occurs in skb_dequeue() when processing the dump queue due to improper return value handling: [ 93.672166] Bluetooth: hci0: ACL memdump size(589824) [ 93.672475] BUG: kernel NULL pointer dereference, address: 0000000000000008 [ 93.672517] Workqueue: hci0 hci_devcd_rx [bluetooth] [ 93.672598] RIP: 0010:skb_dequeue+0x50/0x80 The issue stems from handle_dump_pkt_qca() returning the wrong value on success. It currently returns the value from hci_devcd_init()=20 (0 on success), but callers expect > 0 to indicate successful=20 dump handling. This causes hci_recv_frame() to free the skb while=20 it's still queued for dump processing, leading to the NULL pointer=20 dereference when hci_devcd_rx() tries to dequeue it. Fix this by: 1. Making handle_dump_pkt_qca() return 0 on success and negative errno on failure, consistent with other kernel interfaces 2. Splitting dump packet detection into separate ACL and event functions for better code organization and maintainability This prevents premature skb freeing by ensuring proper handling=20 of dump packets. Fixes: 20981ce2d5a5 ("Bluetooth: btusb: Add WCN6855 devcoredump support") Signed-off-by: En-Wei Wu --- drivers/bluetooth/btusb.c | 97 ++++++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 28 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 279fe6c115fa..2bfb915062cf 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2930,22 +2930,16 @@ static void btusb_coredump_qca(struct hci_dev *hdev) bt_dev_err(hdev, "%s: triggle crash failed (%d)", __func__, err); } =20 -/* - * =3D=3D0: not a dump pkt. - * < 0: fails to handle a dump pkt - * > 0: otherwise. - */ +/* Return: 0 on success, negative errno on failure. */ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb) { - int ret =3D 1; + int ret =3D 0; u8 pkt_type; u8 *sk_ptr; unsigned int sk_len; u16 seqno; u32 dump_size; =20 - struct hci_event_hdr *event_hdr; - struct hci_acl_hdr *acl_hdr; struct qca_dump_hdr *dump_hdr; struct btusb_data *btdata =3D hci_get_drvdata(hdev); struct usb_device *udev =3D btdata->udev; @@ -2955,30 +2949,14 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev= , struct sk_buff *skb) sk_len =3D skb->len; =20 if (pkt_type =3D=3D HCI_ACLDATA_PKT) { - acl_hdr =3D hci_acl_hdr(skb); - if (le16_to_cpu(acl_hdr->handle) !=3D QCA_MEMDUMP_ACL_HANDLE) - return 0; sk_ptr +=3D HCI_ACL_HDR_SIZE; sk_len -=3D HCI_ACL_HDR_SIZE; - event_hdr =3D (struct hci_event_hdr *)sk_ptr; - } else { - event_hdr =3D hci_event_hdr(skb); } =20 - if ((event_hdr->evt !=3D HCI_VENDOR_PKT) - || (event_hdr->plen !=3D (sk_len - HCI_EVENT_HDR_SIZE))) - return 0; - sk_ptr +=3D HCI_EVENT_HDR_SIZE; sk_len -=3D HCI_EVENT_HDR_SIZE; =20 dump_hdr =3D (struct qca_dump_hdr *)sk_ptr; - if ((sk_len < offsetof(struct qca_dump_hdr, data)) - || (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) - || (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) - return 0; - - /*it is dump pkt now*/ seqno =3D le16_to_cpu(dump_hdr->seqno); if (seqno =3D=3D 0) { set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags); @@ -3052,17 +3030,80 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev= , struct sk_buff *skb) return ret; } =20 +/* Return: true if the ACL packet is a dump packet, false otherwise. */ +static bool btqca_acl_pkt_is_dump(struct hci_dev *hdev, struct sk_buff *sk= b) +{ + struct hci_event_hdr *event_hdr; + struct hci_acl_hdr *acl_hdr; + struct qca_dump_hdr *dump_hdr; + + sk_ptr =3D skb->data; + sk_len =3D skb->len; + + acl_hdr =3D hci_acl_hdr(skb); + if (le16_to_cpu(acl_hdr->handle) !=3D QCA_MEMDUMP_ACL_HANDLE) + return false; + sk_ptr +=3D HCI_ACL_HDR_SIZE; + sk_len -=3D HCI_ACL_HDR_SIZE; + event_hdr =3D (struct hci_event_hdr *)sk_ptr; + + if ((event_hdr->evt !=3D HCI_VENDOR_PKT) + || (event_hdr->plen !=3D (sk_len - HCI_EVENT_HDR_SIZE))) + return false; + + sk_ptr +=3D HCI_EVENT_HDR_SIZE; + sk_len -=3D HCI_EVENT_HDR_SIZE; + + dump_hdr =3D (struct qca_dump_hdr *)sk_ptr; + if ((sk_len < offsetof(struct qca_dump_hdr, data)) + || (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) + || (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) + return false; + + return true; +} + +/* Return: true if the event packet is a dump packet, false otherwise. */ +static bool btqca_event_pkt_is_dump(struct hci_dev *hdev, struct sk_buff *= skb) +{ + u8 *sk_ptr; + unsigned int sk_len; + + struct hci_event_hdr *event_hdr; + struct qca_dump_hdr *dump_hdr; + + sk_ptr =3D skb->data; + sk_len =3D skb->len; + + event_hdr =3D hci_event_hdr(skb); + + if ((event_hdr->evt !=3D HCI_VENDOR_PKT) + || (event_hdr->plen !=3D (sk_len - HCI_EVENT_HDR_SIZE))) + return false; + + sk_ptr +=3D HCI_EVENT_HDR_SIZE; + sk_len -=3D HCI_EVENT_HDR_SIZE; + + dump_hdr =3D (struct qca_dump_hdr *)sk_ptr; + if ((sk_len < offsetof(struct qca_dump_hdr, data)) + || (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) + || (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) + return false; + + return true; +} + static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb) { - if (handle_dump_pkt_qca(hdev, skb)) - return 0; + if (btqca_acl_pkt_is_dump(hdev, skb)) + return handle_dump_pkt_qca(hdev, skb); return hci_recv_frame(hdev, skb); } =20 static int btusb_recv_evt_qca(struct hci_dev *hdev, struct sk_buff *skb) { - if (handle_dump_pkt_qca(hdev, skb)) - return 0; + if (btqca_event_pkt_is_dump(hdev, skb)) + return handle_dump_pkt_qca(hdev, skb); return hci_recv_frame(hdev, skb); } =20 --=20 2.43.0 From nobody Thu Dec 18 07:52:47 2025 Received: from smtp-relay-internal-0.canonical.com (smtp-relay-internal-0.canonical.com [185.125.188.122]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 566611B0F33 for ; Thu, 5 Dec 2024 07:17:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.125.188.122 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733383068; cv=none; b=q+4iKilFEXhUCKEl6a+5VW5kQJTmVSmEa6uAHhY+q4HEHZFg5me3BTq7y4MkIUkZ4MuWPeWrpPGBHcBm/b+Zi9D9g7VSeByQQ8G+rAnzzdB7m+CVaHgclv1xcV1axrUuJgE3kAX8/8l/OrcKJZZ4OLurE2BxRpAPPD3Wox28PHg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733383068; c=relaxed/simple; bh=DUk6KztYOuWCHkF2w3Ail7VTXEtKfUcqVezNhB/cCVs=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=G7KiFhSioKN8aD/avfD3d5sqVhURLIPzMKPqCZOUD+M7nEA0+4YKOouLjIpLEaypXjxnqEnbbwsHP+PpcSQ6DawaWTDrIHlIAu0qSHpmxbO+kxqvRfnTxKSu8JFKcQvvccX0dByUdjFZenxTrDf+9NCe7UGdxrTo5DfTPoX8aAE= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=canonical.com; spf=pass smtp.mailfrom=canonical.com; dkim=pass (2048-bit key) header.d=canonical.com header.i=@canonical.com header.b=Idxzssr0; arc=none smtp.client-ip=185.125.188.122 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=canonical.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=canonical.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=canonical.com header.i=@canonical.com header.b="Idxzssr0" Received: from mail-pg1-f197.google.com (mail-pg1-f197.google.com [209.85.215.197]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 4CD043F851 for ; Thu, 5 Dec 2024 07:17:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1733383059; bh=veChOtHywlqJWRuy07dGfiMiZy6FsscGgp0bVfweBmw=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=Idxzssr0tP3giLCBg5x6H9NiXbBHDHWrSpiquSth371BqdtF5u98js4pPYxLp/+0f Do+xTUMpzD6sF8KlETTV49YYptgR2F5RU2fGZEufSHxvJNL55YU8Q0f4ik+vXQU2MX uhigUr+VY6bK5Mw2ugg8G+WntJF5/sqk8Yr30v6ZmGgEIoWOzHrHjmO5plpi1/otd2 vIaB4o4KZgZ/BQznaHCVbrIq4rpdMymFbjZjjItrzjKzSXKN6eyP3FCpBqDU0bbBal 5lS8GCLe0qUBS0sBzMLb1W4oN2DYgIucHjUDQkRL8Wqx7kRTWnYtUoY+wHxUIojds1 Fbl7Ulz6J/nsA== Received: by mail-pg1-f197.google.com with SMTP id 41be03b00d2f7-7fbd38074f4so296128a12.1 for ; Wed, 04 Dec 2024 23:17:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733383058; x=1733987858; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=veChOtHywlqJWRuy07dGfiMiZy6FsscGgp0bVfweBmw=; b=qU5VpuPmie7BK5YrmGqBe3y7NOfvNGHrWCUNrcEdmW1kHUunrhDuDoCOwLzqcO7CfF p5eiP+/+vhp93HMhrxy1zxxncserybsciIJgnpto2IfHiTQq0ngp6CREAVDcrW6sToiJ XLmWK3B16sBjFDZmFyEBHrNzprsPoWWYjisQ97HVIVPWIPrRusCmTLy9/XQfwfQ1mpb/ OYGrO2e0eWPg0mZHIaK08jdbkKUNDJHrI3du0f+P1K0HLVkrLqTorZoyR6v8wSoiBkHj A8mxoRx8HkOgGJ05nZuLqiIXzpIGrAyOEx8DRvIxqGDGtes+E4/Fmb0HiyAze7HK9OdP TpcA== X-Forwarded-Encrypted: i=1; AJvYcCUZpwNnm2/rwZIG4s94N4umryL37SXu2JAwkWADmk8XgVbxSjNxXyeChMlAtG62JmmlYO5kJR/H6q0/sKM=@vger.kernel.org X-Gm-Message-State: AOJu0YxlVzzYYUi9hdaab/pLBh4uNR8f4BgaBCXojqcKkthu+n0mRZ6/ K7RAsdcn1pTOrCkn5AXXaAfgz8t8QbNFqR+7XHFkuoCYhEeB/pyeub3rWHT3BLKdKGaeTt2dvmz 6wnevQ1BROCs9HYMCM8X9FAvUlSsfaWsTm7i41nLjUZmTPzEZ5tzEY+lRd2CVxJSup6V83G4jEk /2GA== X-Gm-Gg: ASbGncsySphEYt4InZpF+G4IcAh3JB6v9/5PADO98YwJqtz2BJ1BojJredHMZm/HWsk 9U7LmnWcvuv1uqzXf9FOh2kfb4pebXB/YINGLnEXRdxWNEHc68jAawuVhBzYEvAOGYVKolUhqPW wmadLr91lmMZg/SgT9apXd6QVZDhG4IvvZ91lN3/6jlcil47974MhO/7s/HVVjSTZMrIBg3h9ww hrU+48Y1Aicy6/JSZcxBsJY2MXutm6fBa6aaRiqh1ZNBE/xhijiccYO+Pm7mQaP7p3++amJmaa8 NI9EuOiTs7+tnE7lnJk5aXx2/r+5uWyZQNY8GPrIWmQU6L3f1cZj X-Received: by 2002:a05:6a21:330b:b0:1e0:d837:c929 with SMTP id adf61e73a8af0-1e16bdd32cemr11192494637.9.1733383057939; Wed, 04 Dec 2024 23:17:37 -0800 (PST) X-Google-Smtp-Source: AGHT+IHgzsr7JcfsegoUdP6GY5G8iGsXuJUVNjVey+/PZYj54eOigRoJ8F45T7xC4maZMzZD7dP8lQ== X-Received: by 2002:a05:6a21:330b:b0:1e0:d837:c929 with SMTP id adf61e73a8af0-1e16bdd32cemr11192479637.9.1733383057640; Wed, 04 Dec 2024 23:17:37 -0800 (PST) Received: from rickywu0421-ThinkPad-X1-Carbon-Gen-11.. (118-163-61-247.hinet-ip.hinet.net. [118.163.61.247]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-7fd156fb8fcsm573909a12.40.2024.12.04.23.17.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Dec 2024 23:17:37 -0800 (PST) From: En-Wei Wu To: marcel@holtmann.org, luiz.dentz@gmail.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org, pmenzel@molgen.mpg.de Cc: quic_tjiang@quicinc.com, kuan-ying.lee@canonical.com, anthony.wong@canonical.com Subject: [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling Date: Thu, 5 Dec 2024 15:17:27 +0800 Message-ID: <20241205071727.36710-3-en-wei.wu@canonical.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20241205071727.36710-1-en-wei.wu@canonical.com> References: <20241205071727.36710-1-en-wei.wu@canonical.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Replace direct buffer access and manual pointer arithmetic in QCA dump packet handling with safer skb_pull_data() calls. This ensures proper bounds checking when accessing packet headers and adds proper restoration of SKB data pointer and length on error paths. The changes include: - Replacing manual pointer arithmetic with skb_pull_data() for=20 safer packet header access - Adding SKB state restoration in error paths This prevents potential buffer overflows and ensures SKB state remains consistent even when packet validation fails. Signed-off-by: En-Wei Wu --- drivers/bluetooth/btusb.c | 95 +++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 54 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 2bfb915062cf..cbeb1cec790a 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2935,8 +2935,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, = struct sk_buff *skb) { int ret =3D 0; u8 pkt_type; - u8 *sk_ptr; - unsigned int sk_len; u16 seqno; u32 dump_size; =20 @@ -2945,18 +2943,8 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev,= struct sk_buff *skb) struct usb_device *udev =3D btdata->udev; =20 pkt_type =3D hci_skb_pkt_type(skb); - sk_ptr =3D skb->data; - sk_len =3D skb->len; + dump_hdr =3D (struct qca_dump_hdr *)skb->data; =20 - if (pkt_type =3D=3D HCI_ACLDATA_PKT) { - sk_ptr +=3D HCI_ACL_HDR_SIZE; - sk_len -=3D HCI_ACL_HDR_SIZE; - } - - sk_ptr +=3D HCI_EVENT_HDR_SIZE; - sk_len -=3D HCI_EVENT_HDR_SIZE; - - dump_hdr =3D (struct qca_dump_hdr *)sk_ptr; seqno =3D le16_to_cpu(dump_hdr->seqno); if (seqno =3D=3D 0) { set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags); @@ -2976,16 +2964,15 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev= , struct sk_buff *skb) =20 btdata->qca_dump.ram_dump_size =3D dump_size; btdata->qca_dump.ram_dump_seqno =3D 0; - sk_ptr +=3D offsetof(struct qca_dump_hdr, data0); - sk_len -=3D offsetof(struct qca_dump_hdr, data0); + + skb_pull(skb, offsetof(struct qca_dump_hdr, data0)); =20 usb_disable_autosuspend(udev); bt_dev_info(hdev, "%s memdump size(%u)\n", (pkt_type =3D=3D HCI_ACLDATA_PKT) ? "ACL" : "event", dump_size); } else { - sk_ptr +=3D offsetof(struct qca_dump_hdr, data); - sk_len -=3D offsetof(struct qca_dump_hdr, data); + skb_pull(skb, offsetof(struct qca_dump_hdr, data)); } =20 if (!btdata->qca_dump.ram_dump_size) { @@ -3005,7 +2992,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, = struct sk_buff *skb) return ret; } =20 - skb_pull(skb, skb->len - sk_len); hci_devcd_append(hdev, skb); btdata->qca_dump.ram_dump_seqno++; if (seqno =3D=3D QCA_LAST_SEQUENCE_NUM) { @@ -3036,61 +3022,62 @@ static bool btqca_acl_pkt_is_dump(struct hci_dev *h= dev, struct sk_buff *skb) struct hci_event_hdr *event_hdr; struct hci_acl_hdr *acl_hdr; struct qca_dump_hdr *dump_hdr; + void *orig_data; + unsigned int orig_len; =20 - sk_ptr =3D skb->data; - sk_len =3D skb->len; + orig_data =3D skb->data; + orig_len =3D skb->len; =20 - acl_hdr =3D hci_acl_hdr(skb); - if (le16_to_cpu(acl_hdr->handle) !=3D QCA_MEMDUMP_ACL_HANDLE) - return false; - sk_ptr +=3D HCI_ACL_HDR_SIZE; - sk_len -=3D HCI_ACL_HDR_SIZE; - event_hdr =3D (struct hci_event_hdr *)sk_ptr; - - if ((event_hdr->evt !=3D HCI_VENDOR_PKT) - || (event_hdr->plen !=3D (sk_len - HCI_EVENT_HDR_SIZE))) - return false; + acl_hdr =3D skb_pull_data(skb, sizeof(*acl_hdr)); + if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) !=3D QCA_MEMDUMP_ACL_HANDLE= )) + goto restore_return; =20 - sk_ptr +=3D HCI_EVENT_HDR_SIZE; - sk_len -=3D HCI_EVENT_HDR_SIZE; + event_hdr =3D skb_pull_data(skb, sizeof(*event_hdr)); + if (!event_hdr || (event_hdr->evt !=3D HCI_VENDOR_PKT)) + goto restore_return; =20 - dump_hdr =3D (struct qca_dump_hdr *)sk_ptr; - if ((sk_len < offsetof(struct qca_dump_hdr, data)) - || (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) - || (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) - return false; + dump_hdr =3D (struct qca_dump_hdr *)skb->data; + if ((skb->len < sizeof(*dump_hdr)) || + (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) || + (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) + goto restore_return; =20 return true; + +restore_return: + skb->data =3D orig_data; + skb->len =3D orig_len; + return false; } =20 /* Return: true if the event packet is a dump packet, false otherwise. */ static bool btqca_event_pkt_is_dump(struct hci_dev *hdev, struct sk_buff *= skb) { - u8 *sk_ptr; - unsigned int sk_len; - struct hci_event_hdr *event_hdr; struct qca_dump_hdr *dump_hdr; + void *orig_data; + unsigned int orig_len; =20 - sk_ptr =3D skb->data; - sk_len =3D skb->len; + orig_data =3D skb->data; + orig_len =3D skb->len; =20 - event_hdr =3D hci_event_hdr(skb); + event_hdr =3D skb_pull_data(skb, sizeof(*event_hdr)); + if (!event_hdr || (event_hdr->evt !=3D HCI_VENDOR_PKT)) + goto restore_return; =20 - if ((event_hdr->evt !=3D HCI_VENDOR_PKT) - || (event_hdr->plen !=3D (sk_len - HCI_EVENT_HDR_SIZE))) - return false; + dump_hdr =3D (struct qca_dump_hdr *)skb->data; + if ((skb->len < sizeof(*dump_hdr)) || + (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) || + (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) + goto restore_return; =20 - sk_ptr +=3D HCI_EVENT_HDR_SIZE; - sk_len -=3D HCI_EVENT_HDR_SIZE; + return true; =20 - dump_hdr =3D (struct qca_dump_hdr *)sk_ptr; - if ((sk_len < offsetof(struct qca_dump_hdr, data)) - || (dump_hdr->vse_class !=3D QCA_MEMDUMP_VSE_CLASS) - || (dump_hdr->msg_type !=3D QCA_MEMDUMP_MSG_TYPE)) - return false; +restore_return: + skb->data =3D orig_data; + skb->len =3D orig_len; + return false; =20 - return true; } =20 static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb) --=20 2.43.0