Re: [PATCH 2/2] efi_selftest: add minimal fmp test

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 9/22/26 16:58, Vincent Stehlé wrote:
> Add a minimal test for the UEFI Firmware Management Protocol.
> At this point the test only does the following:
> 
> - Get current firmware images info.
> - For each image, verify consistency of the authentication flags.
> 
> This can be tested on the sandbox with the following command:
> 
>    ./u-boot -T -c "setenv efi_selftest firmware management protocol; \
>                    bootefi selftest"
> 
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_selftest/Makefile           |   2 +
>   lib/efi_selftest/efi_selftest_fmp.c | 135 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 137 insertions(+)
> 
> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> index 67102648703..9882d9f2748 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -80,6 +80,8 @@ obj-$(CONFIG_EFI_ESRT) += efi_selftest_esrt.o
>   
>   obj-$(CONFIG_EFI_DEBUG_SUPPORT) += efi_selftest_debug_support.o
>   
> +obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_selftest_fmp.o
> +
>   targets += \
>   efi_miniapp_file_image_exception.h \
>   efi_miniapp_file_image_exit.h \
> diff --git a/lib/efi_selftest/efi_selftest_fmp.c b/lib/efi_selftest/efi_selftest_fmp.c
> new file mode 100644
> index 00000000000..c17cf59a237
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_fmp.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Test Firmware Management Protocol support.
> + *
> + * Copyright (C) 2026 Arm Ltd.
> + */
> +#include <efi_selftest.h>
> +
> +static const efi_guid_t firmware_management_protocol_guid =
> +	EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID;
> +
> +static struct efi_boot_services *boottime;
> +static struct efi_firmware_management_protocol *fmp;
> +
> +/**
> + * setup() - Setup unit test.
> + * @handle:	handle of the loaded image
> + * @systable:	system table
> + *
> + * Keep Boot Services and locate firmware management protocol.
> + *
> + * Return:	EFI_ST_SUCCESS for success, EFI_ST_FAILURE in case of error.
> + */
> +static int setup(const efi_handle_t handle,
> +		 const struct efi_system_table *systable)
> +{
> +	efi_status_t ret;
> +
> +	boottime = systable->boottime;
> +
> +	ret = boottime->locate_protocol(&firmware_management_protocol_guid,
> +					NULL, (void **)&fmp);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Firmware management protocol not available.\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	return EFI_ST_SUCCESS;
> +}
> +
> +/**
> + * execute() - Perform the test.
> + *
> + * Call GetImageInfo() to retrieve current firmware images descriptors.
> + * For each descriptor, verify image authentication consistency.
> + *
> + * Return:	EFI_ST_SUCCESS for success, EFI_ST_FAILURE in case of error.
> + */
> +static int execute(void)
> +{
> +	efi_status_t ret;
> +	efi_uintn_t img_info_size, desc_size;
> +	struct efi_firmware_image_descriptor *img_info = NULL;
> +	u32 desc_ver, pkg_ver;
> +	u8 desc_cnt;
> +	u16 *pkg_ver_name;
> +	int i, result = EFI_ST_FAILURE;
> +
> +	/* Determine buffer size */
> +	img_info_size = 0;
> +	ret = fmp->get_image_info(fmp, &img_info_size, NULL, NULL, NULL, NULL,
> +				  NULL, NULL);
> +	if (ret != EFI_BUFFER_TOO_SMALL) {
> +		efi_st_error("GetImageInfo returned %u\n", (unsigned int)ret);
> +		goto out;
> +	}

This fails if the number of images is 0.

ERROR: GetImageInfo returned 2 (EFI_INVALID_PARAMETER)

+struct efi_fw_image fw_images[] = {
+};
+
+struct efi_capsule_update_info update_info = {
+       .dfu_string = NULL,
+       .num_images = ARRAY_SIZE(fw_images),
+       .images = fw_images,
+};

This looks like a bug in efi_fill_image_desc_array((). Why does it 
return EFI_INVALID_PARAMETER if the number of images is 0?

We should just return *img_info_size = 0 and EFI_SUCCESS instead.

The test should return EFI_ST_SUCCESS in this case.

Best regards

Heinrich

> +
> +	/* Allocate enough memory for image info */
> +	ret = boottime->allocate_pool(EFI_LOADER_DATA, img_info_size,
> +				      (void **)&img_info);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("AllocatePool returned %u\n", (unsigned int)ret);
> +		goto out;
> +	}
> +
> +	ret = fmp->get_image_info(fmp, &img_info_size, img_info, &desc_ver,
> +				  &desc_cnt, &desc_size, &pkg_ver,
> +				  &pkg_ver_name);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("GetImageInfo returned %u\n", (unsigned int)ret);
> +		goto out;
> +	}
> +
> +	if (desc_ver != EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION) {
> +		efi_st_error("Bad DescriptorVersion %u\n", desc_ver);
> +		goto out;
> +	}
> +
> +	/* Verify each image info */
> +	for (i = 0; i < desc_cnt; i++) {
> +		const struct efi_firmware_image_descriptor *inf = &img_info[i];
> +		u64 attr_supp = inf->attributes_supported,
> +		    attr_set = inf->attributes_setting;
> +
> +		efi_st_printf("  image %u: %ps\n", inf->image_index, inf->image_id_name);
> +
> +		/* Verify authentication */
> +		if (CONFIG_IS_ENABLED(EFI_CAPSULE_AUTHENTICATE) &&
> +		    (attr_supp & IMAGE_ATTRIBUTE_IMAGE_UPDATABLE) &&
> +		    !(attr_supp & IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED)) {
> +			efi_st_error("Bad AttributesSupported %u\n",
> +				     (unsigned int)attr_supp);
> +			goto out;
> +		}
> +
> +		if (CONFIG_IS_ENABLED(EFI_CAPSULE_AUTHENTICATE) &&
> +		    (attr_set & IMAGE_ATTRIBUTE_IMAGE_UPDATABLE) &&
> +		    !(attr_set & IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED)) {
> +			efi_st_error("Bad AttributesSetting %u\n",
> +				     (unsigned int)attr_set);
> +			goto out;
> +		}
> +	}
> +
> +	result = EFI_ST_SUCCESS;
> +
> +out:
> +	if (img_info) {
> +		ret = boottime->free_pool(img_info);
> +		if (ret != EFI_SUCCESS) {
> +			efi_st_error("FreePool returned %u\n",
> +				     (unsigned int)ret);
> +			result = EFI_ST_FAILURE;
> +		}
> +	}
> +
> +	return result;
> +}
> +
> +EFI_UNIT_TEST(fmp) = {
> +	.name = "firmware management protocol",
> +	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> +	.setup = setup,
> +	.execute = execute,
> +};
>