[PATCH v5 0/4] FMP versioning support
Firmware version management is not implemented in the current FMP implementation. This series aims to add the versioning support in FMP.
There is a major design change in v5. Until v4, the fw_version and lowest_supported_version are stored as a EFI variable. If the backing storage is a file we can't trust any of that information since anyone can tamper with the file, although the variables are defined as RO. With that, we store the version information in the device tree in v5. We can trust the information from dtb as long as the former stage boot loader verifies the image containing the dtb.
The disadvantage of this design change is that we need to maintain the fw_version in both device tree and FMP Payload Header. It is inevitable since not all the capsule files contain the dtb.
EDK II reference implementation utilizes the FMP Payload Header inserted right before the capsule payload. With this series, U-Boot also follows the EDK II implementation.
Currently, there is no way to know the current running firmware version through the EFI interface. FMP->GetImageInfo() returns always 0 for the version number. So a user can not know that expected firmware is running after the capsule update.
With this series applied, version number can be specified in the capsule file generation with mkeficapsule tool, then user can know the running firmware version through FMP->GetImageInfo() and ESRT.
Note that this series does not mandate the FMP Payload Header, compatible with boards that are already using the existing U-Boot FMP implementation. If no FMP Payload Header is found in the capsule file, fw_version, lowest supported version, last attempt version and last attempt status is set to 0 and this is the same behavior as existing FMP implementation.
Major Changes in v5: - major design changes, versioning is implemented with device tree instead of EFI variable
Major Changes in v4: - add python-based test
Major Changes in v3: - exclude CONFIG_FWU_MULTI
Masahisa Kojima (4): efi_loader: get version information from device tree efi_loader: check lowest supported version mkeficapsule: add FMP Payload Header test/py: efi_capsule: test for FMP versioning
.../firmware/firmware-version.txt | 25 +++ doc/mkeficapsule.1 | 10 + lib/efi_loader/efi_firmware.c | 187 +++++++++++++--- test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ tools/eficapsule.h | 30 +++ tools/mkeficapsule.c | 37 +++- 11 files changed, 1082 insertions(+), 29 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt create mode 100644 test/py/tests/test_efi_capsule/version.dts
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- Changes in v5: - newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +------------------------------- + +Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version + +Example: + + firmware-version { + image1 { + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + image-index = <1>; + fw-version = <5>; + lowest-supported-version = <3>; + }; + image2 { + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; + image-index = <2>; + fw-version = <10>; + lowest-supported-version = <7>; + }; + }; diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/** + * efi_firmware_get_firmware_version - get firmware version from dtb + * @image_index: Image index + * @image_type_id: Image type id + * @fw_version: Pointer to store the version number + * @lsv: Pointer to store the lowest supported version + * + * Authenticate the capsule if authentication is enabled. + * The image pointer and the image size are updated in case of success. + */ +void efi_firmware_get_firmware_version(u8 image_index, + efi_guid_t *image_type_id, + u32 *fw_version, u32 *lsv) +{ + const void *fdt = gd->fdt_blob; + const fdt32_t *val; + const char *guid_str; + int len, offset, index; + int parent; + + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); + if (parent < 0) + return; + + fdt_for_each_subnode(offset, fdt, parent) { + efi_guid_t guid; + + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); + if (!guid_str) + continue; + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); + + val = fdt_getprop(fdt, offset, "image-index", &len); + if (!val) + continue; + index = fdt32_to_cpu(*val); + + if (!guidcmp(&guid, image_type_id) && index == image_index) { + val = fdt_getprop(fdt, offset, "fw-version", &len); + if (val) + *fw_version = fdt32_to_cpu(*val); + + val = fdt_getprop(fdt, offset, + "lowest-supported-version", &len); + if (val) + *lsv = fdt32_to_cpu(*val); + } + } +} + /** * efi_fill_image_desc_array - populate image descriptor array * @image_info_size: Size of @image_info @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) { + u32 fw_version = 0; + u32 lowest_supported_version = 0; + image_info[i].image_index = fw_array[i].image_index; image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index;
image_info[i].image_id_name = fw_array[i].fw_name; - - image_info[i].version = 0; /* not supported */ + efi_firmware_get_firmware_version(fw_array[i].image_index, + &fw_array[i].image_type_id, + &fw_version, + &lowest_supported_version); + image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */ image_info[i].size = 0; image_info[i].attributes_supported = @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
- image_info[i].lowest_supported_image_version = 0; + image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1; @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name); - return EFI_EXIT(ret); }
On 4/10/23 11:07, Masahisa Kojima wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
- firmware-version {
image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};- };
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/**
- efi_firmware_get_firmware_version - get firmware version from dtb
- @image_index: Image index
- @image_type_id: Image type id
- @fw_version: Pointer to store the version number
- @lsv: Pointer to store the lowest supported version
- Authenticate the capsule if authentication is enabled.
- The image pointer and the image size are updated in case of success.
- */
+void efi_firmware_get_firmware_version(u8 image_index,
efi_guid_t *image_type_id,u32 *fw_version, u32 *lsv)+{
- const void *fdt = gd->fdt_blob;
- const fdt32_t *val;
- const char *guid_str;
- int len, offset, index;
- int parent;
- parent = fdt_subnode_offset(fdt, 0, "firmware-version");
- if (parent < 0)
return;- fdt_for_each_subnode(offset, fdt, parent) {
efi_guid_t guid;guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);if (!guid_str)continue;uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);val = fdt_getprop(fdt, offset, "image-index", &len);if (!val)continue;index = fdt32_to_cpu(*val);if (!guidcmp(&guid, image_type_id) && index == image_index) {val = fdt_getprop(fdt, offset, "fw-version", &len);if (val)*fw_version = fdt32_to_cpu(*val);val = fdt_getprop(fdt, offset,"lowest-supported-version", &len);if (val)*lsv = fdt32_to_cpu(*val);}- }
+}
- /**
- efi_fill_image_desc_array - populate image descriptor array
- @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {
Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c.
u32 fw_version = 0;u32 lowest_supported_version = 0;
These assignments should be in efi_firmware_get_firmware_version.
- image_info[i].image_index = fw_array[i].image_index;
Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image.
image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name;
image_info[i].version = 0; /* not supported */
efi_firmware_get_firmware_version(fw_array[i].image_index,&fw_array[i].image_type_id,&fw_version,&lowest_supported_version);
This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
image_info[i].version = fw_version;
Please, add the missing functionality to efi_firmware_get_firmware_version().
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code.
Best regards
Heinrich
image_info[i].size = 0; image_info[i].attributes_supported =@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
image_info[i].lowest_supported_image_version = 0;
image_info[i].lowest_supported_image_version = lowest_supported_version;@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name);
- return EFI_EXIT(ret); }
Hi Heinrich,
On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/10/23 11:07, Masahisa Kojima wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/**
- efi_firmware_get_firmware_version - get firmware version from dtb
- @image_index: Image index
- @image_type_id: Image type id
- @fw_version: Pointer to store the version number
- @lsv: Pointer to store the lowest supported version
- Authenticate the capsule if authentication is enabled.
- The image pointer and the image size are updated in case of success.
- */
+void efi_firmware_get_firmware_version(u8 image_index,
efi_guid_t *image_type_id,u32 *fw_version, u32 *lsv)+{
const void *fdt = gd->fdt_blob;const fdt32_t *val;const char *guid_str;int len, offset, index;int parent;parent = fdt_subnode_offset(fdt, 0, "firmware-version");if (parent < 0)return;fdt_for_each_subnode(offset, fdt, parent) {efi_guid_t guid;guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);if (!guid_str)continue;uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);val = fdt_getprop(fdt, offset, "image-index", &len);if (!val)continue;index = fdt32_to_cpu(*val);if (!guidcmp(&guid, image_type_id) && index == image_index) {val = fdt_getprop(fdt, offset, "fw-version", &len);if (val)*fw_version = fdt32_to_cpu(*val);val = fdt_getprop(fdt, offset,"lowest-supported-version", &len);if (val)*lsv = fdt32_to_cpu(*val);}}+}
- /**
- efi_fill_image_desc_array - populate image descriptor array
- @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c.
Sorry for the late reply.
num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", fw_images[] array is also defined in each board file, so we can not simply move num_image_type_guids into lib/efi_loader/efi_firmware.c.
And fw_images[] array is abstracted by struct efi_capsule_update_info, so I think we should not extract the fw_images[] array.
u32 fw_version = 0;u32 lowest_supported_version = 0;These assignments should be in efi_firmware_get_firmware_version.
OK.
image_info[i].image_index = fw_array[i].image_index;Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image.
This is derived from the UEFI specification. UEFI specification "23.1.2 EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex and ImageTypeId(guid).
ImageIndex: A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount.
image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name;
image_info[i].version = 0; /* not supported */
efi_firmware_get_firmware_version(fw_array[i].image_index,&fw_array[i].image_type_id,&fw_version,&lowest_supported_version);This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
I don't clearly understand this comment, but the expected GUID is fw_array[i].image_type_id.
image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */Please, add the missing functionality to efi_firmware_get_firmware_version().
Does it mean we need to support version_name? I can add a version_name in dtb.
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code.
OK.
Thank you for your review.
Regards, Masahisa Kojima
Best regards
Heinrich
image_info[i].size = 0; image_info[i].attributes_supported =@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
image_info[i].lowest_supported_image_version = 0;
image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1;@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name);
return EFI_EXIT(ret);
On 5/8/23 10:15, Masahisa Kojima wrote:
Hi Heinrich,
On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/10/23 11:07, Masahisa Kojima wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/**
- efi_firmware_get_firmware_version - get firmware version from dtb
- @image_index: Image index
- @image_type_id: Image type id
- @fw_version: Pointer to store the version number
- @lsv: Pointer to store the lowest supported version
- Authenticate the capsule if authentication is enabled.
- The image pointer and the image size are updated in case of success.
- */
+void efi_firmware_get_firmware_version(u8 image_index,
efi_guid_t *image_type_id,u32 *fw_version, u32 *lsv)+{
const void *fdt = gd->fdt_blob;const fdt32_t *val;const char *guid_str;int len, offset, index;int parent;parent = fdt_subnode_offset(fdt, 0, "firmware-version");if (parent < 0)return;fdt_for_each_subnode(offset, fdt, parent) {efi_guid_t guid;guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);if (!guid_str)continue;uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);val = fdt_getprop(fdt, offset, "image-index", &len);if (!val)continue;index = fdt32_to_cpu(*val);if (!guidcmp(&guid, image_type_id) && index == image_index) {val = fdt_getprop(fdt, offset, "fw-version", &len);if (val)*fw_version = fdt32_to_cpu(*val);val = fdt_getprop(fdt, offset,"lowest-supported-version", &len);if (val)*lsv = fdt32_to_cpu(*val);}}+}
- /**
- efi_fill_image_desc_array - populate image descriptor array
- @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c.
Sorry for the late reply.
num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", fw_images[] array is also defined in each board file, so we can not simply move num_image_type_guids into lib/efi_loader/efi_firmware.c.
Why can't we have
int num_image_type_guids = ARRAY_SIZE(fw_images);
in lib/efi_loader/efi_firmware.c?
Best regards
Heinrich
And fw_images[] array is abstracted by struct efi_capsule_update_info, so I think we should not extract the fw_images[] array.
u32 fw_version = 0;u32 lowest_supported_version = 0;These assignments should be in efi_firmware_get_firmware_version.
OK.
image_info[i].image_index = fw_array[i].image_index;Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image.
This is derived from the UEFI specification. UEFI specification "23.1.2 EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex and ImageTypeId(guid).
ImageIndex: A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount.
image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name;
image_info[i].version = 0; /* not supported */
efi_firmware_get_firmware_version(fw_array[i].image_index,&fw_array[i].image_type_id,&fw_version,&lowest_supported_version);This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
I don't clearly understand this comment, but the expected GUID is fw_array[i].image_type_id.
image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */Please, add the missing functionality to efi_firmware_get_firmware_version().
Does it mean we need to support version_name? I can add a version_name in dtb.
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code.
OK.
Thank you for your review.
Regards, Masahisa Kojima
Best regards
Heinrich
image_info[i].size = 0; image_info[i].attributes_supported =@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
image_info[i].lowest_supported_image_version = 0;
image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1;@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name);
return EFI_EXIT(ret);
On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 5/8/23 10:15, Masahisa Kojima wrote:
Hi Heinrich,
On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/10/23 11:07, Masahisa Kojima wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/**
- efi_firmware_get_firmware_version - get firmware version from dtb
- @image_index: Image index
- @image_type_id: Image type id
- @fw_version: Pointer to store the version number
- @lsv: Pointer to store the lowest supported version
- Authenticate the capsule if authentication is enabled.
- The image pointer and the image size are updated in case of success.
- */
+void efi_firmware_get_firmware_version(u8 image_index,
efi_guid_t *image_type_id,u32 *fw_version, u32 *lsv)+{
const void *fdt = gd->fdt_blob;const fdt32_t *val;const char *guid_str;int len, offset, index;int parent;parent = fdt_subnode_offset(fdt, 0, "firmware-version");if (parent < 0)return;fdt_for_each_subnode(offset, fdt, parent) {efi_guid_t guid;guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);if (!guid_str)continue;uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);val = fdt_getprop(fdt, offset, "image-index", &len);if (!val)continue;index = fdt32_to_cpu(*val);if (!guidcmp(&guid, image_type_id) && index == image_index) {val = fdt_getprop(fdt, offset, "fw-version", &len);if (val)*fw_version = fdt32_to_cpu(*val);val = fdt_getprop(fdt, offset,"lowest-supported-version", &len);if (val)*lsv = fdt32_to_cpu(*val);}}+}
- /**
- efi_fill_image_desc_array - populate image descriptor array
- @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c.
Sorry for the late reply.
num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", fw_images[] array is also defined in each board file, so we can not simply move num_image_type_guids into lib/efi_loader/efi_firmware.c.
Why can't we have
int num_image_type_guids = ARRAY_SIZE(fw_images);
in lib/efi_loader/efi_firmware.c?
At first thought, I thought it was a matter of abstraction.
But there is a compilation error when we expose fw_images[]. fw_images[] array is initialized in each board file, and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c will cause compilation failure. We need to specify the array size when fw_images is exposed, for example: extern struct efi_fw_image fw_images[2];
but currently there is no method to pre-define the fw_images[] array size, it is board specific.
We can define the macro to indicate the array size or having sentinel in the fw_images[] array, but I think the current implementation is simpler, I would like to keep the current implementation. Correct me if I'm wrong.
Thanks, Masahisa Kojima
Best regards
Heinrich
And fw_images[] array is abstracted by struct efi_capsule_update_info, so I think we should not extract the fw_images[] array.
u32 fw_version = 0;u32 lowest_supported_version = 0;These assignments should be in efi_firmware_get_firmware_version.
OK.
image_info[i].image_index = fw_array[i].image_index;Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image.
This is derived from the UEFI specification. UEFI specification "23.1.2 EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex and ImageTypeId(guid).
ImageIndex: A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount.
image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name;
image_info[i].version = 0; /* not supported */
efi_firmware_get_firmware_version(fw_array[i].image_index,&fw_array[i].image_type_id,&fw_version,&lowest_supported_version);This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
I don't clearly understand this comment, but the expected GUID is fw_array[i].image_type_id.
image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */Please, add the missing functionality to efi_firmware_get_firmware_version().
Does it mean we need to support version_name? I can add a version_name in dtb.
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code.
OK.
Thank you for your review.
Regards, Masahisa Kojima
Best regards
Heinrich
image_info[i].size = 0; image_info[i].attributes_supported =@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
image_info[i].lowest_supported_image_version = 0;
image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1;@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name);
return EFI_EXIT(ret);
On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote:
On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 5/8/23 10:15, Masahisa Kojima wrote:
Hi Heinrich,
On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/10/23 11:07, Masahisa Kojima wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/**
- efi_firmware_get_firmware_version - get firmware version from dtb
- @image_index: Image index
- @image_type_id: Image type id
- @fw_version: Pointer to store the version number
- @lsv: Pointer to store the lowest supported version
- Authenticate the capsule if authentication is enabled.
- The image pointer and the image size are updated in case of success.
- */
+void efi_firmware_get_firmware_version(u8 image_index,
efi_guid_t *image_type_id,u32 *fw_version, u32 *lsv)+{
const void *fdt = gd->fdt_blob;const fdt32_t *val;const char *guid_str;int len, offset, index;int parent;parent = fdt_subnode_offset(fdt, 0, "firmware-version");if (parent < 0)return;fdt_for_each_subnode(offset, fdt, parent) {efi_guid_t guid;guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);if (!guid_str)continue;uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);val = fdt_getprop(fdt, offset, "image-index", &len);if (!val)continue;index = fdt32_to_cpu(*val);if (!guidcmp(&guid, image_type_id) && index == image_index) {val = fdt_getprop(fdt, offset, "fw-version", &len);if (val)*fw_version = fdt32_to_cpu(*val);val = fdt_getprop(fdt, offset,"lowest-supported-version", &len);if (val)*lsv = fdt32_to_cpu(*val);}}+}
- /**
- efi_fill_image_desc_array - populate image descriptor array
- @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c.
Sorry for the late reply.
num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", fw_images[] array is also defined in each board file, so we can not simply move num_image_type_guids into lib/efi_loader/efi_firmware.c.
Why can't we have
int num_image_type_guids = ARRAY_SIZE(fw_images);
in lib/efi_loader/efi_firmware.c?
At first thought, I thought it was a matter of abstraction.
But there is a compilation error when we expose fw_images[]. fw_images[] array is initialized in each board file, and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c will cause compilation failure. We need to specify the array size when fw_images is exposed, for example: extern struct efi_fw_image fw_images[2];
but currently there is no method to pre-define the fw_images[] array size, it is board specific.
We can define the macro to indicate the array size or having sentinel in the fw_images[] array, but I think the current
I simply wonder if the value should be embedded in "struct efi_capsule_update_info". struct efi_capsule_update_info { const char *dfu_string; int num_images; <- added struct efi_fw_image *images; }; This is the best place because the value must match not only "images" but also (entries in) "dfu_string".
Even now, efi_fill_image_desc_array() tries to access "fw_images[]" via the exposed update_info variable. Beautiful, isn't it?
One more comment: uefi.rst doesn't mention anything about num_image_type_guids.
-Takahiro Akashi
implementation is simpler, I would like to keep the current implementation. Correct me if I'm wrong.
Thanks, Masahisa Kojima
Best regards
Heinrich
And fw_images[] array is abstracted by struct efi_capsule_update_info, so I think we should not extract the fw_images[] array.
u32 fw_version = 0;u32 lowest_supported_version = 0;These assignments should be in efi_firmware_get_firmware_version.
OK.
image_info[i].image_index = fw_array[i].image_index;Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image.
This is derived from the UEFI specification. UEFI specification "23.1.2 EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex and ImageTypeId(guid).
ImageIndex: A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount.
image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name;
image_info[i].version = 0; /* not supported */
efi_firmware_get_firmware_version(fw_array[i].image_index,&fw_array[i].image_type_id,&fw_version,&lowest_supported_version);This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
I don't clearly understand this comment, but the expected GUID is fw_array[i].image_type_id.
image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */Please, add the missing functionality to efi_firmware_get_firmware_version().
Does it mean we need to support version_name? I can add a version_name in dtb.
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code.
OK.
Thank you for your review.
Regards, Masahisa Kojima
Best regards
Heinrich
image_info[i].size = 0; image_info[i].attributes_supported =@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
image_info[i].lowest_supported_image_version = 0;
image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1;@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name);
return EFI_EXIT(ret);
Hi Akashi-san,
On Wed, 10 May 2023 at 09:28, Takahiro Akashi takahiro.akashi@linaro.org wrote:
On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote:
On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 5/8/23 10:15, Masahisa Kojima wrote:
Hi Heinrich,
On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/10/23 11:07, Masahisa Kojima wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); }
+/**
- efi_firmware_get_firmware_version - get firmware version from dtb
- @image_index: Image index
- @image_type_id: Image type id
- @fw_version: Pointer to store the version number
- @lsv: Pointer to store the lowest supported version
- Authenticate the capsule if authentication is enabled.
- The image pointer and the image size are updated in case of success.
- */
+void efi_firmware_get_firmware_version(u8 image_index,
efi_guid_t *image_type_id,u32 *fw_version, u32 *lsv)+{
const void *fdt = gd->fdt_blob;const fdt32_t *val;const char *guid_str;int len, offset, index;int parent;parent = fdt_subnode_offset(fdt, 0, "firmware-version");if (parent < 0)return;fdt_for_each_subnode(offset, fdt, parent) {efi_guid_t guid;guid_str = fdt_getprop(fdt, offset, "image-type-id", &len);if (!guid_str)continue;uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);val = fdt_getprop(fdt, offset, "image-index", &len);if (!val)continue;index = fdt32_to_cpu(*val);if (!guidcmp(&guid, image_type_id) && index == image_index) {val = fdt_getprop(fdt, offset, "fw-version", &len);if (val)*fw_version = fdt32_to_cpu(*val);val = fdt_getprop(fdt, offset,"lowest-supported-version", &len);if (val)*lsv = fdt32_to_cpu(*val);}}+}
- /**
- efi_fill_image_desc_array - populate image descriptor array
- @image_info_size: Size of @image_info
@@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */
for (i = 0; i < num_image_type_guids; i++) {Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c.
Sorry for the late reply.
num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", fw_images[] array is also defined in each board file, so we can not simply move num_image_type_guids into lib/efi_loader/efi_firmware.c.
Why can't we have
int num_image_type_guids = ARRAY_SIZE(fw_images);
in lib/efi_loader/efi_firmware.c?
At first thought, I thought it was a matter of abstraction.
But there is a compilation error when we expose fw_images[]. fw_images[] array is initialized in each board file, and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c will cause compilation failure. We need to specify the array size when fw_images is exposed, for example: extern struct efi_fw_image fw_images[2];
but currently there is no method to pre-define the fw_images[] array size, it is board specific.
We can define the macro to indicate the array size or having sentinel in the fw_images[] array, but I think the current
I simply wonder if the value should be embedded in "struct efi_capsule_update_info". struct efi_capsule_update_info { const char *dfu_string; int num_images; <- added struct efi_fw_image *images; };
Thank you, I agree with this.
This is the best place because the value must match not only "images" but also (entries in) "dfu_string".
Even now, efi_fill_image_desc_array() tries to access "fw_images[]" via the exposed update_info variable. Beautiful, isn't it?
One more comment: uefi.rst doesn't mention anything about num_image_type_guids.
I will also update uefi.rst.
Regards, Masahisa Kojima
-Takahiro Akashi
implementation is simpler, I would like to keep the current implementation. Correct me if I'm wrong.
Thanks, Masahisa Kojima
Best regards
Heinrich
And fw_images[] array is abstracted by struct efi_capsule_update_info, so I think we should not extract the fw_images[] array.
u32 fw_version = 0;u32 lowest_supported_version = 0;These assignments should be in efi_firmware_get_firmware_version.
OK.
image_info[i].image_index = fw_array[i].image_index;Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image.
This is derived from the UEFI specification. UEFI specification "23.1.2 EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex and ImageTypeId(guid).
ImageIndex: A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount.
image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name;
image_info[i].version = 0; /* not supported */
efi_firmware_get_firmware_version(fw_array[i].image_index,&fw_array[i].image_type_id,&fw_version,&lowest_supported_version);This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule.
So you must pass the expected GUID as input variable here.
I don't clearly understand this comment, but the expected GUID is fw_array[i].image_type_id.
image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */Please, add the missing functionality to efi_firmware_get_firmware_version().
Does it mean we need to support version_name? I can add a version_name in dtb.
Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code.
OK.
Thank you for your review.
Regards, Masahisa Kojima
Best regards
Heinrich
image_info[i].size = 0; image_info[i].attributes_supported =@@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
image_info[i].lowest_supported_image_version = 0;
image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1;@@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name);
return EFI_EXIT(ret);
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
Nit: please use lower-case hex and add a decoder to uuid.c so we can look it up when debugging.
image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};
[..]
Regards, Simon
Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";Nit: please use lower-case hex and add a decoder to uuid.c so we can look it up when debugging.
The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere.
Instead we should define a string per firmware image. We already have a field for it:
fw_array[i].fw_name
Best regards
Heinrich
image-index = <1>;fw-version = <5>;lowest-supported-version = <3>;};image2 {image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0";image-index = <2>;fw-version = <10>;lowest-supported-version = <7>;};};[..]
Regards, Simon
Hi Heinrich,
On Thu, 27 Apr 2023 at 22:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";Nit: please use lower-case hex and add a decoder to uuid.c so we can look it up when debugging.
The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere.
Instead we should define a string per firmware image. We already have a field for it:
fw_array[i].fw_name
This is of course madness. NAK to any UUIDs in U-Boot that don't have a decoder ring.
Let's use a compatible string (vendor,board) and drop these silly UUIDs.
Regards, Simon
On 5/10/23 22:46, Simon Glass wrote:
Hi Heinrich,
On Thu, 27 Apr 2023 at 22:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface.
This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo().
Now FMP->GetImageInfo() and ESRT have the meaningful version number.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
newly implement a device tree based versioning
.../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +-------------------------------
+Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version
+Example:
firmware-version {image1 {image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";Nit: please use lower-case hex and add a decoder to uuid.c so we can look it up when debugging.
The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere.
Instead we should define a string per firmware image. We already have a field for it:
fw_array[i].fw_name
This is of course madness. NAK to any UUIDs in U-Boot that don't have a decoder ring.
I never have heard of a "decoder ring" for UUIDs in U-Boot.
git grep -A2 -ni decoder | grep -i ring
does not find such a term.
Our development target is to keep the code size of U-Boot small. Hence, we should not clutter uuid.c with board specific strings. These should be kept in board specific files.
Let's use a compatible string (vendor,board) and drop these silly UUIDs.
Please, avoid derogatory language like "silly".
The UEFI specification defines EFI_CAPSULE_HEADER. This structure contains field CapusuleGuid (A GUID that defines the contents of a capsule).
Best regards
Heinrich
The FMP Payload Header which EDK II capsule generation scripts insert has a firmware version. This commit reads the lowest supported version stored in the device tree, then check if the firmware version in FMP payload header of the ongoing capsule is equal or greater than the lowest supported version. If the firmware version is lower than lowest supported version, capsule update fails.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- Changes in v5: - newly implement the device tree based versioning
Changes in v4: - use log_err() instead of printf()
Changes in v2: - add error message when the firmware version is lower than lowest supported version
lib/efi_loader/efi_firmware.c | 124 ++++++++++++++++++++++++++++------ 1 file changed, 103 insertions(+), 21 deletions(-)
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 1c6ef468bf..c88c1bb364 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -33,7 +33,7 @@ struct fmp_payload_header { u32 signature; u32 header_size; u32 fw_version; - u32 lowest_supported_version; + u32 lowest_supported_version; /* not used */ };
__weak void set_dfu_alt_info(char *interface, char *devstr) @@ -250,8 +250,6 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image, { const void *image = *p_image; efi_uintn_t image_size = *p_image_size; - u32 fmp_hdr_signature; - struct fmp_payload_header *header; void *capsule_payload; efi_status_t status; efi_uintn_t capsule_payload_size; @@ -264,7 +262,7 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image, &capsule_payload_size);
if (status == EFI_SECURITY_VIOLATION) { - printf("Capsule authentication check failed. Aborting update\n"); + log_err("Capsule authentication check failed. Aborting update\n"); return status; } else if (status != EFI_SUCCESS) { return status; @@ -278,21 +276,6 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image, debug("Updating capsule without authenticating.\n"); }
- fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE; - header = (void *)image; - - if (!memcmp(&header->signature, &fmp_hdr_signature, - sizeof(fmp_hdr_signature))) { - /* - * When building the capsule with the scripts in - * edk2, a FMP header is inserted above the capsule - * payload. Compensate for this header to get the - * actual payload that is to be updated. - */ - image += header->header_size; - image_size -= header->header_size; - } - *p_image = image; *p_image_size = image_size; return EFI_SUCCESS; @@ -349,6 +332,105 @@ efi_status_t EFIAPI efi_firmware_get_image_info( return EFI_EXIT(ret); }
+/** + * efi_firmware_get_image_type_id - get image_type_id + * @image_index: image index + * + * Return the image_type_id identified by the image index. + * + * Return: pointer to the image_type_id, NULL if image_index is invalid + */ +static +efi_guid_t *efi_firmware_get_image_type_id(u8 image_index) +{ + int i; + struct efi_fw_image *fw_array; + + fw_array = update_info.images; + for (i = 0; i < num_image_type_guids; i++) { + if (fw_array[i].image_index == image_index) + return &fw_array[i].image_type_id; + } + + return NULL; +} + +/** + * efi_firmware_check_lowest_supported_version - check the lowest supported version + * @p_image: Pointer to new image + * @p_image_size: Pointer to size of new image + * @image_index: image_index + * + * Check the fw_version in FMP payload header is equal to or greather than the + * lowest_supported_version stored in the device tree + * + * Return: status code + */ +static +efi_status_t efi_firmware_check_lowest_supported_version(const void **p_image, + efi_uintn_t *p_image_size, + u8 image_index) +{ + const void *image = *p_image; + efi_uintn_t image_size = *p_image_size; + const struct fmp_payload_header *header; + u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE; + + /* FMP header is inserted above the capsule payload */ + header = image; + if (header->signature == fmp_hdr_signature) { + efi_guid_t *image_type_id; + u32 version = 0, lsv = 0; + + image_type_id = efi_firmware_get_image_type_id(image_index); + if (!image_type_id) + return EFI_INVALID_PARAMETER; + + efi_firmware_get_firmware_version(image_index, image_type_id, + &version, &lsv); + + if (header->fw_version >= lsv) { + image += header->header_size; + image_size -= header->header_size; + } else { + log_err("Firmware version %u too low. Expecting >= %u. Aborting update\n", + header->fw_version, lsv); + return EFI_INCOMPATIBLE_VERSION; + } + } + + *p_image = image; + *p_image_size = image_size; + + return EFI_SUCCESS; +} + +/** + * efi_firmware_verify_image - verify image + * @p_image: Pointer to new image + * @p_image_size: Pointer to size of new image + * @image_index Image index + * + * Verify the capsule file + * + * Return: status code + */ +static +efi_status_t efi_firmware_verify_image(const void **p_image, + efi_uintn_t *p_image_size, + u8 image_index) +{ + efi_status_t ret; + + ret = efi_firmware_capsule_authenticate(p_image, p_image_size); + if (ret != EFI_SUCCESS) + return ret; + + ret = efi_firmware_check_lowest_supported_version(p_image, p_image_size, image_index); + + return ret; +} + #ifdef CONFIG_EFI_CAPSULE_FIRMWARE_FIT /* * This FIRMWARE_MANAGEMENT_PROTOCOL driver provides a firmware update @@ -393,7 +475,7 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( if (!image || image_index != 1) return EFI_EXIT(EFI_INVALID_PARAMETER);
- status = efi_firmware_capsule_authenticate(&image, &image_size); + status = efi_firmware_verify_image(&image, &image_size, image_index); if (status != EFI_SUCCESS) return EFI_EXIT(status);
@@ -454,7 +536,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( if (!image) return EFI_EXIT(EFI_INVALID_PARAMETER);
- status = efi_firmware_capsule_authenticate(&image, &image_size); + status = efi_firmware_verify_image(&image, &image_size, image_index); if (status != EFI_SUCCESS) return EFI_EXIT(status);
Current mkeficapsule tool does not provide firmware version management. EDK II reference implementation inserts the FMP Payload Header right before the payload. It coutains the fw_version and lowest supported version.
This commit adds a new parameters required to generate the FMP Payload Header for mkeficapsule tool. '-v' indicates the firmware version.
When mkeficapsule tool is invoked without '-v' option, FMP Payload Header is not inserted, the behavior is same as current implementation.
The lowest supported version included in the FMP Payload Header is not used in the current versioning support, the value stored in the device tree is used.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- Changes in v5: - remove --lsv since we use the lowest_supported_version in the dtb
Changes in v3: - remove '-f' option - move some definitions into tools/eficapsule.h - add dependency check of fw_version and lowest_supported_version - remove unexpected modification of existing fprintf() call - add documentation
Newly created in v2
doc/mkeficapsule.1 | 10 ++++++++++ tools/eficapsule.h | 30 ++++++++++++++++++++++++++++++ tools/mkeficapsule.c | 37 +++++++++++++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 index 1ca245a10f..c4c2057d5c 100644 --- a/doc/mkeficapsule.1 +++ b/doc/mkeficapsule.1 @@ -61,6 +61,16 @@ Specify an image index .BI "-I\fR,\fB --instance " instance Specify a hardware instance
+.PP +FMP Payload Header is inserted right before the payload if +.BR --fw-version +is specified + + +.TP +.BI "-v\fR,\fB --fw-version " firmware-version +Specify a firmware version, 0 if omitted + .PP For generation of firmware accept empty capsule .BR --guid diff --git a/tools/eficapsule.h b/tools/eficapsule.h index 072a4b5598..753fb73313 100644 --- a/tools/eficapsule.h +++ b/tools/eficapsule.h @@ -113,4 +113,34 @@ struct efi_firmware_image_authentication { struct win_certificate_uefi_guid auth_info; } __packed;
+/* fmp payload header */ +#define SIGNATURE_16(A, B) ((A) | ((B) << 8)) +#define SIGNATURE_32(A, B, C, D) \ + (SIGNATURE_16(A, B) | (SIGNATURE_16(C, D) << 16)) + +#define FMP_PAYLOAD_HDR_SIGNATURE SIGNATURE_32('M', 'S', 'S', '1') + +/** + * struct fmp_payload_header - EDK2 header for the FMP payload + * + * This structure describes the header which is preprended to the + * FMP payload by the edk2 capsule generation scripts. + * + * @signature: Header signature used to identify the header + * @header_size: Size of the structure + * @fw_version: Firmware versions used + * @lowest_supported_version: Lowest supported version (not used) + */ +struct fmp_payload_header { + uint32_t signature; + uint32_t header_size; + uint32_t fw_version; + uint32_t lowest_supported_version; +}; + +struct fmp_payload_header_params { + bool have_header; + uint32_t fw_version; +}; + #endif /* _EFI_CAPSULE_H */ diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index b71537beee..52be1f122e 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -41,6 +41,7 @@ static struct option options[] = { {"guid", required_argument, NULL, 'g'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'}, + {"fw-version", required_argument, NULL, 'v'}, {"private-key", required_argument, NULL, 'p'}, {"certificate", required_argument, NULL, 'c'}, {"monotonic-count", required_argument, NULL, 'm'}, @@ -60,6 +61,7 @@ static void print_usage(void) "\t-g, --guid <guid string> guid for image blob type\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n" + "\t-v, --fw-version <version> firmware version\n" "\t-p, --private-key <privkey file> private key file\n" "\t-c, --certificate <cert file> signer's certificate file\n" "\t-m, --monotonic-count <count> monotonic count\n" @@ -402,6 +404,7 @@ static void free_sig_data(struct auth_context *ctx) */ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, unsigned long index, unsigned long instance, + struct fmp_payload_header_params *fmp_ph_params, uint64_t mcount, char *privkey_file, char *cert_file, uint16_t oemflags) { @@ -410,10 +413,11 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, struct efi_firmware_management_capsule_image_header image; struct auth_context auth_context; FILE *f; - uint8_t *data; + uint8_t *data, *new_data, *buf; off_t bin_size; uint64_t offset; int ret; + struct fmp_payload_header payload_header;
#ifdef DEBUG fprintf(stderr, "For output: %s\n", path); @@ -423,6 +427,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, auth_context.sig_size = 0; f = NULL; data = NULL; + new_data = NULL; ret = -1;
/* @@ -431,12 +436,30 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, if (read_bin_file(bin, &data, &bin_size)) goto err;
+ buf = data; + + /* insert fmp payload header right before the payload */ + if (fmp_ph_params->have_header) { + new_data = malloc(bin_size + sizeof(payload_header)); + if (!new_data) + goto err; + + payload_header.signature = FMP_PAYLOAD_HDR_SIGNATURE; + payload_header.header_size = sizeof(payload_header); + payload_header.fw_version = fmp_ph_params->fw_version; + payload_header.lowest_supported_version = 0; /* not used */ + memcpy(new_data, &payload_header, sizeof(payload_header)); + memcpy(new_data + sizeof(payload_header), data, bin_size); + buf = new_data; + bin_size += sizeof(payload_header); + } + /* first, calculate signature to determine its size */ if (privkey_file && cert_file) { auth_context.key_file = privkey_file; auth_context.cert_file = cert_file; auth_context.auth.monotonic_count = mcount; - auth_context.image_data = data; + auth_context.image_data = buf; auth_context.image_size = bin_size;
if (create_auth_data(&auth_context)) { @@ -536,7 +559,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, /* * firmware binary */ - if (write_capsule_file(f, data, bin_size, "Firmware binary")) + if (write_capsule_file(f, buf, bin_size, "Firmware binary")) goto err;
ret = 0; @@ -545,6 +568,7 @@ err: fclose(f); free_sig_data(&auth_context); free(data); + free(new_data);
return ret; } @@ -644,6 +668,7 @@ int main(int argc, char **argv) unsigned long oemflags; char *privkey_file, *cert_file; int c, idx; + struct fmp_payload_header_params fmp_ph_params = { 0 };
guid = NULL; index = 0; @@ -679,6 +704,10 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break; + case 'v': + fmp_ph_params.fw_version = strtoul(optarg, NULL, 0); + fmp_ph_params.have_header = true; + break; case 'p': if (privkey_file) { fprintf(stderr, @@ -751,7 +780,7 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, - index, instance, mcount, privkey_file, + index, instance, &fmp_ph_params, mcount, privkey_file, cert_file, (uint16_t)oemflags) < 0) { fprintf(stderr, "Creating firmware capsule failed\n"); exit(EXIT_FAILURE);
This test covers FMP versioning for both raw and FIT image, and both signed and non-signed capsule update.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- Changes in v5: - get aligned to the device tree based versioning
Newly created in v4
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ 6 files changed, 822 insertions(+) create mode 100644 test/py/tests/test_efi_capsule/version.dts
diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py index 4879f2b5c2..9655e5ec1f 100644 --- a/test/py/tests/test_efi_capsule/conftest.py +++ b/test/py/tests/test_efi_capsule/conftest.py @@ -70,6 +70,23 @@ def efi_capsule_data(request, u_boot_config): '-out SIGNER2.crt -nodes -days 365' % data_dir, shell=True)
+ # Update dtb adding version information + check_call('cd %s; ' + 'cp %s/test/py/tests/test_efi_capsule/version.dts .' + % (data_dir, u_boot_config.source_dir), shell=True) + if capsule_auth_enabled: + check_call('cd %s; ' + 'dtc -@ -I dts -O dtb -o version.dtbo version.dts; ' + 'fdtoverlay -i test_sig.dtb ' + '-o test_ver.dtb version.dtbo' + % (data_dir), shell=True) + else: + check_call('cd %s; ' + 'dtc -@ -I dts -O dtb -o version.dtbo version.dts; ' + 'fdtoverlay -i %s/arch/sandbox/dts/test.dtb ' + '-o test_ver.dtb version.dtbo' + % (data_dir, u_boot_config.build_dir), shell=True) + # Create capsule files # two regions: one for u-boot.bin and the other for u-boot.env check_call('cd %s; echo -n u-boot:Old > u-boot.bin.old; echo -n u-boot:New > u-boot.bin.new; echo -n u-boot-env:Old > u-boot.env.old; echo -n u-boot-env:New > u-boot.env.new' % data_dir, @@ -95,6 +112,26 @@ def efi_capsule_data(request, u_boot_config): check_call('cd %s; %s/tools/mkeficapsule --index 1 --guid 058B7D83-50D5-4C47-A195-60D86AD341C4 uboot_bin_env.itb Test05' % (data_dir, u_boot_config.build_dir), shell=True) + check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 5 ' + '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 u-boot.bin.new Test101' % + (data_dir, u_boot_config.build_dir), + shell=True) + check_call('cd %s; %s/tools/mkeficapsule --index 2 --fw-version 10 ' + '--guid 5A7021F5-FEF2-48B4-AABA-832E777418C0 u-boot.env.new Test102' % + (data_dir, u_boot_config.build_dir), + shell=True) + check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 2 ' + '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 u-boot.bin.new Test103' % + (data_dir, u_boot_config.build_dir), + shell=True) + check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 5 ' + '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 uboot_bin_env.itb Test104' % + (data_dir, u_boot_config.build_dir), + shell=True) + check_call('cd %s; %s/tools/mkeficapsule --index 1 --fw-version 2 ' + '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 uboot_bin_env.itb Test105' % + (data_dir, u_boot_config.build_dir), + shell=True)
if capsule_auth_enabled: # raw firmware signed with proper key @@ -131,6 +168,42 @@ def efi_capsule_data(request, u_boot_config): 'uboot_bin_env.itb Test14' % (data_dir, u_boot_config.build_dir), shell=True) + # raw firmware signed with proper key with version information + check_call('cd %s; ' + '%s/tools/mkeficapsule --index 1 --monotonic-count 1 ' + '--fw-version 5 ' + '--private-key SIGNER.key --certificate SIGNER.crt ' + '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 ' + 'u-boot.bin.new Test111' + % (data_dir, u_boot_config.build_dir), + shell=True) + # raw firmware signed with *mal* key with version information + check_call('cd %s; ' + '%s/tools/mkeficapsule --index 1 --monotonic-count 1 ' + '--fw-version 2 ' + '--private-key SIGNER.key --certificate SIGNER.crt ' + '--guid 09D7CF52-0720-4710-91D1-08469B7FE9C8 ' + 'u-boot.bin.new Test112' + % (data_dir, u_boot_config.build_dir), + shell=True) + # FIT firmware signed with proper key with version information + check_call('cd %s; ' + '%s/tools/mkeficapsule --index 1 --monotonic-count 1 ' + '--fw-version 5 ' + '--private-key SIGNER.key --certificate SIGNER.crt ' + '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 ' + 'uboot_bin_env.itb Test113' + % (data_dir, u_boot_config.build_dir), + shell=True) + # FIT firmware signed with *mal* key with version information + check_call('cd %s; ' + '%s/tools/mkeficapsule --index 1 --monotonic-count 1 ' + '--fw-version 2 ' + '--private-key SIGNER.key --certificate SIGNER.crt ' + '--guid 3673B45D-6A7C-46F3-9E60-ADABB03F7937 ' + 'uboot_bin_env.itb Test114' + % (data_dir, u_boot_config.build_dir), + shell=True)
# Create a disk image with EFI system partition check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat %s %s' % diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py index d28b53a1a1..e774a06d10 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py @@ -189,3 +189,190 @@ class TestEfiCapsuleFirmwareFit(object): assert 'u-boot-env:Old' in ''.join(output) else: assert 'u-boot-env:New' in ''.join(output) + + def test_efi_capsule_fw3( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 3 - Update U-Boot and U-Boot environment on SPI Flash with version information + 0x100000-0x150000: U-Boot binary (but dummy) + 0x150000-0x200000: U-Boot environment (but dummy) + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 3-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize contents + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.env.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 150000 10', + 'sf read 5000000 150000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test104' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test104 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test104' in ''.join(output) + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot(expect_reset = capsule_early) + + with u_boot_console.log.section('Test Case 3-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test104' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test104' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + if capsule_auth: + assert 'u-boot:Old' in ''.join(output) + else: + assert 'u-boot:New' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf read 4000000 150000 10', + 'md.b 4000000 10']) + if capsule_auth: + assert 'u-boot-env:Old' in ''.join(output) + else: + assert 'u-boot-env:New' in ''.join(output) + + def test_efi_capsule_fw4( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 4 - Update U-Boot and U-Boot environment on SPI Flash with version information + but fw_version is lower than lowest_supported_version + No update should happen + 0x100000-0x150000: U-Boot binary (but dummy) + 0x150000-0x200000: U-Boot environment (but dummy) + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 4-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize contents + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.env.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 150000 10', + 'sf read 5000000 150000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test105' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test105 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test105' in ''.join(output) + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot(expect_reset = capsule_early) + + with u_boot_console.log.section('Test Case 4-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test105' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test105' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + # make sure capsule update failed + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:Old' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf read 4000000 150000 10', + 'md.b 4000000 10']) + assert 'u-boot-env:Old' in ''.join(output) diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py index 92bfb14932..bd6419bcc3 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py @@ -292,3 +292,204 @@ class TestEfiCapsuleFirmwareRaw: assert 'u-boot-env:Old' in ''.join(output) else: assert 'u-boot-env:New' in ''.join(output) + + def test_efi_capsule_fw4( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ Test Case 4 + Update U-Boot on SPI Flash, raw image format with fw_version and lowest_supported_version + 0x100000-0x150000: U-Boot binary (but dummy) + 0x150000-0x200000: U-Boot environment (but dummy) + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 4-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize contents + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.env.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 150000 10', + 'sf read 5000000 150000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place the capsule files + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test101' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test101 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test101' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test102' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test102 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test102' in ''.join(output) + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot(expect_reset = capsule_early) + + with u_boot_console.log.section('Test Case 4-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test101' in ''.join(output) + assert 'Test102' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test101' not in ''.join(output) + assert 'Test102' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + + # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT. + assert '09D7CF52-0720-4710-91D1-08469B7FE9C8' in ''.join(output) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT. + assert '5A7021F5-FEF2-48B4-AABA-832E777418C0' in ''.join(output) + assert 'ESRT: fw_version=10' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=7' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + if capsule_auth: + assert 'u-boot:Old' in ''.join(output) + else: + assert 'u-boot:New' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 150000 10', + 'md.b 4000000 10']) + + if capsule_auth: + assert 'u-boot-env:Old' in ''.join(output) + else: + assert 'u-boot-env:New' in ''.join(output) + + def test_efi_capsule_fw5( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ Test Case 5 + Update U-Boot on SPI Flash, raw image format with fw_version and lowest_supported_version + but fw_version is lower than lowest_supported_version + No update should happen + 0x100000-0x150000: U-Boot binary (but dummy) + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 5-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi -s ""', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize contents + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place the capsule files + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test103' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test103 $filesize' % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test103' in ''.join(output) + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + capsule_auth = u_boot_config.buildconfig.get( + 'config_efi_capsule_authenticate') + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot(expect_reset = capsule_early) + + with u_boot_console.log.section('Test Case 5-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test103' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test103' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + + # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT. + assert '09D7CF52-0720-4710-91D1-08469B7FE9C8' in ''.join(output) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT. + assert '5A7021F5-FEF2-48B4-AABA-832E777418C0' in ''.join(output) + assert 'ESRT: fw_version=10' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=7' in ''.join(output) + + # make sure capsule update failed + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:Old' in ''.join(output) diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py index 8c2d616fd0..4696ad962a 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py @@ -257,3 +257,168 @@ class TestEfiCapsuleFirmwareSignedFit(object): 'sf read 4000000 100000 10', 'md.b 4000000 10']) assert 'u-boot:Old' in ''.join(output) + + def test_efi_capsule_auth4( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 4 - Update U-Boot on SPI Flash, FIT image format + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is properly signed with version information, + the authentication should pass and the firmware be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 4-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' + % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test113' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test113 $filesize' + % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test113' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 4-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test113' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test113' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:New' in ''.join(output) + + def test_efi_capsule_auth5( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 5 - Update U-Boot on SPI Flash, FIT image format + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is signed but fw_version is lower than lowest + supported version, the authentication should fail and the firmware + not be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 5-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' + % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test114' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test114 $filesize' + % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test114' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 5-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test114' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test114' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:Old' in ''.join(output) diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py index 2bbaa9cc55..98ce9c9afd 100644 --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py @@ -254,3 +254,172 @@ class TestEfiCapsuleFirmwareSignedRaw(object): 'sf read 4000000 100000 10', 'md.b 4000000 10']) assert 'u-boot:Old' in ''.join(output) + + def test_efi_capsule_auth4( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 4 - Update U-Boot on SPI Flash, raw image format with version information + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is properly signed, the authentication + should pass and the firmware be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 4-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + + output = u_boot_console.run_command('efidebug capsule esrt') + + + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' + % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test111' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test111 $filesize' + % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test111' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 4-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test111' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test111' not in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:New' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + def test_efi_capsule_auth5( + self, u_boot_config, u_boot_console, efi_capsule_data): + """ + Test Case 5 - Update U-Boot on SPI Flash, raw image format with version information + 0x100000-0x150000: U-Boot binary (but dummy) + + If the capsule is signed but fw_version is lower than lowest + supported version, the authentication should fail and the firmware + not be updated. + """ + disk_img = efi_capsule_data + with u_boot_console.log.section('Test Case 5-a, before reboot'): + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'printenv -e PlatformLangCodes', # workaround for terminal size determination + 'efidebug boot add -b 1 TEST host 0:1 /helloworld.efi', + 'efidebug boot order 1', + 'env set -e -nv -bs -rt OsIndications =0x0000000000000004', + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'env save']) + + # initialize content + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'fatload host 0:1 4000000 %s/u-boot.bin.old' + % CAPSULE_DATA_DIR, + 'sf write 4000000 100000 10', + 'sf read 5000000 100000 10', + 'md.b 5000000 10']) + assert 'Old' in ''.join(output) + + # place a capsule file + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 %s/Test112' % CAPSULE_DATA_DIR, + 'fatwrite host 0:1 4000000 %s/Test112 $filesize' + % CAPSULE_INSTALL_DIR, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test112' in ''.join(output) + + # reboot + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule' + u_boot_console.config.dtb = mnt_point + CAPSULE_DATA_DIR \ + + '/test_ver.dtb' + u_boot_console.restart_uboot() + + capsule_early = u_boot_config.buildconfig.get( + 'config_efi_capsule_on_disk_early') + with u_boot_console.log.section('Test Case 5-b, after reboot'): + if not capsule_early: + # make sure that dfu_alt_info exists even persistent variables + # are not available. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info ' + '"sf 0:0=u-boot-bin raw 0x100000 ' + '0x50000;u-boot-env raw 0x150000 0x200000"', + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test112' in ''.join(output) + + # need to run uefi command to initiate capsule handling + output = u_boot_console.run_command( + 'env print -e Capsule0000', wait_for_reboot = True) + + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR]) + assert 'Test112' not in ''.join(output) + + # make sure the dfu_alt_info exists because it is required for making ESRT. + output = u_boot_console.run_command_list([ + 'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"', + 'efidebug capsule esrt']) + assert 'ESRT: fw_version=5' in ''.join(output) + assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output) + + output = u_boot_console.run_command_list([ + 'sf probe 0:0', + 'sf read 4000000 100000 10', + 'md.b 4000000 10']) + assert 'u-boot:Old' in ''.join(output) diff --git a/test/py/tests/test_efi_capsule/version.dts b/test/py/tests/test_efi_capsule/version.dts new file mode 100644 index 0000000000..0e544524e3 --- /dev/null +++ b/test/py/tests/test_efi_capsule/version.dts @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; +/plugin/; + +&{/} { + firmware-version { + image1 { + lowest-supported-version = <3>; + fw-version = <5>; + image-index = <1>; + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + }; + image2 { + lowest-supported-version = <7>; + fw-version = <10>; + image-index = <2>; + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; + }; + image3 { + lowest-supported-version = <3>; + fw-version = <5>; + image-index = <1>; + image-type-id = "3673B45D-6A7C-46F3-9E60-ADABB03F7937"; + }; + }; +};
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
This test covers FMP versioning for both raw and FIT image, and both signed and non-signed capsule update.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- get aligned to the device tree based versioning
Newly created in v4
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ 6 files changed, 822 insertions(+) create mode 100644 test/py/tests/test_efi_capsule/version.dts
I hate to say this, but we must fix the reboot stuff here before adding more code.
The test needs to tell U-Boot to do the update (e.g. via a command), then continue. It should not reboot sandbox.I have said this before but the feedback was not understood or taken. If you need help designing this, please let me know. In fact I am happy to create a patch for one of the tests if that is what it takes to convey this.
Also, while it is normal to have a fair bit of duplication in tests, just to make them easier to read, it does make them hard to maintain, and the duplication here seems very large.
We have the same code repeated but with different code style or indentation. Sorry, but this is not setting us up for the future, in terms of maintaining this code. Just search for 'env set -e -nv -bs -rt OsIndications =0x0000000000000004' for example.
The common code needs to go in functions in a separate Python file, as we have done for other tests. The goal should be to use the same code for the same functions. Sure there will be some duplication, but it is too much.
These fixes should happen before we accept any more non=trivial patches to this code.
BTW I do wonder whether the test would be better written mostly in C, since from what I can see, it mostly just runs commands. You can still have a Python wrapper - see for example how test_vbe works. It uses 'ut xxx -f' to execute the C part of a Python test. You can do setup in Python, then run the C test. What do you think?
Regards, Simon
Hi Simon,
On Wed, 19 Apr 2023 at 10:47, Simon Glass sjg@chromium.org wrote:
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
This test covers FMP versioning for both raw and FIT image, and both signed and non-signed capsule update.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- get aligned to the device tree based versioning
Newly created in v4
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ 6 files changed, 822 insertions(+) create mode 100644 test/py/tests/test_efi_capsule/version.dts
I hate to say this, but we must fix the reboot stuff here before adding more code.
I read the previous discussion. https://lore.kernel.org/u-boot/164388018493.446835.11931101380744085380.stgi...
The test needs to tell U-Boot to do the update (e.g. via a command), then continue. It should not reboot sandbox.I have said this before but the feedback was not understood or taken. If you need help designing this, please let me know. In fact I am happy to create a patch for one of the tests if that is what it takes to convey this.
Current efi capsule update test tries to verify the following behavior stated in the UEFI spec v2.10, so the reboot is required to trigger the capsule update.
=== quote from UEFI v2.10 P.243 8.5.5 Delivery of Capsules via file on Mass Storage Device
As an alternative to the UpdateCapsule() runtime API, capsules of any type supported by platform may also be delivered to firmware via a file within the EFI system partition on the mass storage device targeted for boot. Capsules staged using this method are processed on the next system restart. This method is only available when booting from mass storage devices which are formatted with GPT and contain an EFI System Partition in the device image. System firmware will search for capsule when EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED bit in OsIndications is set as described in Exchanging information between the OS and Firmware. ===
Also, while it is normal to have a fair bit of duplication in tests, just to make them easier to read, it does make them hard to m
aintain,
and the duplication here seems very large.
We have the same code repeated but with different code style or indentation. Sorry, but this is not setting us up for the future, in terms of maintaining this code. Just search for 'env set -e -nv -bs -rt OsIndications =0x0000000000000004' for example.
The common code needs to go in functions in a separate Python file, as we have done for other tests. The goal should be to use the same code for the same functions. Sure there will be some duplication, but it is too much.
These fixes should happen before we accept any more non=trivial patches to this code.
I agree, I will try to create common functions to reduce code duplication.
BTW I do wonder whether the test would be better written mostly in C, since from what I can see, it mostly just runs commands. You can still have a Python wrapper - see for example how test_vbe works. It uses 'ut xxx -f' to execute the C part of a Python test. You can do setup in Python, then run the C test. What do you think?
It is just my opinion, if the test requires some setup in Python, I like Python to write the test cases as far as tests can be implemented in Python, because we can maintain the test code in one place.
Thanks, Masahisa Kojima
Regards, Simon
Hi Masahisa,
On Wed, 19 Apr 2023 at 23:17, Masahisa Kojima masahisa.kojima@linaro.org wrote:
Hi Simon,
On Wed, 19 Apr 2023 at 10:47, Simon Glass sjg@chromium.org wrote:
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
This test covers FMP versioning for both raw and FIT image, and both signed and non-signed capsule update.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- get aligned to the device tree based versioning
Newly created in v4
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ 6 files changed, 822 insertions(+) create mode 100644 test/py/tests/test_efi_capsule/version.dts
I hate to say this, but we must fix the reboot stuff here before adding more code.
I read the previous discussion. https://lore.kernel.org/u-boot/164388018493.446835.11931101380744085380.stgi...
The test needs to tell U-Boot to do the update (e.g. via a command), then continue. It should not reboot sandbox.I have said this before but the feedback was not understood or taken. If you need help designing this, please let me know. In fact I am happy to create a patch for one of the tests if that is what it takes to convey this.
Current efi capsule update test tries to verify the following behavior stated in the UEFI spec v2.10, so the reboot is required to trigger the capsule update.
=== quote from UEFI v2.10 P.243 8.5.5 Delivery of Capsules via file on Mass Storage Device
As an alternative to the UpdateCapsule() runtime API, capsules of any type supported by platform may also be delivered to firmware via a file within the EFI system partition on the mass storage device targeted for boot. Capsules staged using this method are processed on the next system restart. This method is only available when booting from mass storage devices which are formatted with GPT and contain an EFI System Partition in the device image. System firmware will search for capsule when EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED bit in OsIndications is set as described in Exchanging information between the OS and Firmware. ===
Let's agree for the moment that the spec actually requires a reboot to initial the update. For a test, we can split it into three parts:
- setting up the update - triggering the update - processing the update
We can write separate tests for each of these:
- test that the update is set up correctly - test that the update is triggered on a reboot, if one is present - test that the update is processed correctly
There is no need to bring them all together.
Also, while it is normal to have a fair bit of duplication in tests, just to make them easier to read, it does make them hard to m
aintain,
and the duplication here seems very large.
We have the same code repeated but with different code style or indentation. Sorry, but this is not setting us up for the future, in terms of maintaining this code. Just search for 'env set -e -nv -bs -rt OsIndications =0x0000000000000004' for example.
The common code needs to go in functions in a separate Python file, as we have done for other tests. The goal should be to use the same code for the same functions. Sure there will be some duplication, but it is too much.
These fixes should happen before we accept any more non=trivial patches to this code.
I agree, I will try to create common functions to reduce code duplication.
OK great.
BTW I do wonder whether the test would be better written mostly in C, since from what I can see, it mostly just runs commands. You can still have a Python wrapper - see for example how test_vbe works. It uses 'ut xxx -f' to execute the C part of a Python test. You can do setup in Python, then run the C test. What do you think?
It is just my opinion, if the test requires some setup in Python, I like Python to write the test cases as far as tests can be implemented in Python, because we can maintain the test code in one place.
Actually almost all the test code can go in C. See here for the trade-offs [1].
Regards, Simon
[1] https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html
Hi Kojima-san,
On Mon, Apr 10, 2023 at 06:07:28PM +0900, Masahisa Kojima wrote:
Firmware version management is not implemented in the current FMP implementation. This series aims to add the versioning support in FMP.
There is a major design change in v5. Until v4, the fw_version and lowest_supported_version are stored as a EFI variable. If the backing storage is a file we can't trust any of that information since anyone can tamper with the file, although the variables are defined as RO. With that, we store the version information in the device tree in v5. We can trust the information from dtb as long as the former stage boot loader verifies the image containing the dtb.
I have a basic question here. You said that the former-stage boot loader is responsible for maintaining the dtb with the correct firmware version to be passed to U-Boot. But how can it obtain the new version number of firmware which is only contained in a capsule file (and its header)?
Looking into you pytest, you simply always *reboot* the sandbox with an already-modified dtb (test_ver.dtb). (Please note that, at the reboot time, a capsule has not been applied yet.)
I believe that your current approach is rather incomplete as a workable solution.
-Takahiro Akashi
The disadvantage of this design change is that we need to maintain the fw_version in both device tree and FMP Payload Header. It is inevitable since not all the capsule files contain the dtb.
EDK II reference implementation utilizes the FMP Payload Header inserted right before the capsule payload. With this series, U-Boot also follows the EDK II implementation.
Currently, there is no way to know the current running firmware version through the EFI interface. FMP->GetImageInfo() returns always 0 for the version number. So a user can not know that expected firmware is running after the capsule update.
With this series applied, version number can be specified in the capsule file generation with mkeficapsule tool, then user can know the running firmware version through FMP->GetImageInfo() and ESRT.
Note that this series does not mandate the FMP Payload Header, compatible with boards that are already using the existing U-Boot FMP implementation. If no FMP Payload Header is found in the capsule file, fw_version, lowest supported version, last attempt version and last attempt status is set to 0 and this is the same behavior as existing FMP implementation.
Major Changes in v5:
- major design changes, versioning is implemented with device tree instead of EFI variable
Major Changes in v4:
- add python-based test
Major Changes in v3:
- exclude CONFIG_FWU_MULTI
Masahisa Kojima (4): efi_loader: get version information from device tree efi_loader: check lowest supported version mkeficapsule: add FMP Payload Header test/py: efi_capsule: test for FMP versioning
.../firmware/firmware-version.txt | 25 +++ doc/mkeficapsule.1 | 10 + lib/efi_loader/efi_firmware.c | 187 +++++++++++++--- test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ tools/eficapsule.h | 30 +++ tools/mkeficapsule.c | 37 +++- 11 files changed, 1082 insertions(+), 29 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt create mode 100644 test/py/tests/test_efi_capsule/version.dts
-- 2.17.1
Hi Akashi-san,
On Tue, 11 Apr 2023 at 10:48, Takahiro Akashi takahiro.akashi@linaro.org wrote:
Hi Kojima-san,
On Mon, Apr 10, 2023 at 06:07:28PM +0900, Masahisa Kojima wrote:
Firmware version management is not implemented in the current FMP implementation. This series aims to add the versioning support in FMP.
There is a major design change in v5. Until v4, the fw_version and lowest_supported_version are stored as a EFI variable. If the backing storage is a file we can't trust any of that information since anyone can tamper with the file, although the variables are defined as RO. With that, we store the version information in the device tree in v5. We can trust the information from dtb as long as the former stage boot loader verifies the image containing the dtb.
I have a basic question here. You said that the former-stage boot loader is responsible for maintaining the dtb with the correct firmware version to be passed to U-Boot. But how can it obtain the new version number of firmware which is only contained in a capsule file (and its header)?
Yes, there is a problem that we need to maintain the fw_version in both FMP Payload Header and dtb, it is not an ideal situation and prone to errors.
On second thought, I need to change my approach again. fw_version: specified in FMP Payload Header and stored in EFI variable lowest_supported_version: stored in dtb
When the capsule update is performed, U-Boot gets the fw_version from FMP Payload Header of the ongoing capsule, then checks if the version is equal to or greater than lowest_supported_version read from dtb. One disadvantage is that fw_version in the EFI variable can be tampered if the backing storage is file, but it is not the critical issue.
Thanks, Masahisa Kojima
Looking into you pytest, you simply always *reboot* the sandbox with an already-modified dtb (test_ver.dtb). (Please note that, at the reboot time, a capsule has not been applied yet.)
I believe that your current approach is rather incomplete as a workable solution.
-Takahiro Akashi
The disadvantage of this design change is that we need to maintain the fw_version in both device tree and FMP Payload Header. It is inevitable since not all the capsule files contain the dtb.
EDK II reference implementation utilizes the FMP Payload Header inserted right before the capsule payload. With this series, U-Boot also follows the EDK II implementation.
Currently, there is no way to know the current running firmware version through the EFI interface. FMP->GetImageInfo() returns always 0 for the version number. So a user can not know that expected firmware is running after the capsule update.
With this series applied, version number can be specified in the capsule file generation with mkeficapsule tool, then user can know the running firmware version through FMP->GetImageInfo() and ESRT.
Note that this series does not mandate the FMP Payload Header, compatible with boards that are already using the existing U-Boot FMP implementation. If no FMP Payload Header is found in the capsule file, fw_version, lowest supported version, last attempt version and last attempt status is set to 0 and this is the same behavior as existing FMP implementation.
Major Changes in v5:
- major design changes, versioning is implemented with device tree instead of EFI variable
Major Changes in v4:
- add python-based test
Major Changes in v3:
- exclude CONFIG_FWU_MULTI
Masahisa Kojima (4): efi_loader: get version information from device tree efi_loader: check lowest supported version mkeficapsule: add FMP Payload Header test/py: efi_capsule: test for FMP versioning
.../firmware/firmware-version.txt | 25 +++ doc/mkeficapsule.1 | 10 + lib/efi_loader/efi_firmware.c | 187 +++++++++++++--- test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ tools/eficapsule.h | 30 +++ tools/mkeficapsule.c | 37 +++- 11 files changed, 1082 insertions(+), 29 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt create mode 100644 test/py/tests/test_efi_capsule/version.dts
-- 2.17.1
participants (4)
-
Heinrich Schuchardt -
Masahisa Kojima -
Simon Glass -
Takahiro Akashi