From patchwork Thu Dec 5 07:17:26 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: En-Wei Wu X-Patchwork-Id: 13894767 Received: from smtp-relay-internal-1.canonical.com (smtp-relay-internal-1.canonical.com [185.125.188.123]) (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 3C1871B218C for ; Thu, 5 Dec 2024 07:17:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.125.188.123 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733383069; cv=none; b=qjeJvpUTXBi0qNOX8txD0W6fN1+V+9VB5t9ro5Az+W8Ft/bNMQ61bOjzaw8ia5rSoawD6/RnNeaTcb8aoaYaBzuFEWchDA55vqboji99H8aRE6sQfwt8uAkUWi/Gsgj2XCuOfZUrhT6rN1f4J+EPxcvfKkzL4HdJbsQEk/2lnmM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733383069; c=relaxed/simple; bh=tCAmYwcPUUuakZw/lHI+qHX63POcQUnz7J3kWSWQjRs=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=Wz48JXxmn3ZWBQsyQLyuCbyuAmwBlLo7/hNQ8AViYXg85j14647NmVLnBJH17bon3/+R1ZoZskZ5Uy/wIigL4guGg1eS7IUtZsc3wrT7Ec/uuDHxSGTOilQe8qlG/L9X4jBeFUlx3cLQgVAXrnwE23CZW2PJCBai041rVdW78Nk= 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.123 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-ot1-f70.google.com (mail-ot1-f70.google.com [209.85.210.70]) (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-1.canonical.com (Postfix) with ESMTPS id 601793F766 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-ot1-f70.google.com with SMTP id 46e09a7af769-71d46c2ead8so523831a34.3 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=LLusH/MW/gXKEWRH7jdDgYy0oeagW2aQiBa9RW+n4ZwiySr5qBL4j8kLdDI6bD0EP7 HrBlMkZ962wdNGC04XKrDfaR0klMPBkDf2xl+6Y7qFUVIkj67APplJZSOFS3MiUI8G+D Vk+JEaXzD0TTwSxPJWHffXB47IF5jvcmn+JHRqQ4EbdfqUyab5E0O65RN4ODzCgOAI6y OGsZ7GsrqHZC7tN5XVrWXWmhzXTQoXDmLcdxiYtes1PoBHdeI18UEuYPdPY+jpp+wXjm cowQyALP9l2NLnelrXptYpjTnSO+//+f01vZ63dyzHG5HIo/czU+LfItdVmz0x3yBS6p hy9Q== X-Forwarded-Encrypted: i=1; AJvYcCU3iXa7orUSGuICjqdfSLmKubDDCCouB3cAEpI/5t4zxuj6ibRtkMfsaAXbsR4soMiXDOSyEqoMsBZ+IQmi9uU=@vger.kernel.org X-Gm-Message-State: AOJu0YxDOgjVtWBXDsPzbQI0GJfwNbR6QIgS/FuOcIdO4t6MdTuZSzL5 BgFCJzVABbo3yrvSQUTOMJt7YoXdGvgcbBpzVOl8oLKDhfWOz35O3eShQhAyaKWVDzZbS/QtVuo wZJkQYQj7oP+YRezCkZIF3p1SvifJStCCbZWjWDjXX3OQi5dIdSuf5NhOtCvjwN5PalAs29/XZN m31hurgHiC4G72NA== X-Gm-Gg: ASbGncu7ODRQbMNotsV5putdcqo9CaCHNTE5SK3YVBd10eHKtxJ+4+3/vDfEFoQmYNr p6oS8Wo6iv+WtQvU9NdOp+vl2NfB0Y9qCLXOJ0CTBEsEbkhhmUETJGGx6nJWGRqez8ZRx3mWwJc TVkvxVIoykfhNEIkv5wPNGZhHkiPmdOJjgd6tCaWsjLuWnKWfrnGaFCOXeanyvekMGDzyDKphXH NCpCGxTP+1VFcLZT6L2Gli0EHxFOR4PEKPwyuOTyWmYYnFVu6wR30sAijJerQQ0vfzpyUVLpzVZ wJuU6kkPWn8oMSxuByla4nZKadzx4qR0hNg6dlW4Yc59G2l4/Lad X-Received: by 2002:a05:6830:264e:b0:71d:5983:8827 with SMTP id 46e09a7af769-71dad6d3bb3mr10216251a34.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-bluetooth@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 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() (0 on success), but callers expect > 0 to indicate successful dump handling. This causes hci_recv_frame() to free the skb while it's still queued for dump processing, leading to the NULL pointer 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 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); } -/* - * ==0: 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 = 1; + int ret = 0; u8 pkt_type; u8 *sk_ptr; unsigned int sk_len; u16 seqno; u32 dump_size; - struct hci_event_hdr *event_hdr; - struct hci_acl_hdr *acl_hdr; struct qca_dump_hdr *dump_hdr; struct btusb_data *btdata = hci_get_drvdata(hdev); struct usb_device *udev = btdata->udev; @@ -2955,30 +2949,14 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb) sk_len = skb->len; if (pkt_type == HCI_ACLDATA_PKT) { - acl_hdr = hci_acl_hdr(skb); - if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE) - return 0; sk_ptr += HCI_ACL_HDR_SIZE; sk_len -= HCI_ACL_HDR_SIZE; - event_hdr = (struct hci_event_hdr *)sk_ptr; - } else { - event_hdr = hci_event_hdr(skb); } - if ((event_hdr->evt != HCI_VENDOR_PKT) - || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE))) - return 0; - sk_ptr += HCI_EVENT_HDR_SIZE; sk_len -= HCI_EVENT_HDR_SIZE; dump_hdr = (struct qca_dump_hdr *)sk_ptr; - if ((sk_len < offsetof(struct qca_dump_hdr, data)) - || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) - || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE)) - return 0; - - /*it is dump pkt now*/ seqno = le16_to_cpu(dump_hdr->seqno); if (seqno == 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; } +/* 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 *skb) +{ + struct hci_event_hdr *event_hdr; + struct hci_acl_hdr *acl_hdr; + struct qca_dump_hdr *dump_hdr; + + sk_ptr = skb->data; + sk_len = skb->len; + + acl_hdr = hci_acl_hdr(skb); + if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE) + return false; + sk_ptr += HCI_ACL_HDR_SIZE; + sk_len -= HCI_ACL_HDR_SIZE; + event_hdr = (struct hci_event_hdr *)sk_ptr; + + if ((event_hdr->evt != HCI_VENDOR_PKT) + || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE))) + return false; + + sk_ptr += HCI_EVENT_HDR_SIZE; + sk_len -= HCI_EVENT_HDR_SIZE; + + dump_hdr = (struct qca_dump_hdr *)sk_ptr; + if ((sk_len < offsetof(struct qca_dump_hdr, data)) + || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) + || (dump_hdr->msg_type != 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 = skb->data; + sk_len = skb->len; + + event_hdr = hci_event_hdr(skb); + + if ((event_hdr->evt != HCI_VENDOR_PKT) + || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE))) + return false; + + sk_ptr += HCI_EVENT_HDR_SIZE; + sk_len -= HCI_EVENT_HDR_SIZE; + + dump_hdr = (struct qca_dump_hdr *)sk_ptr; + if ((sk_len < offsetof(struct qca_dump_hdr, data)) + || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) + || (dump_hdr->msg_type != 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); } 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); } From patchwork Thu Dec 5 07:17:27 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: En-Wei Wu X-Patchwork-Id: 13894766 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 2974E1B0F29 for ; Thu, 5 Dec 2024 07:17:45 +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-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) (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 5312940C4B 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-f198.google.com with SMTP id 41be03b00d2f7-7fbd38074f4so296129a12.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=UlNgh85l01IGuWRXvKYvmVRB760gbbaT+bH9V3ZNESNvSfL72XYOBXVFhx+VmO3Uj7 F7ZkATGHqdOCaSCWFC9c9dV8gqcFbHUUiozQi+Cf7QcDBQqiGR0epaVVUq9ha6/gbtGd R48YRsZmScwnJ0WCTDLK6ytkaV31IkxUod4Gv2aad6PFAJJy5i3OIsy9evI6AmtERyA/ OGDikU27HItGBUcl+poi9bUnKsx15qBVT1bzVnBlDzMxnPKiyyS3H6dlKzqI+NETbql/ KVuNdx5TFZ2J8E/dbiWQ+/z3x5hn4yg7IBTSwHlFkTjtu9DdZ1dMxumzj7sPbI/QeNAs w+HA== X-Forwarded-Encrypted: i=1; AJvYcCWxXhyTfdavc4wnVKutDmwyt2BPYcXrJFZJARyDP/ENCL5Od8f4uG12iX95TUcpxqmaMHVRzn/R+5bHOmng/mA=@vger.kernel.org X-Gm-Message-State: AOJu0Yw69XUUSP9/6cqdxNV7csC9vj0e2WmLLjX+xUSvxktZFu2WqRP/ Ek74anzIui/qXmoK4Jl9I19y4gIiWgEk2ECIERDdNz5QaEuDJwDvZdCtK4DdKPJys+LU7Dz4o44 SbClVCVxgO/RwLeF61I0ZAXX5I+r7065YY//gdfrFuIqsqHMwIiwqCtPd3NaJPw/QYfSqFja2ij 1rUgLK3Q== X-Gm-Gg: ASbGncsHH8wXauRmUA7HlcmUENvqFghevMHnniYKQCFX1iSt8MVFIOVZViIM35BddUF rhWQM/RZI2/2Mpx9HK32zatEpsYa0IeX93TbcFaebiTgVKsMvYczmZA5DcGQ9ByApfNtDuPsxic fJ9+5mRPdNg1GFOeNVUjxhUWDrRrgpZXNd8SaX5xMlqYI0IzvXaQD2hAIO4B+LFUeuCgIbUq7Cm JiJLyMnneSsfNah71s2ptl53a+SK4SyyiWgbUrPqDIl/q9gdoyFIz0bLwqJfXOITvsfZSieNNDA mBiUg0kZ+zgQJcf03ZKKp1OyC/DD26sMJmJG9TaB8U72mfzrS3xi X-Received: by 2002:a05:6a21:330b:b0:1e0:d837:c929 with SMTP id adf61e73a8af0-1e16bdd32cemr11192499637.9.1733383057942; 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-bluetooth@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 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 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 = 0; u8 pkt_type; - u8 *sk_ptr; - unsigned int sk_len; u16 seqno; u32 dump_size; @@ -2945,18 +2943,8 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb) struct usb_device *udev = btdata->udev; pkt_type = hci_skb_pkt_type(skb); - sk_ptr = skb->data; - sk_len = skb->len; + dump_hdr = (struct qca_dump_hdr *)skb->data; - if (pkt_type == HCI_ACLDATA_PKT) { - sk_ptr += HCI_ACL_HDR_SIZE; - sk_len -= HCI_ACL_HDR_SIZE; - } - - sk_ptr += HCI_EVENT_HDR_SIZE; - sk_len -= HCI_EVENT_HDR_SIZE; - - dump_hdr = (struct qca_dump_hdr *)sk_ptr; seqno = le16_to_cpu(dump_hdr->seqno); if (seqno == 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) btdata->qca_dump.ram_dump_size = dump_size; btdata->qca_dump.ram_dump_seqno = 0; - sk_ptr += offsetof(struct qca_dump_hdr, data0); - sk_len -= offsetof(struct qca_dump_hdr, data0); + + skb_pull(skb, offsetof(struct qca_dump_hdr, data0)); usb_disable_autosuspend(udev); bt_dev_info(hdev, "%s memdump size(%u)\n", (pkt_type == HCI_ACLDATA_PKT) ? "ACL" : "event", dump_size); } else { - sk_ptr += offsetof(struct qca_dump_hdr, data); - sk_len -= offsetof(struct qca_dump_hdr, data); + skb_pull(skb, offsetof(struct qca_dump_hdr, data)); } 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; } - skb_pull(skb, skb->len - sk_len); hci_devcd_append(hdev, skb); btdata->qca_dump.ram_dump_seqno++; if (seqno == QCA_LAST_SEQUENCE_NUM) { @@ -3036,61 +3022,62 @@ static bool btqca_acl_pkt_is_dump(struct hci_dev *hdev, 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; - sk_ptr = skb->data; - sk_len = skb->len; + orig_data = skb->data; + orig_len = skb->len; - acl_hdr = hci_acl_hdr(skb); - if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE) - return false; - sk_ptr += HCI_ACL_HDR_SIZE; - sk_len -= HCI_ACL_HDR_SIZE; - event_hdr = (struct hci_event_hdr *)sk_ptr; - - if ((event_hdr->evt != HCI_VENDOR_PKT) - || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE))) - return false; + acl_hdr = skb_pull_data(skb, sizeof(*acl_hdr)); + if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE)) + goto restore_return; - sk_ptr += HCI_EVENT_HDR_SIZE; - sk_len -= HCI_EVENT_HDR_SIZE; + event_hdr = skb_pull_data(skb, sizeof(*event_hdr)); + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT)) + goto restore_return; - dump_hdr = (struct qca_dump_hdr *)sk_ptr; - if ((sk_len < offsetof(struct qca_dump_hdr, data)) - || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) - || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE)) - return false; + dump_hdr = (struct qca_dump_hdr *)skb->data; + if ((skb->len < sizeof(*dump_hdr)) || + (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) || + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE)) + goto restore_return; return true; + +restore_return: + skb->data = orig_data; + skb->len = orig_len; + return false; } /* 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; - sk_ptr = skb->data; - sk_len = skb->len; + orig_data = skb->data; + orig_len = skb->len; - event_hdr = hci_event_hdr(skb); + event_hdr = skb_pull_data(skb, sizeof(*event_hdr)); + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT)) + goto restore_return; - if ((event_hdr->evt != HCI_VENDOR_PKT) - || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE))) - return false; + dump_hdr = (struct qca_dump_hdr *)skb->data; + if ((skb->len < sizeof(*dump_hdr)) || + (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) || + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE)) + goto restore_return; - sk_ptr += HCI_EVENT_HDR_SIZE; - sk_len -= HCI_EVENT_HDR_SIZE; + return true; - dump_hdr = (struct qca_dump_hdr *)sk_ptr; - if ((sk_len < offsetof(struct qca_dump_hdr, data)) - || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) - || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE)) - return false; +restore_return: + skb->data = orig_data; + skb->len = orig_len; + return false; - return true; } static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)