diff mbox series

[v1] btintel: Add recovery when secure verification of firmware fails

Message ID 20230110155905.18203-1-kiran.k@intel.com (mailing list archive)
State New, archived
Headers show
Series [v1] btintel: Add recovery when secure verification of firmware fails | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix fail "Bluetooth: " prefix is not specified in the subject
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester success TestRunner PASS
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

K, Kiran Jan. 10, 2023, 3:59 p.m. UTC 
On warm reboot stress test case, firmware download failure
has been observed with failure in secure verification of firmware
and BT becomes completely inaccessible. This can happen due to a race
condition in TOP registers when Wifi driver is also getting loaded
at the same time. This patch adds a work around to load the BT firmware
again when secure verify failure is observed.

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>
---
 drivers/bluetooth/btintel.c | 20 ++++++++++++++++----
 drivers/bluetooth/btintel.h |  1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

Comments

K, Kiran Jan. 10, 2023, 3:52 p.m. UTC | #1 
Adding An, Tedd in CC list.

> -----Original Message-----
> From: K, Kiran <kiran.k@intel.com>
> Sent: Tuesday, January 10, 2023 9:29 PM
> To: linux-bluetooth@vger.kernel.org
> Cc: Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; K, Kiran
> <kiran.k@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: [PATCH v1] btintel: Add recovery when secure verification of
> firmware fails
> 
> On warm reboot stress test case, firmware download failure has been
> observed with failure in secure verification of firmware and BT becomes
> completely inaccessible. This can happen due to a race condition in TOP
> registers when Wifi driver is also getting loaded at the same time. This patch
> adds a work around to load the BT firmware again when secure verify failure
> is observed.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan Tumkur Narayan
> <chethan.tumkur.narayan@intel.com>
> ---
>  drivers/bluetooth/btintel.c | 20 ++++++++++++++++----
> drivers/bluetooth/btintel.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index
> d4e2cb9a4eb4..3f2976fb056a 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1700,6 +1700,11 @@ static int btintel_download_wait(struct hci_dev
> *hdev, ktime_t calltime, int mse
>  		return -ETIMEDOUT;
>  	}
> 
> +	if (btintel_test_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED)) {
> +		bt_dev_err(hdev, "Firmware secure verification failed");
> +		return -EAGAIN;
> +	}
> +
>  	if (btintel_test_flag(hdev, INTEL_FIRMWARE_FAILED)) {
>  		bt_dev_err(hdev, "Firmware loading failed");
>  		return -ENOEXEC;
> @@ -1961,7 +1966,7 @@ static int btintel_download_fw(struct hci_dev
> *hdev,
>  	 * of this device.
>  	 */
>  	err = btintel_download_wait(hdev, calltime, 5000);
> -	if (err == -ETIMEDOUT)
> +	if (err == -ETIMEDOUT || err == -EAGAIN)
>  		btintel_reset_to_bootloader(hdev);
> 
>  done:
> @@ -2153,7 +2158,7 @@ static int btintel_prepare_fw_download_tlv(struct
> hci_dev *hdev,
>  	 * of this device.
>  	 */
>  	err = btintel_download_wait(hdev, calltime, 5000);
> -	if (err == -ETIMEDOUT)
> +	if (err == -ETIMEDOUT || err == -EAGAIN)
>  		btintel_reset_to_bootloader(hdev);
> 
>  done:
> @@ -2644,8 +2649,15 @@ void btintel_secure_send_result(struct hci_dev
> *hdev,
>  	if (len != sizeof(*evt))
>  		return;
> 
> -	if (evt->result)
> -		btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +	if (evt->result) {
> +		bt_dev_err(hdev, "Intel Secure Send Results event result: %u
> status: %u",
> +			   evt->result, evt->status);
> +
> +		if (evt->result == 3)
> +			btintel_set_flag(hdev,
> INTEL_FIRMWARE_VERIFY_FAILED);
> +		else
> +			btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +	}
> 
>  	if (btintel_test_and_clear_flag(hdev, INTEL_DOWNLOADING) &&
>  	    btintel_test_flag(hdev, INTEL_FIRMWARE_LOADED)) diff --git
> a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h index
> e0060e58573c..10e5be7e451a 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -147,6 +147,7 @@ enum {
>  	INTEL_BOOTLOADER,
>  	INTEL_DOWNLOADING,
>  	INTEL_FIRMWARE_LOADED,
> +	INTEL_FIRMWARE_VERIFY_FAILED,
>  	INTEL_FIRMWARE_FAILED,
>  	INTEL_BOOTING,
>  	INTEL_BROKEN_INITIAL_NCMD,
> --
> 2.17.1
bluez.test.bot@gmail.com Jan. 10, 2023, 4:39 p.m. UTC | #2 
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=710575

---Test result---

Test Summary:
CheckPatch                    PASS      0.90 seconds
GitLint                       PASS      0.35 seconds
SubjectPrefix                 FAIL      0.39 seconds
BuildKernel                   PASS      32.29 seconds
CheckAllWarning               PASS      35.00 seconds
CheckSparse                   PASS      39.88 seconds
CheckSmatch                   PASS      107.37 seconds
BuildKernel32                 PASS      30.46 seconds
TestRunnerSetup               PASS      441.80 seconds
TestRunner_l2cap-tester       PASS      16.27 seconds
TestRunner_iso-tester         PASS      17.18 seconds
TestRunner_bnep-tester        PASS      5.80 seconds
TestRunner_mgmt-tester        PASS      110.21 seconds
TestRunner_rfcomm-tester      PASS      9.07 seconds
TestRunner_sco-tester         PASS      8.33 seconds
TestRunner_ioctl-tester       PASS      9.87 seconds
TestRunner_mesh-tester        PASS      7.24 seconds
TestRunner_smp-tester         PASS      8.26 seconds
TestRunner_userchan-tester    PASS      6.08 seconds
IncrementalBuild              PASS      28.91 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject


---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Jan. 10, 2023, 8:10 p.m. UTC | #3 
Hi Kiran,

On Tue, Jan 10, 2023 at 7:52 AM Kiran K <kiran.k@intel.com> wrote:
>
> On warm reboot stress test case, firmware download failure
> has been observed with failure in secure verification of firmware
> and BT becomes completely inaccessible. This can happen due to a race
> condition in TOP registers when Wifi driver is also getting loaded
> at the same time. This patch adds a work around to load the BT firmware
> again when secure verify failure is observed.

In other words we can't trust the controller will be able to verify
the firmware?

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>
> ---
>  drivers/bluetooth/btintel.c | 20 ++++++++++++++++----
>  drivers/bluetooth/btintel.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index d4e2cb9a4eb4..3f2976fb056a 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1700,6 +1700,11 @@ static int btintel_download_wait(struct hci_dev *hdev, ktime_t calltime, int mse
>                 return -ETIMEDOUT;
>         }
>
> +       if (btintel_test_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED)) {
> +               bt_dev_err(hdev, "Firmware secure verification failed");
> +               return -EAGAIN;
> +       }
> +
>         if (btintel_test_flag(hdev, INTEL_FIRMWARE_FAILED)) {
>                 bt_dev_err(hdev, "Firmware loading failed");
>                 return -ENOEXEC;
> @@ -1961,7 +1966,7 @@ static int btintel_download_fw(struct hci_dev *hdev,
>          * of this device.
>          */
>         err = btintel_download_wait(hdev, calltime, 5000);
> -       if (err == -ETIMEDOUT)
> +       if (err == -ETIMEDOUT || err == -EAGAIN)
>                 btintel_reset_to_bootloader(hdev);
>
>  done:
> @@ -2153,7 +2158,7 @@ static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
>          * of this device.
>          */
>         err = btintel_download_wait(hdev, calltime, 5000);
> -       if (err == -ETIMEDOUT)
> +       if (err == -ETIMEDOUT || err == -EAGAIN)
>                 btintel_reset_to_bootloader(hdev);
>
>  done:
> @@ -2644,8 +2649,15 @@ void btintel_secure_send_result(struct hci_dev *hdev,
>         if (len != sizeof(*evt))
>                 return;
>
> -       if (evt->result)
> -               btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +       if (evt->result) {
> +               bt_dev_err(hdev, "Intel Secure Send Results event result: %u status: %u",
> +                          evt->result, evt->status);
> +
> +               if (evt->result == 3)
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED);

The result 3 is what exactly? If it returns the same value for both
the race condition and when the firmware is actually invalid that
means the later will cause a look since we don't have any counter of
how many times we would be attempting to reload the firmware, so we
better limit the number of times we attempt to reload e.g. 1-2 times
at most, or we have the firmware provide a different result when it
busy loading the wifi side.

> +               else
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +       }
>
>         if (btintel_test_and_clear_flag(hdev, INTEL_DOWNLOADING) &&
>             btintel_test_flag(hdev, INTEL_FIRMWARE_LOADED))
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index e0060e58573c..10e5be7e451a 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -147,6 +147,7 @@ enum {
>         INTEL_BOOTLOADER,
>         INTEL_DOWNLOADING,
>         INTEL_FIRMWARE_LOADED,
> +       INTEL_FIRMWARE_VERIFY_FAILED,
>         INTEL_FIRMWARE_FAILED,
>         INTEL_BOOTING,
>         INTEL_BROKEN_INITIAL_NCMD,
> --
> 2.17.1
>
Tedd Ho-Jeong An Jan. 10, 2023, 9:57 p.m. UTC | #4 
Hi Kiran

On Tue, 2023-01-10 at 21:29 +0530, Kiran K wrote:
> On warm reboot stress test case, firmware download failure
> has been observed with failure in secure verification of firmware
> and BT becomes completely inaccessible. This can happen due to a race
> condition in TOP registers when Wifi driver is also getting loaded
> at the same time. This patch adds a work around to load the BT firmware
> again when secure verify failure is observed.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>
> ---
>  drivers/bluetooth/btintel.c | 20 ++++++++++++++++----
>  drivers/bluetooth/btintel.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index d4e2cb9a4eb4..3f2976fb056a 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1700,6 +1700,11 @@ static int btintel_download_wait(struct hci_dev *hdev, ktime_t calltime, int mse
>                 return -ETIMEDOUT;
>         }
>  
> +       if (btintel_test_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED)) {
> +               bt_dev_err(hdev, "Firmware secure verification failed");
> +               return -EAGAIN;
> +       }
> +
>         if (btintel_test_flag(hdev, INTEL_FIRMWARE_FAILED)) {
>                 bt_dev_err(hdev, "Firmware loading failed");
>                 return -ENOEXEC;
> @@ -1961,7 +1966,7 @@ static int btintel_download_fw(struct hci_dev *hdev,
>          * of this device.
>          */
>         err = btintel_download_wait(hdev, calltime, 5000);
> -       if (err == -ETIMEDOUT)
> +       if (err == -ETIMEDOUT || err == -EAGAIN)
>                 btintel_reset_to_bootloader(hdev);
>  
>  done:
> @@ -2153,7 +2158,7 @@ static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
>          * of this device.
>          */
>         err = btintel_download_wait(hdev, calltime, 5000);
> -       if (err == -ETIMEDOUT)
> +       if (err == -ETIMEDOUT || err == -EAGAIN)
>                 btintel_reset_to_bootloader(hdev);
>  
>  done:
> @@ -2644,8 +2649,15 @@ void btintel_secure_send_result(struct hci_dev *hdev,
>         if (len != sizeof(*evt))
>                 return;
>  
> -       if (evt->result)
> -               btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +       if (evt->result) {
> +               bt_dev_err(hdev, "Intel Secure Send Results event result: %u status: %u",
> +                          evt->result, evt->status);
> +
> +               if (evt->result == 3)
Please avoid any magic number here. You can define it in btintel.h.
As Luiz suggested, let's try to add the reloading counter in btintel_data to limit the reloading try.
 
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED);
> +               else
> +                       btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
> +       }
>  
>         if (btintel_test_and_clear_flag(hdev, INTEL_DOWNLOADING) &&
>             btintel_test_flag(hdev, INTEL_FIRMWARE_LOADED))
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index e0060e58573c..10e5be7e451a 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -147,6 +147,7 @@ enum {
>         INTEL_BOOTLOADER,
>         INTEL_DOWNLOADING,
>         INTEL_FIRMWARE_LOADED,
> +       INTEL_FIRMWARE_VERIFY_FAILED,
>         INTEL_FIRMWARE_FAILED,
>         INTEL_BOOTING,
>         INTEL_BROKEN_INITIAL_NCMD,

Regards,
Tedd
diff mbox series

Patch

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index d4e2cb9a4eb4..3f2976fb056a 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -1700,6 +1700,11 @@  static int btintel_download_wait(struct hci_dev *hdev, ktime_t calltime, int mse
 		return -ETIMEDOUT;
 	}
 
+	if (btintel_test_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED)) {
+		bt_dev_err(hdev, "Firmware secure verification failed");
+		return -EAGAIN;
+	}
+
 	if (btintel_test_flag(hdev, INTEL_FIRMWARE_FAILED)) {
 		bt_dev_err(hdev, "Firmware loading failed");
 		return -ENOEXEC;
@@ -1961,7 +1966,7 @@  static int btintel_download_fw(struct hci_dev *hdev,
 	 * of this device.
 	 */
 	err = btintel_download_wait(hdev, calltime, 5000);
-	if (err == -ETIMEDOUT)
+	if (err == -ETIMEDOUT || err == -EAGAIN)
 		btintel_reset_to_bootloader(hdev);
 
 done:
@@ -2153,7 +2158,7 @@  static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
 	 * of this device.
 	 */
 	err = btintel_download_wait(hdev, calltime, 5000);
-	if (err == -ETIMEDOUT)
+	if (err == -ETIMEDOUT || err == -EAGAIN)
 		btintel_reset_to_bootloader(hdev);
 
 done:
@@ -2644,8 +2649,15 @@  void btintel_secure_send_result(struct hci_dev *hdev,
 	if (len != sizeof(*evt))
 		return;
 
-	if (evt->result)
-		btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
+	if (evt->result) {
+		bt_dev_err(hdev, "Intel Secure Send Results event result: %u status: %u",
+			   evt->result, evt->status);
+
+		if (evt->result == 3)
+			btintel_set_flag(hdev, INTEL_FIRMWARE_VERIFY_FAILED);
+		else
+			btintel_set_flag(hdev, INTEL_FIRMWARE_FAILED);
+	}
 
 	if (btintel_test_and_clear_flag(hdev, INTEL_DOWNLOADING) &&
 	    btintel_test_flag(hdev, INTEL_FIRMWARE_LOADED))
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index e0060e58573c..10e5be7e451a 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -147,6 +147,7 @@  enum {
 	INTEL_BOOTLOADER,
 	INTEL_DOWNLOADING,
 	INTEL_FIRMWARE_LOADED,
+	INTEL_FIRMWARE_VERIFY_FAILED,
 	INTEL_FIRMWARE_FAILED,
 	INTEL_BOOTING,
 	INTEL_BROKEN_INITIAL_NCMD,