* RE: [PATCH v1] btintel: Add recovery when secure verification of firmware fails
2023-01-10 15:59 [PATCH v1] btintel: Add recovery when secure verification of firmware fails Kiran K
@ 2023-01-10 15:52 ` K, Kiran
2023-01-10 16:39 ` [v1] " bluez.test.bot
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: K, Kiran @ 2023-01-10 15:52 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
Cc: Srivatsa, Ravishankar, Tumkur Narayan, Chethan, An, Tedd
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
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [v1] btintel: Add recovery when secure verification of firmware fails
2023-01-10 15:59 [PATCH v1] btintel: Add recovery when secure verification of firmware fails Kiran K
2023-01-10 15:52 ` K, Kiran
@ 2023-01-10 16:39 ` bluez.test.bot
2023-01-10 20:10 ` [PATCH v1] " Luiz Augusto von Dentz
2023-01-10 21:57 ` Tedd Ho-Jeong An
3 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2023-01-10 16:39 UTC (permalink / raw)
To: linux-bluetooth, kiran.k
[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] btintel: Add recovery when secure verification of firmware fails
2023-01-10 15:59 [PATCH v1] btintel: Add recovery when secure verification of firmware fails Kiran K
2023-01-10 15:52 ` K, Kiran
2023-01-10 16:39 ` [v1] " bluez.test.bot
@ 2023-01-10 20:10 ` Luiz Augusto von Dentz
2023-01-10 21:57 ` Tedd Ho-Jeong An
3 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2023-01-10 20:10 UTC (permalink / raw)
To: Kiran K; +Cc: linux-bluetooth, ravishankar.srivatsa, Chethan Tumkur Narayan
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
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] btintel: Add recovery when secure verification of firmware fails
2023-01-10 15:59 [PATCH v1] btintel: Add recovery when secure verification of firmware fails Kiran K
` (2 preceding siblings ...)
2023-01-10 20:10 ` [PATCH v1] " Luiz Augusto von Dentz
@ 2023-01-10 21:57 ` Tedd Ho-Jeong An
3 siblings, 0 replies; 5+ messages in thread
From: Tedd Ho-Jeong An @ 2023-01-10 21:57 UTC (permalink / raw)
To: Kiran K, linux-bluetooth; +Cc: ravishankar.srivatsa, Chethan Tumkur Narayan
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
^ permalink raw reply [flat|nested] 5+ messages in thread