[v1] Bluetooth: btnxpuart: No need to check the received bootloader signature

Commit Message

Neeraj Sanjay Kale April 3, 2023, 12:24 p.m. UTC

We can never assume the uart will deliver a complete packet to the BT
layer at once, the expected packet may be divided into several parts by
uart as uart doesn't know the received packet size, the received data
count may mismatch with the expected packet size, so here
is_valid_bootloader_signature() check may always return false.

Even we remove the count check in is_valid_bootloader_signature(), then
the first part of the data which includes the packet type can pass the
is_valid_bootloader_signature() check, but the remaining parts don't
have the packet type data still cannot pass the check, here return
directly will cause the data loss.

So need to remove the received bootloader signature check here, the
h4_recv_buf() can help us combine the different data received into one
packet. If any out-of-sync or incomplete bootloader signature is received,
it is safe to ignore and discard it, and process the next bootloader
signature.

Co-developed-by: Sherry Sun <[e-mail]>
Signed-off-by: Sherry Sun <[e-mail]>
Signed-off-by: Neeraj Sanjay Kale <[e-mail]>
---
 drivers/bluetooth/btnxpuart.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

Comments

[e-mail] April 3, 2023, 1:13 p.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=736377

---Test result---

Test Summary:
CheckPatch                    PASS      0.70 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      31.81 seconds
CheckAllWarning               PASS      34.70 seconds
CheckSparse                   PASS      39.50 seconds
CheckSmatch                   PASS      109.03 seconds
BuildKernel32                 PASS      30.76 seconds
TestRunnerSetup               PASS      441.63 seconds
TestRunner_l2cap-tester       PASS      16.19 seconds
TestRunner_iso-tester         PASS      16.28 seconds
TestRunner_bnep-tester        PASS      5.38 seconds
TestRunner_mgmt-tester        PASS      110.72 seconds
TestRunner_rfcomm-tester      PASS      8.58 seconds
TestRunner_sco-tester         PASS      7.85 seconds
TestRunner_ioctl-tester       PASS      9.13 seconds
TestRunner_mesh-tester        PASS      6.73 seconds
TestRunner_smp-tester         PASS      7.77 seconds
TestRunner_userchan-tester    PASS      5.68 seconds
IncrementalBuild              PASS      29.60 seconds



---
Regards,
Linux Bluetooth

[e-mail] April 6, 2023, 9 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Mon,  3 Apr 2023 17:54:30 +0530 you wrote:
> We can never assume the uart will deliver a complete packet to the BT
> layer at once, the expected packet may be divided into several parts by
> uart as uart doesn't know the received packet size, the received data
> count may mismatch with the expected packet size, so here
> is_valid_bootloader_signature() check may always return false.
> 
> Even we remove the count check in is_valid_bootloader_signature(), then
> the first part of the data which includes the packet type can pass the
> is_valid_bootloader_signature() check, but the remaining parts don't
> have the packet type data still cannot pass the check, here return
> directly will cause the data loss.
> 
> [...]

Here is the summary with links:
  - [v1] Bluetooth: btnxpuart: No need to check the received bootloader signature
    https://git.kernel.org/bluetooth/bluetooth-next/c/b1ff41fd0ee6

You are awesome, thank you!

Patch

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 5f641466d695..7c22c1ac087a 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1147,33 +1147,20 @@  static const struct h4_recv_pkt nxp_recv_pkts[] = {
 	{ NXP_RECV_FW_REQ_V3,   .recv = nxp_recv_fw_req_v3 },
 };
 
-static bool is_valid_bootloader_signature(const u8 *data, size_t count)
-{
-	if ((*data == NXP_V1_FW_REQ_PKT && count == sizeof(struct v1_data_req) + 1) ||
-	    (*data == NXP_V3_FW_REQ_PKT && count == sizeof(struct v3_data_req) + 1) ||
-	    (*data == NXP_V3_CHIP_VER_PKT && count == sizeof(struct v3_start_ind) + 1))
-		return true;
-	else
-		return false;
-}
-
 static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
 				 size_t count)
 {
 	struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
 
-	if (is_fw_downloading(nxpdev) && !is_valid_bootloader_signature(data, count)) {
-		/* Unknown bootloader signature, skip without returning error */
-		return count;
-	}
-
 	ps_start_timer(nxpdev);
 
 	nxpdev->rx_skb = h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count,
 				     nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts));
 	if (IS_ERR(nxpdev->rx_skb)) {
 		int err = PTR_ERR(nxpdev->rx_skb);
-
+		/* Safe to ignore out-of-sync bootloader signatures */
+		if (is_fw_downloading(nxpdev))
+			return count;
 		bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
 		nxpdev->rx_skb = NULL;
 		return err;