| Message ID | 20210210165916.2148856-2-luiz.dentz@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/6] Bluetooth: btintel: Check firmware version before download | expand |
On Wed, 2021-02-10 at 08:59 -0800, Luiz Augusto von Dentz wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > In order to allow new firmware to be loaded it first needs to check if > the firmware version on file matches the one loaded if it doesn't then > it needs to revert to boorloader mode in order to load the new firmware. > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > drivers/bluetooth/btintel.c | 22 +++++++++++ > drivers/bluetooth/btusb.c | 74 +++++++++++++++---------------------- > 2 files changed, 52 insertions(+), 44 deletions(-) > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > index 89f85d54ca64..0d0f643f972a 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -949,6 +949,17 @@ int btintel_download_firmware(struct hci_dev *hdev, > return -EALREADY; > } > > + /* The firmware variant determines if the device is in bootloader > + * mode or is running operational firmware. The value 0x06 identifies > + * the bootloader and the value 0x23 identifies the operational > + * firmware. > + * > + * If the firmware version has changed that means it needs to be reset > + * to bootloader when operational so the new firmware can be loaded. > + */ > + if (ver->fw_variant == 0x23) > + return -EINVAL; > + > err = btintel_sfi_rsa_header_secure_send(hdev, fw); > if (err) > return err; > @@ -976,6 +987,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev, > return -EALREADY; > } > > + /* The firmware variant determines if the device is in bootloader > + * mode or is running operational firmware. The value 0x03 identifies > + * the bootloader and the value 0x23 identifies the operational > + * firmware. > + * > + * If the firmware version has changed that means it needs to be reset > + * to bootloader when operational so the new firmware can be loaded. > + */ > + if (ver->img_type == 0x03) > + return -EINVAL; > + > /* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support > * only RSA secure boot engine. Hence, the corresponding sfi file will > * have RSA header of 644 bytes followed by Command Buffer. > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index c92060e7472c..a44f3cf25790 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -2469,14 +2469,30 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb) > return -EILSEQ; > } > > -static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver, > +static int btusb_setup_intel_new_get_fw_name(struct intel_version *ver, > struct intel_boot_params *params, > char *fw_name, size_t len, > const char *suffix) > { > + /* The hardware platform number has a fixed value of 0x37 and > + * for now only accept this single value. > + */ > + if (ver->hw_platform != 0x37) > + return -EINVAL; > + > switch (ver->hw_variant) { > case 0x0b: /* SfP */ > case 0x0c: /* WsP */ > + /* The firmware variant determines if the device is in > + * bootloader mode or is running operational firmware. > + * > + * Version checking cannot be performed in these models since > + * the firmware versioning depends on the firmware being in > + * bootloader mode. > + */ > + if (ver->fw_variant == 0x23) > + return -EALREADY; > + > snprintf(fw_name, len, "intel/ibt-%u-%u.%s", > le16_to_cpu(ver->hw_variant), > le16_to_cpu(params->dev_revid), > @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver, > suffix); > break; > default: > - return false; > + return -EINVAL; > } > - return true; > + > + return 0; There is one more place in btusb_setup_intel_new()@btusb.c to update the handling of return value of this funcion, which is related to loading the DDC. Code like this... if (!err) { bt_dev_err(hdev, "Unsupported Intel firmware naming"); } else { > } > > static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv, > @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev, > if (ver->img_type == 0x03) { > clear_bit(BTUSB_BOOTLOADER, &data->flags); > btintel_check_bdaddr(hdev); > - return 0; > } > > /* Check for supported iBT hardware variants of this firmware > @@ -2694,35 +2710,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > if (!ver || !params) > return -EINVAL; > > - /* The hardware platform number has a fixed value of 0x37 and > - * for now only accept this single value. > - */ > - if (ver->hw_platform != 0x37) { > - bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)", > - ver->hw_platform); > - return -EINVAL; > - } > - > - /* Check for supported iBT hardware variants of this firmware > - * loading method. > - * > - * This check has been put in place to ensure correct forward > - * compatibility options when newer hardware variants come along. > - */ > - switch (ver->hw_variant) { > - case 0x0b: /* SfP */ > - case 0x0c: /* WsP */ > - case 0x11: /* JfP */ > - case 0x12: /* ThP */ > - case 0x13: /* HrP */ > - case 0x14: /* CcP */ > - break; > - default: > - bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)", > - ver->hw_variant); > - return -EINVAL; > - } > - > btintel_version_info(hdev, ver); > > /* The firmware variant determines if the device is in bootloader > @@ -2741,16 +2728,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > if (ver->fw_variant == 0x23) { > clear_bit(BTUSB_BOOTLOADER, &data->flags); > btintel_check_bdaddr(hdev); > - return 0; > - } > - > - /* If the device is not in bootloader mode, then the only possible > - * choice is to return an error and abort the device initialization. > - */ > - if (ver->fw_variant != 0x06) { > - bt_dev_err(hdev, "Unsupported Intel firmware variant (%u)", > - ver->fw_variant); > - return -ENODEV; > + /* Proceed to download to check if the version matches */ > + goto download; > } > > /* Read the secure boot parameters to identify the operating > @@ -2778,6 +2757,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > } > > +download: > /* With this Intel bootloader only the hardware variant and device > * revision information are used to select the right firmware for SfP > * and WsP. > @@ -2801,7 +2781,13 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > */ > err = btusb_setup_intel_new_get_fw_name(ver, params, fwname, > sizeof(fwname), "sfi"); > - if (!err) { > + if (err < 0) { > + if (err == -EALREADY) { > + /* Firmware has already been loaded */ > + set_bit(BTUSB_FIRMWARE_LOADED, &data->flags); > + goto done; > + } > + > bt_dev_err(hdev, "Unsupported Intel firmware naming"); > return -EINVAL; > }
Hi Luiz, On Wed, 2021-02-10 at 11:20 -0800, Luiz Augusto von Dentz wrote: > Hi Tedd, > > On Wed, Feb 10, 2021 at 11:10 AM An, Tedd <tedd.an@intel.com> wrote: > > On Wed, 2021-02-10 at 08:59 -0800, Luiz Augusto von Dentz wrote: > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > > > In order to allow new firmware to be loaded it first needs to check if > > > the firmware version on file matches the one loaded if it doesn't then > > > it needs to revert to boorloader mode in order to load the new firmware. > > > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > --- > > > drivers/bluetooth/btintel.c | 22 +++++++++++ > > > drivers/bluetooth/btusb.c | 74 +++++++++++++++---------------------- > > > 2 files changed, 52 insertions(+), 44 deletions(-) > > > > > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > > > index 89f85d54ca64..0d0f643f972a 100644 > > > --- a/drivers/bluetooth/btintel.c > > > +++ b/drivers/bluetooth/btintel.c > > > @@ -949,6 +949,17 @@ int btintel_download_firmware(struct hci_dev *hdev, > > > return -EALREADY; > > > } > > > > > > + /* The firmware variant determines if the device is in bootloader > > > + * mode or is running operational firmware. The value 0x06 identifies > > > + * the bootloader and the value 0x23 identifies the operational > > > + * firmware. > > > + * > > > + * If the firmware version has changed that means it needs to be reset > > > + * to bootloader when operational so the new firmware can be loaded. > > > + */ > > > + if (ver->fw_variant == 0x23) > > > + return -EINVAL; > > > + > > > err = btintel_sfi_rsa_header_secure_send(hdev, fw); > > > if (err) > > > return err; > > > @@ -976,6 +987,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev, > > > return -EALREADY; > > > } > > > > > > + /* The firmware variant determines if the device is in bootloader > > > + * mode or is running operational firmware. The value 0x03 identifies > > > + * the bootloader and the value 0x23 identifies the operational > > > + * firmware. > > > + * > > > + * If the firmware version has changed that means it needs to be reset > > > + * to bootloader when operational so the new firmware can be loaded. > > > + */ > > > + if (ver->img_type == 0x03) > > > + return -EINVAL; > > > + > > > /* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support > > > * only RSA secure boot engine. Hence, the corresponding sfi file will > > > * have RSA header of 644 bytes followed by Command Buffer. > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > > index c92060e7472c..a44f3cf25790 100644 > > > --- a/drivers/bluetooth/btusb.c > > > +++ b/drivers/bluetooth/btusb.c > > > @@ -2469,14 +2469,30 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb) > > > return -EILSEQ; > > > } > > > > > > -static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver, > > > +static int btusb_setup_intel_new_get_fw_name(struct intel_version *ver, > > > struct intel_boot_params *params, > > > char *fw_name, size_t len, > > > const char *suffix) > > > { > > > + /* The hardware platform number has a fixed value of 0x37 and > > > + * for now only accept this single value. > > > + */ > > > + if (ver->hw_platform != 0x37) > > > + return -EINVAL; > > > + > > > switch (ver->hw_variant) { > > > case 0x0b: /* SfP */ > > > case 0x0c: /* WsP */ > > > + /* The firmware variant determines if the device is in > > > + * bootloader mode or is running operational firmware. > > > + * > > > + * Version checking cannot be performed in these models since > > > + * the firmware versioning depends on the firmware being in > > > + * bootloader mode. > > > + */ > > > + if (ver->fw_variant == 0x23) > > > + return -EALREADY; > > > + > > > snprintf(fw_name, len, "intel/ibt-%u-%u.%s", > > > le16_to_cpu(ver->hw_variant), > > > le16_to_cpu(params->dev_revid), > > > @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver, > > > suffix); > > > break; > > > default: > > > - return false; > > > + return -EINVAL; > > > } > > > - return true; > > > + > > > + return 0; > > > > There is one more place in btusb_setup_intel_new()@btusb.c to update the handling of return > > value of this funcion, which is related to loading the DDC. > > Code like this... > > > > if (!err) { > > bt_dev_err(hdev, "Unsupported Intel firmware naming"); > > } else { > > This should never fail though since the first one would check if the > firmware name cannot be generated if would fail and never reach this > one, that said perhaps static analyzer will complain about not > checking the return here. > It is not for failure case. For success case, it should get the DDC file name. It gets correct name with err=0, which should be an error. > > > > > } > > > > > > static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv, > > > @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev, > > > if (ver->img_type == 0x03) { > > > clear_bit(BTUSB_BOOTLOADER, &data->flags); > > > btintel_check_bdaddr(hdev); > > > - return 0; > > > } > > > > > > /* Check for supported iBT hardware variants of this firmware > > > @@ -2694,35 +2710,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > > > if (!ver || !params) > > > return -EINVAL; > > > > > > - /* The hardware platform number has a fixed value of 0x37 and > > > - * for now only accept this single value. > > > - */ > > > - if (ver->hw_platform != 0x37) { > > > - bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)", > > > - ver->hw_platform); > > > - return -EINVAL; > > > - } > > > - > > > - /* Check for supported iBT hardware variants of this firmware > > > - * loading method. > > > - * > > > - * This check has been put in place to ensure correct forward > > > - * compatibility options when newer hardware variants come along. > > > - */ > > > - switch (ver->hw_variant) { > > > - case 0x0b: /* SfP */ > > > - case 0x0c: /* WsP */ > > > - case 0x11: /* JfP */ > > > - case 0x12: /* ThP */ > > > - case 0x13: /* HrP */ > > > - case 0x14: /* CcP */ > > > - break; > > > - default: > > > - bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)", > > > - ver->hw_variant); > > > - return -EINVAL; > > > - } > > > - > > > btintel_version_info(hdev, ver); > > > > > > /* The firmware variant determines if the device is in bootloader > > > @@ -2741,16 +2728,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > > > if (ver->fw_variant == 0x23) { > > > clear_bit(BTUSB_BOOTLOADER, &data->flags); > > > btintel_check_bdaddr(hdev); > > > - return 0; > > > - } > > > - > > > - /* If the device is not in bootloader mode, then the only possible > > > - * choice is to return an error and abort the device initialization. > > > - */ > > > - if (ver->fw_variant != 0x06) { > > > - bt_dev_err(hdev, "Unsupported Intel firmware variant (%u)", > > > - ver->fw_variant); > > > - return -ENODEV; > > > + /* Proceed to download to check if the version matches */ > > > + goto download; > > > } > > > > > > /* Read the secure boot parameters to identify the operating > > > @@ -2778,6 +2757,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > > > set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > > > } > > > > > > +download: > > > /* With this Intel bootloader only the hardware variant and device > > > * revision information are used to select the right firmware for SfP > > > * and WsP. > > > @@ -2801,7 +2781,13 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, > > > */ > > > err = btusb_setup_intel_new_get_fw_name(ver, params, fwname, > > > sizeof(fwname), "sfi"); > > > - if (!err) { > > > + if (err < 0) { > > > + if (err == -EALREADY) { > > > + /* Firmware has already been loaded */ > > > + set_bit(BTUSB_FIRMWARE_LOADED, &data->flags); > > > + goto done; > > > + } > > > + > > > bt_dev_err(hdev, "Unsupported Intel firmware naming"); > > > return -EINVAL; > > > } > >
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index 89f85d54ca64..0d0f643f972a 100644 --- a/drivers/bluetooth/btintel.c +++ b/drivers/bluetooth/btintel.c @@ -949,6 +949,17 @@ int btintel_download_firmware(struct hci_dev *hdev, return -EALREADY; } + /* The firmware variant determines if the device is in bootloader + * mode or is running operational firmware. The value 0x06 identifies + * the bootloader and the value 0x23 identifies the operational + * firmware. + * + * If the firmware version has changed that means it needs to be reset + * to bootloader when operational so the new firmware can be loaded. + */ + if (ver->fw_variant == 0x23) + return -EINVAL; + err = btintel_sfi_rsa_header_secure_send(hdev, fw); if (err) return err; @@ -976,6 +987,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev, return -EALREADY; } + /* The firmware variant determines if the device is in bootloader + * mode or is running operational firmware. The value 0x03 identifies + * the bootloader and the value 0x23 identifies the operational + * firmware. + * + * If the firmware version has changed that means it needs to be reset + * to bootloader when operational so the new firmware can be loaded. + */ + if (ver->img_type == 0x03) + return -EINVAL; + /* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support * only RSA secure boot engine. Hence, the corresponding sfi file will * have RSA header of 644 bytes followed by Command Buffer. diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index c92060e7472c..a44f3cf25790 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2469,14 +2469,30 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb) return -EILSEQ; } -static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver, +static int btusb_setup_intel_new_get_fw_name(struct intel_version *ver, struct intel_boot_params *params, char *fw_name, size_t len, const char *suffix) { + /* The hardware platform number has a fixed value of 0x37 and + * for now only accept this single value. + */ + if (ver->hw_platform != 0x37) + return -EINVAL; + switch (ver->hw_variant) { case 0x0b: /* SfP */ case 0x0c: /* WsP */ + /* The firmware variant determines if the device is in + * bootloader mode or is running operational firmware. + * + * Version checking cannot be performed in these models since + * the firmware versioning depends on the firmware being in + * bootloader mode. + */ + if (ver->fw_variant == 0x23) + return -EALREADY; + snprintf(fw_name, len, "intel/ibt-%u-%u.%s", le16_to_cpu(ver->hw_variant), le16_to_cpu(params->dev_revid), @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver, suffix); break; default: - return false; + return -EINVAL; } - return true; + + return 0; } static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv, @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev, if (ver->img_type == 0x03) { clear_bit(BTUSB_BOOTLOADER, &data->flags); btintel_check_bdaddr(hdev); - return 0; } /* Check for supported iBT hardware variants of this firmware @@ -2694,35 +2710,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, if (!ver || !params) return -EINVAL; - /* The hardware platform number has a fixed value of 0x37 and - * for now only accept this single value. - */ - if (ver->hw_platform != 0x37) { - bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)", - ver->hw_platform); - return -EINVAL; - } - - /* Check for supported iBT hardware variants of this firmware - * loading method. - * - * This check has been put in place to ensure correct forward - * compatibility options when newer hardware variants come along. - */ - switch (ver->hw_variant) { - case 0x0b: /* SfP */ - case 0x0c: /* WsP */ - case 0x11: /* JfP */ - case 0x12: /* ThP */ - case 0x13: /* HrP */ - case 0x14: /* CcP */ - break; - default: - bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)", - ver->hw_variant); - return -EINVAL; - } - btintel_version_info(hdev, ver); /* The firmware variant determines if the device is in bootloader @@ -2741,16 +2728,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, if (ver->fw_variant == 0x23) { clear_bit(BTUSB_BOOTLOADER, &data->flags); btintel_check_bdaddr(hdev); - return 0; - } - - /* If the device is not in bootloader mode, then the only possible - * choice is to return an error and abort the device initialization. - */ - if (ver->fw_variant != 0x06) { - bt_dev_err(hdev, "Unsupported Intel firmware variant (%u)", - ver->fw_variant); - return -ENODEV; + /* Proceed to download to check if the version matches */ + goto download; } /* Read the secure boot parameters to identify the operating @@ -2778,6 +2757,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); } +download: /* With this Intel bootloader only the hardware variant and device * revision information are used to select the right firmware for SfP * and WsP. @@ -2801,7 +2781,13 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev, */ err = btusb_setup_intel_new_get_fw_name(ver, params, fwname, sizeof(fwname), "sfi"); - if (!err) { + if (err < 0) { + if (err == -EALREADY) { + /* Firmware has already been loaded */ + set_bit(BTUSB_FIRMWARE_LOADED, &data->flags); + goto done; + } + bt_dev_err(hdev, "Unsupported Intel firmware naming"); return -EINVAL; }