[PATCH v2 0/2] fix FMP versioning for FWU multi bank update
The current FMP versioning does not work when CONFIG_FWU_MULTI_BANK_UPDATE is enabled. This series aims to support FMP versioning for FWU multi bank update.
[Changelog] v1 -> v2 - update fwu_get_image_index() function to return the dfu_alt_num - store firmware version into the array of fmp_state structure
Masahisa Kojima (2): fwu: fix fwu_get_image_index interface efi_loader: support fmp versioning for multi bank update
include/fwu.h | 13 +++--- lib/efi_loader/efi_firmware.c | 80 +++++++++++++++++++++++++++-------- lib/fwu_updates/fwu.c | 32 ++++++-------- 3 files changed, 80 insertions(+), 45 deletions(-)
The capsule update uses the DFU framework for updating storage. fwu_get_image_index() currently returns the image_index calculated by (dfu_alt_num + 1), but this is different from the image_index in UEFI terminology.
Since capsule update implementation calls dfu_write_by_alt function, it is better that FWU returns the dfu_alt_num.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- include/fwu.h | 13 +++++-------- lib/efi_loader/efi_firmware.c | 11 +++++++++-- lib/fwu_updates/fwu.c | 32 ++++++++++++-------------------- 3 files changed, 26 insertions(+), 30 deletions(-)
diff --git a/include/fwu.h b/include/fwu.h index ac5c5de870..eb5638f4f3 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -122,21 +122,18 @@ int fwu_get_active_index(uint *active_idxp); int fwu_set_active_index(uint active_idx);
/** - * fwu_get_image_index() - Get the Image Index to be used for capsule update - * @image_index: The Image Index for the image - * - * The FWU multi bank update feature computes the value of image_index at - * runtime, based on the bank to which the image needs to be written to. - * Derive the image_index value for the image. + * fwu_get_dfu_alt_num() - Get the dfu_alt_num to be used for capsule update + * @image_index: The Image Index for the image + * @alt_num: pointer to store dfu_alt_num * * Currently, the capsule update driver uses the DFU framework for * the updates. This function gets the DFU alt number which is to - * be used as the Image Index + * be used for capsule update. * * Return: 0 if OK, -ve on error * */ -int fwu_get_image_index(u8 *image_index); +int fwu_get_dfu_alt_num(u8 image_index, u8 *alt_num);
/** * fwu_revert_boot_index() - Revert the active index in the FWU metadata diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9abb29f1df..9b8630625f 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -611,6 +611,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( u16 **abort_reason) { int ret; + u8 dfu_alt_num; efi_status_t status; struct fmp_state state = { 0 };
@@ -625,19 +626,25 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( if (status != EFI_SUCCESS) return EFI_EXIT(status);
+ /* + * dfu_alt_num is assigned from 0 while image_index starts from 1. + * dfu_alt_num is calculated by (image_index - 1) when multi bank update + * is not used. + */ + dfu_alt_num = image_index - 1; if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { /* * Based on the value of update bank, derive the * image index value. */ - ret = fwu_get_image_index(&image_index); + ret = fwu_get_dfu_alt_num(image_index, &dfu_alt_num); if (ret) { log_debug("Unable to get FWU image_index\n"); return EFI_EXIT(EFI_DEVICE_ERROR); } }
- if (dfu_write_by_alt(image_index - 1, (void *)image, image_size, + if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, NULL, NULL)) return EFI_EXIT(EFI_DEVICE_ERROR);
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index b580574015..86518108c2 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -125,16 +125,14 @@ static int in_trial_state(struct fwu_mdata *mdata) return 0; }
-static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id) +static int fwu_get_image_type_id(u8 image_index, efi_guid_t *image_type_id) { - u8 index; int i; struct efi_fw_image *image;
- index = *image_index; image = update_info.images; for (i = 0; i < update_info.num_images; i++) { - if (index == image[i].image_index) { + if (image_index == image[i].image_index) { guidcpy(image_type_id, &image[i].image_type_id); return 0; } @@ -332,24 +330,20 @@ int fwu_set_active_index(uint active_idx) }
/** - * fwu_get_image_index() - Get the Image Index to be used for capsule update - * @image_index: The Image Index for the image - * - * The FWU multi bank update feature computes the value of image_index at - * runtime, based on the bank to which the image needs to be written to. - * Derive the image_index value for the image. + * fwu_get_dfu_alt_num() - Get the dfu_alt_num to be used for capsule update + * @image_index: The Image Index for the image + * @alt_num: pointer to store dfu_alt_num * * Currently, the capsule update driver uses the DFU framework for * the updates. This function gets the DFU alt number which is to - * be used as the Image Index + * be used for capsule update. * * Return: 0 if OK, -ve on error * */ -int fwu_get_image_index(u8 *image_index) +int fwu_get_dfu_alt_num(u8 image_index, u8 *alt_num) { int ret, i; - u8 alt_num; uint update_bank; efi_guid_t *image_guid, image_type_id; struct fwu_mdata *mdata = &g_mdata; @@ -365,7 +359,7 @@ int fwu_get_image_index(u8 *image_index) ret = fwu_get_image_type_id(image_index, &image_type_id); if (ret) { log_debug("Unable to get image_type_id for image_index %u\n", - *image_index); + image_index); goto out; }
@@ -380,15 +374,13 @@ int fwu_get_image_index(u8 *image_index) img_entry = &mdata->img_entry[i]; img_bank_info = &img_entry->img_bank_info[update_bank]; image_guid = &img_bank_info->image_uuid; - ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num); - if (ret) { + ret = fwu_plat_get_alt_num(g_dev, image_guid, alt_num); + if (ret) log_debug("alt_num not found for partition with GUID %pUs\n", image_guid); - } else { + else log_debug("alt_num %d for partition %pUs\n", - alt_num, image_guid); - *image_index = alt_num + 1; - } + *alt_num, image_guid);
goto out; }
On Thu, 21 Dec 2023 at 11:54, Masahisa Kojima masahisa.kojima@linaro.org wrote:
The capsule update uses the DFU framework for updating storage. fwu_get_image_index() currently returns the image_index calculated by (dfu_alt_num + 1), but this is different from the image_index in UEFI terminology.
Since capsule update implementation calls dfu_write_by_alt function, it is better that FWU returns the dfu_alt_num.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/fwu.h | 13 +++++-------- lib/efi_loader/efi_firmware.c | 11 +++++++++-- lib/fwu_updates/fwu.c | 32 ++++++++++++-------------------- 3 files changed, 26 insertions(+), 30 deletions(-)
diff --git a/include/fwu.h b/include/fwu.h index ac5c5de870..eb5638f4f3 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -122,21 +122,18 @@ int fwu_get_active_index(uint *active_idxp); int fwu_set_active_index(uint active_idx);
/**
- fwu_get_image_index() - Get the Image Index to be used for capsule update
- @image_index: The Image Index for the image
- The FWU multi bank update feature computes the value of image_index at
- runtime, based on the bank to which the image needs to be written to.
- Derive the image_index value for the image.
- fwu_get_dfu_alt_num() - Get the dfu_alt_num to be used for capsule update
- @image_index: The Image Index for the image
- @alt_num: pointer to store dfu_alt_num
- Currently, the capsule update driver uses the DFU framework for
- the updates. This function gets the DFU alt number which is to
- be used as the Image Index
*/
- be used for capsule update.
- Return: 0 if OK, -ve on error
-int fwu_get_image_index(u8 *image_index); +int fwu_get_dfu_alt_num(u8 image_index, u8 *alt_num);
/**
- fwu_revert_boot_index() - Revert the active index in the FWU metadata
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9abb29f1df..9b8630625f 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -611,6 +611,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( u16 **abort_reason) { int ret;
u8 dfu_alt_num; efi_status_t status; struct fmp_state state = { 0 };@@ -625,19 +626,25 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( if (status != EFI_SUCCESS) return EFI_EXIT(status);
/** dfu_alt_num is assigned from 0 while image_index starts from 1.* dfu_alt_num is calculated by (image_index - 1) when multi bank update* is not used.*/dfu_alt_num = image_index - 1; if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { /* * Based on the value of update bank, derive the * image index value. */
ret = fwu_get_image_index(&image_index);
ret = fwu_get_dfu_alt_num(image_index, &dfu_alt_num); if (ret) { log_debug("Unable to get FWU image_index\n"); return EFI_EXIT(EFI_DEVICE_ERROR); } }
if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, NULL, NULL)) return EFI_EXIT(EFI_DEVICE_ERROR);diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index b580574015..86518108c2 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -125,16 +125,14 @@ static int in_trial_state(struct fwu_mdata *mdata) return 0; }
-static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id) +static int fwu_get_image_type_id(u8 image_index, efi_guid_t *image_type_id) {
u8 index; int i; struct efi_fw_image *image;index = *image_index; image = update_info.images; for (i = 0; i < update_info.num_images; i++) {if (index == image[i].image_index) {
if (image_index == image[i].image_index) { guidcpy(image_type_id, &image[i].image_type_id); return 0; }@@ -332,24 +330,20 @@ int fwu_set_active_index(uint active_idx) }
/**
- fwu_get_image_index() - Get the Image Index to be used for capsule update
- @image_index: The Image Index for the image
- The FWU multi bank update feature computes the value of image_index at
- runtime, based on the bank to which the image needs to be written to.
- Derive the image_index value for the image.
- fwu_get_dfu_alt_num() - Get the dfu_alt_num to be used for capsule update
- @image_index: The Image Index for the image
- @alt_num: pointer to store dfu_alt_num
- Currently, the capsule update driver uses the DFU framework for
- the updates. This function gets the DFU alt number which is to
- be used as the Image Index
*/
- be used for capsule update.
- Return: 0 if OK, -ve on error
-int fwu_get_image_index(u8 *image_index) +int fwu_get_dfu_alt_num(u8 image_index, u8 *alt_num) { int ret, i;
u8 alt_num; uint update_bank; efi_guid_t *image_guid, image_type_id; struct fwu_mdata *mdata = &g_mdata;@@ -365,7 +359,7 @@ int fwu_get_image_index(u8 *image_index) ret = fwu_get_image_type_id(image_index, &image_type_id); if (ret) { log_debug("Unable to get image_type_id for image_index %u\n",
*image_index);
image_index); goto out; }@@ -380,15 +374,13 @@ int fwu_get_image_index(u8 *image_index) img_entry = &mdata->img_entry[i]; img_bank_info = &img_entry->img_bank_info[update_bank]; image_guid = &img_bank_info->image_uuid;
ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num);if (ret) {
ret = fwu_plat_get_alt_num(g_dev, image_guid, alt_num);if (ret) log_debug("alt_num not found for partition with GUID %pUs\n", image_guid);
} else {
else log_debug("alt_num %d for partition %pUs\n",
alt_num, image_guid);*image_index = alt_num + 1;}
*alt_num, image_guid); goto out; }-- 2.34.1
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
This modification keeps the backward compatibility with the existing versioning feature.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 15 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9b8630625f..5459f3d38d 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret; - efi_uintn_t size; - struct fmp_state var_state = { 0 }; - - efi_create_indexed_name(varname, sizeof(varname), "FmpState", - fw_array->image_index); - size = sizeof(var_state); - ret = efi_get_variable_int(varname, &fw_array->image_type_id, - NULL, &size, &var_state, NULL); - if (ret == EFI_SUCCESS) - image_info->version = var_state.fw_version; - else - image_info->version = 0; + efi_uintn_t size, expected_size; + uint num_banks = 1; + uint active_index = 0; + struct fmp_state *var_state;
efi_firmware_get_lsv_from_dtb(fw_array->image_index, &fw_array->image_type_id, @@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ image_info->version_name = NULL; /* not supported */ image_info->last_attempt_version = 0; image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; + image_info->version = 0; + + /* get the fw_version */ + efi_create_indexed_name(varname, sizeof(varname), "FmpState", + fw_array->image_index); + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { + ret = fwu_get_active_index(&active_index); + if (ret) + return; + + num_banks = CONFIG_FWU_NUM_BANKS; + } + + size = num_banks * sizeof(*var_state); + expected_size = size; + var_state = calloc(1, size); + if (!var_state) + return; + + ret = efi_get_variable_int(varname, &fw_array->image_type_id, + NULL, &size, var_state, NULL); + if (ret == EFI_SUCCESS && expected_size == size) + image_info->version = var_state[active_index].fw_version; + + free(var_state); }
/** @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret; + uint num_banks = 1; + uint update_bank = 0; + efi_uintn_t size; efi_guid_t *image_type_id; - struct fmp_state var_state = { 0 }; + struct fmp_state *var_state;
image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id) @@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
+ if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { + ret = fwu_plat_get_update_index(&update_bank); + if (ret) + return EFI_INVALID_PARAMETER; + + num_banks = CONFIG_FWU_NUM_BANKS; + } + + size = num_banks * sizeof(*var_state); + var_state = calloc(1, size); + if (!var_state) + return EFI_OUT_OF_RESOURCES; + + ret = efi_get_variable_int(varname, image_type_id, NULL, &size, + var_state, NULL); + /* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */ - var_state.fw_version = state->fw_version; + var_state[update_bank].fw_version = state->fw_version;
+ size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, - sizeof(var_state), &var_state, false); + size, var_state, false); + + free(var_state);
return ret; }
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
This modification keeps the backward compatibility with the existing versioning feature.
Thanks /Ilias
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 15 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9b8630625f..5459f3d38d 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
- efi_uintn_t size;
- struct fmp_state var_state = { 0 };
- efi_create_indexed_name(varname, sizeof(varname), "FmpState",
fw_array->image_index);- size = sizeof(var_state);
- ret = efi_get_variable_int(varname, &fw_array->image_type_id,
NULL, &size, &var_state, NULL);- if (ret == EFI_SUCCESS)
image_info->version = var_state.fw_version;- else
image_info->version = 0;
efi_uintn_t size, expected_size;
uint num_banks = 1;
uint active_index = 0;
struct fmp_state *var_state;
efi_firmware_get_lsv_from_dtb(fw_array->image_index, &fw_array->image_type_id,
@@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ image_info->version_name = NULL; /* not supported */ image_info->last_attempt_version = 0; image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
- image_info->version = 0;
- /* get the fw_version */
- efi_create_indexed_name(varname, sizeof(varname), "FmpState",
fw_array->image_index);- if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
ret = fwu_get_active_index(&active_index);if (ret)return;num_banks = CONFIG_FWU_NUM_BANKS;- }
- size = num_banks * sizeof(*var_state);
- expected_size = size;
- var_state = calloc(1, size);
- if (!var_state)
return;- ret = efi_get_variable_int(varname, &fw_array->image_type_id,
NULL, &size, var_state, NULL);- if (ret == EFI_SUCCESS && expected_size == size)
image_info->version = var_state[active_index].fw_version;- free(var_state);
}
/** @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
- uint num_banks = 1;
- uint update_bank = 0;
- efi_uintn_t size; efi_guid_t *image_type_id;
- struct fmp_state var_state = { 0 };
struct fmp_state *var_state;
image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id)
@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
- if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
ret = fwu_plat_get_update_index(&update_bank);if (ret)return EFI_INVALID_PARAMETER;num_banks = CONFIG_FWU_NUM_BANKS;- }
- size = num_banks * sizeof(*var_state);
- var_state = calloc(1, size);
- if (!var_state)
return EFI_OUT_OF_RESOURCES;- ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
var_state, NULL);- /*
*/
- Only the fw_version is set here.
- lowest_supported_version in FmpState variable is ignored since
- it can be tampered if the file based EFI variable storage is used.
- var_state.fw_version = state->fw_version;
var_state[update_bank].fw_version = state->fw_version;
size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
sizeof(var_state), &var_state, false);
size, var_state, false);free(var_state);
return ret;
}
2.34.1
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
Sorry for the late reply. When the capsule update is reverted, the active bank is back to the previous active bank. So I think versions for all banks should be stored.
Thanks, Masahisa Kojima
This modification keeps the backward compatibility with the existing versioning feature.
Thanks /Ilias
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 15 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9b8630625f..5459f3d38d 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
efi_uintn_t size;struct fmp_state var_state = { 0 };efi_create_indexed_name(varname, sizeof(varname), "FmpState",fw_array->image_index);size = sizeof(var_state);ret = efi_get_variable_int(varname, &fw_array->image_type_id,NULL, &size, &var_state, NULL);if (ret == EFI_SUCCESS)image_info->version = var_state.fw_version;elseimage_info->version = 0;
efi_uintn_t size, expected_size;uint num_banks = 1;uint active_index = 0;struct fmp_state *var_state; efi_firmware_get_lsv_from_dtb(fw_array->image_index, &fw_array->image_type_id,@@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ image_info->version_name = NULL; /* not supported */ image_info->last_attempt_version = 0; image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
image_info->version = 0;/* get the fw_version */efi_create_indexed_name(varname, sizeof(varname), "FmpState",fw_array->image_index);if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_get_active_index(&active_index);if (ret)return;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);expected_size = size;var_state = calloc(1, size);if (!var_state)return;ret = efi_get_variable_int(varname, &fw_array->image_type_id,NULL, &size, var_state, NULL);if (ret == EFI_SUCCESS && expected_size == size)image_info->version = var_state[active_index].fw_version;free(var_state);}
/** @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
uint num_banks = 1;uint update_bank = 0;efi_uintn_t size; efi_guid_t *image_type_id;
struct fmp_state var_state = { 0 };
struct fmp_state *var_state; image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id)@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_plat_get_update_index(&update_bank);if (ret)return EFI_INVALID_PARAMETER;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);var_state = calloc(1, size);if (!var_state)return EFI_OUT_OF_RESOURCES;ret = efi_get_variable_int(varname, image_type_id, NULL, &size,var_state, NULL);/* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */
var_state.fw_version = state->fw_version;
var_state[update_bank].fw_version = state->fw_version;size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
sizeof(var_state), &var_state, false);
size, var_state, false);free(var_state); return ret;}
2.34.1
On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
Sorry for the late reply. When the capsule update is reverted, the active bank is back to the previous active bank. So I think versions for all banks should be stored.
But even in that case, the active bank should point to the correct version when the ESRT tables are generated. Wouldn't it be simpler to just set the FmpState variable accordingly when that happens? Am I missing anything?
Thanks /Ilias
Thanks, Masahisa Kojima
This modification keeps the backward compatibility with the existing versioning feature.
Thanks /Ilias
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 15 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9b8630625f..5459f3d38d 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
efi_uintn_t size;struct fmp_state var_state = { 0 };efi_create_indexed_name(varname, sizeof(varname), "FmpState",fw_array->image_index);size = sizeof(var_state);ret = efi_get_variable_int(varname, &fw_array->image_type_id,NULL, &size, &var_state, NULL);if (ret == EFI_SUCCESS)image_info->version = var_state.fw_version;elseimage_info->version = 0;
efi_uintn_t size, expected_size;uint num_banks = 1;uint active_index = 0;struct fmp_state *var_state; efi_firmware_get_lsv_from_dtb(fw_array->image_index, &fw_array->image_type_id,@@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ image_info->version_name = NULL; /* not supported */ image_info->last_attempt_version = 0; image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
image_info->version = 0;/* get the fw_version */efi_create_indexed_name(varname, sizeof(varname), "FmpState",fw_array->image_index);if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_get_active_index(&active_index);if (ret)return;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);expected_size = size;var_state = calloc(1, size);if (!var_state)return;ret = efi_get_variable_int(varname, &fw_array->image_type_id,NULL, &size, var_state, NULL);if (ret == EFI_SUCCESS && expected_size == size)image_info->version = var_state[active_index].fw_version;free(var_state);}
/** @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
uint num_banks = 1;uint update_bank = 0;efi_uintn_t size; efi_guid_t *image_type_id;
struct fmp_state var_state = { 0 };
struct fmp_state *var_state; image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id)@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_plat_get_update_index(&update_bank);if (ret)return EFI_INVALID_PARAMETER;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);var_state = calloc(1, size);if (!var_state)return EFI_OUT_OF_RESOURCES;ret = efi_get_variable_int(varname, image_type_id, NULL, &size,var_state, NULL);/* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */
var_state.fw_version = state->fw_version;
var_state[update_bank].fw_version = state->fw_version;size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
sizeof(var_state), &var_state, false);
size, var_state, false);free(var_state); return ret;}
2.34.1
On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
Sorry for the late reply. When the capsule update is reverted, the active bank is back to the previous active bank. So I think versions for all banks should be stored.
But even in that case, the active bank should point to the correct version when the ESRT tables are generated. Wouldn't it be simpler to just set the FmpState variable accordingly when that happens?
The fw_version in FmpState variable comes from UEFI Capsule Header and FmpState variable is set when the UEFI capsule update is performed. If we only store the fw_version of the active bank, the fw_version of the previous active bank is overwritten. When the capsule is reverted, we can not get the fw_version of the previous active bank.
Thanks, Masahisa Kojima
Am I missing anything?
Thanks /Ilias
Thanks, Masahisa Kojima
This modification keeps the backward compatibility with the existing versioning feature.
Thanks /Ilias
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 15 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 9b8630625f..5459f3d38d 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
efi_uintn_t size;struct fmp_state var_state = { 0 };efi_create_indexed_name(varname, sizeof(varname), "FmpState",fw_array->image_index);size = sizeof(var_state);ret = efi_get_variable_int(varname, &fw_array->image_type_id,NULL, &size, &var_state, NULL);if (ret == EFI_SUCCESS)image_info->version = var_state.fw_version;elseimage_info->version = 0;
efi_uintn_t size, expected_size;uint num_banks = 1;uint active_index = 0;struct fmp_state *var_state; efi_firmware_get_lsv_from_dtb(fw_array->image_index, &fw_array->image_type_id,@@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ image_info->version_name = NULL; /* not supported */ image_info->last_attempt_version = 0; image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
image_info->version = 0;/* get the fw_version */efi_create_indexed_name(varname, sizeof(varname), "FmpState",fw_array->image_index);if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_get_active_index(&active_index);if (ret)return;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);expected_size = size;var_state = calloc(1, size);if (!var_state)return;ret = efi_get_variable_int(varname, &fw_array->image_type_id,NULL, &size, var_state, NULL);if (ret == EFI_SUCCESS && expected_size == size)image_info->version = var_state[active_index].fw_version;free(var_state);}
/** @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret;
uint num_banks = 1;uint update_bank = 0;efi_uintn_t size; efi_guid_t *image_type_id;
struct fmp_state var_state = { 0 };
struct fmp_state *var_state; image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id)@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_plat_get_update_index(&update_bank);if (ret)return EFI_INVALID_PARAMETER;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);var_state = calloc(1, size);if (!var_state)return EFI_OUT_OF_RESOURCES;ret = efi_get_variable_int(varname, image_type_id, NULL, &size,var_state, NULL);/* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */
var_state.fw_version = state->fw_version;
var_state[update_bank].fw_version = state->fw_version;size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
sizeof(var_state), &var_state, false);
size, var_state, false);free(var_state); return ret;}
2.34.1
On Wed, 10 Jan 2024 at 02:53, Masahisa Kojima masahisa.kojima@linaro.org wrote:
On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
Sorry for the late reply. When the capsule update is reverted, the active bank is back to the previous active bank. So I think versions for all banks should be stored.
But even in that case, the active bank should point to the correct version when the ESRT tables are generated. Wouldn't it be simpler to just set the FmpState variable accordingly when that happens?
The fw_version in FmpState variable comes from UEFI Capsule Header and FmpState variable is set when the UEFI capsule update is performed. If we only store the fw_version of the active bank, the fw_version of the previous active bank is overwritten. When the capsule is reverted, we can not get the fw_version of the previous active bank.
Thanks, Masahisa Kojima
Ah yes, that makes sense. In that case, the patch looks fine, apart from the return value of the getvariable call which is never checked
image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id)@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_plat_get_update_index(&update_bank);if (ret)return EFI_INVALID_PARAMETER;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);var_state = calloc(1, size);if (!var_state)return EFI_OUT_OF_RESOURCES;ret = efi_get_variable_int(varname, image_type_id, NULL, &size,var_state, NULL);
We should be checking the return value here.
/* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */
var_state.fw_version = state->fw_version;
var_state[update_bank].fw_version = state->fw_version;size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
sizeof(var_state), &var_state, false);
size, var_state, false);free(var_state); return ret;}
2.34.1
Thanks /Ilias
On Wed, 10 Jan 2024 at 18:10, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 10 Jan 2024 at 02:53, Masahisa Kojima masahisa.kojima@linaro.org wrote:
On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
Sorry for the late reply. When the capsule update is reverted, the active bank is back to the previous active bank. So I think versions for all banks should be stored.
But even in that case, the active bank should point to the correct version when the ESRT tables are generated. Wouldn't it be simpler to just set the FmpState variable accordingly when that happens?
The fw_version in FmpState variable comes from UEFI Capsule Header and FmpState variable is set when the UEFI capsule update is performed. If we only store the fw_version of the active bank, the fw_version of the previous active bank is overwritten. When the capsule is reverted, we can not get the fw_version of the previous active bank.
Thanks, Masahisa Kojima
Ah yes, that makes sense. In that case, the patch looks fine, apart from the return value of the getvariable call which is never checked
image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id)@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index);
if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {ret = fwu_plat_get_update_index(&update_bank);if (ret)return EFI_INVALID_PARAMETER;num_banks = CONFIG_FWU_NUM_BANKS;}size = num_banks * sizeof(*var_state);var_state = calloc(1, size);if (!var_state)return EFI_OUT_OF_RESOURCES;ret = efi_get_variable_int(varname, image_type_id, NULL, &size,var_state, NULL);We should be checking the return value here.
I will fix it.
Thanks, Masahisa Kojima
/* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */
var_state.fw_version = state->fw_version;
var_state[update_bank].fw_version = state->fw_version;size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
sizeof(var_state), &var_state, false);
size, var_state, false);free(var_state); return ret;}
2.34.1
Thanks /Ilias
Hi Ilias,
On Thu, 11 Jan 2024 at 10:53, Masahisa Kojima masahisa.kojima@linaro.org wrote:
On Wed, 10 Jan 2024 at 18:10, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 10 Jan 2024 at 02:53, Masahisa Kojima masahisa.kojima@linaro.org wrote:
On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote: > This commit stores the firmware version into the array > of fmp_state structure to support the fmp versioning > for multi bank update. The index of the array is identified > by the bank index.
Why do we all of them? Can't we just always store/use the version of the active bank?
Sorry for the late reply. When the capsule update is reverted, the active bank is back to the previous active bank. So I think versions for all banks should be stored.
But even in that case, the active bank should point to the correct version when the ESRT tables are generated. Wouldn't it be simpler to just set the FmpState variable accordingly when that happens?
The fw_version in FmpState variable comes from UEFI Capsule Header and FmpState variable is set when the UEFI capsule update is performed. If we only store the fw_version of the active bank, the fw_version of the previous active bank is overwritten. When the capsule is reverted, we can not get the fw_version of the previous active bank.
Thanks, Masahisa Kojima
Ah yes, that makes sense. In that case, the patch looks fine, apart from the return value of the getvariable call which is never checked
> > image_type_id = efi_firmware_get_image_type_id(image_index); > if (!image_type_id) > @@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in > efi_create_indexed_name(varname, sizeof(varname), "FmpState", > image_index); > > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > + ret = fwu_plat_get_update_index(&update_bank); > + if (ret) > + return EFI_INVALID_PARAMETER; > + > + num_banks = CONFIG_FWU_NUM_BANKS; > + } > + > + size = num_banks * sizeof(*var_state); > + var_state = calloc(1, size); > + if (!var_state) > + return EFI_OUT_OF_RESOURCES; > + > + ret = efi_get_variable_int(varname, image_type_id, NULL, &size, > + var_state, NULL); > +
We should be checking the return value here.
I will fix it.
This efi_get_variable_int() call may fail, EFI_NOT_FOUND is returned if FmpState variable has not been set yet for example. We can ignore the error since the correct FmpState variable is set later. I will add a comment.
Thanks, Masahisa Kojima
Thanks, Masahisa Kojima
> /* > * Only the fw_version is set here. > * lowest_supported_version in FmpState variable is ignored since > * it can be tampered if the file based EFI variable storage is used. > */ > - var_state.fw_version = state->fw_version; > + var_state[update_bank].fw_version = state->fw_version; > > + size = num_banks * sizeof(*var_state); > ret = efi_set_variable_int(varname, image_type_id, > EFI_VARIABLE_READ_ONLY | > EFI_VARIABLE_NON_VOLATILE | > EFI_VARIABLE_BOOTSERVICE_ACCESS | > EFI_VARIABLE_RUNTIME_ACCESS, > - sizeof(var_state), &var_state, false); > + size, var_state, false); > + > + free(var_state); > > return ret; > } > -- > 2.34.1 >
Thanks /Ilias
participants (2)
-
Ilias Apalodimas -
Masahisa Kojima