[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(-)

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

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
>

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