[PATCH v5 4/8] efi: Define set_dfu_alt_info() for boards with UEFI capsule update enabled

[Heinrich Schuchardt]

On 4/1/22 21:17, Sughosh Ganu wrote:
> Currently, there are a bunch of boards which enable the UEFI capsule
> update feature. The actual update of the firmware images is done
> through the dfu framework which uses the dfu_alt_info environment
> variable for getting information on the update, like device, partition
> number/address etc. The dfu framework allows the variable to be set
> through the set_dfu_alt_info function defined by the platform, or if
> the function is not defined, it gets the variable from the
> environment. Using the value set in the environment is not very
> robust, since the variable can be modified from the u-boot command
> line and this can cause an incorrect update.
>
> To prevent this from happening, define the set_dfu_alt_info function
> when the capsule update feature is enabled. A weak function is defined
> which sets the dfu_alt_info environment variable by getting the string
> for the variable from the platform.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> ---
>
> Changes since V4:
> * Define a weak function set_dfu_alt_info for setting the variable in
>    a non board specific file as suggested by Ilias
> * Drop the definitions of set_dfu_alt_info that were being added in
>    the board files
>
>   lib/efi_loader/Kconfig        |  2 ++
>   lib/efi_loader/efi_firmware.c | 23 +++++++++++++++++++++++
>   2 files changed, 25 insertions(+)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e5e35fe51f..09fb8cbe75 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -174,6 +174,7 @@ config EFI_CAPSULE_FIRMWARE_FIT
>   	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
>   	select UPDATE_FIT
>   	select DFU
> +	select SET_DFU_ALT_INFO
>   	select EFI_CAPSULE_FIRMWARE
>   	help
>   	  Select this option if you want to enable firmware management protocol
> @@ -185,6 +186,7 @@ config EFI_CAPSULE_FIRMWARE_RAW
>   	depends on SANDBOX || (!SANDBOX && !EFI_CAPSULE_FIRMWARE_FIT)
>   	select DFU_WRITE_ALT
>   	select DFU
> +	select SET_DFU_ALT_INFO
>   	select EFI_CAPSULE_FIRMWARE
>   	help
>   	  Select this option if you want to enable firmware management protocol
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index e9fc4b9c1e..cbfa57f64f 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -11,9 +11,11 @@
>   #include <dfu.h>
>   #include <efi_loader.h>
>   #include <image.h>
> +#include <memalign.h>
>   #include <signatures.h>
>
>   #include <linux/list.h>
> +#include <linux/sizes.h>
>
>   #define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
>
> @@ -35,6 +37,27 @@ struct fmp_payload_header {
>   	u32 lowest_supported_version;
>   };
>
> +#define DFU_ALT_BUF_LEN		SZ_1K
> +
> +__weak void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +	int n;
> +	const char *dfu_alt_info;
> +	ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);

env_set() copies the string and does not require a cache aligned buffer.

> +
> +	if (!CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT) &&
> +	    env_get("dfu_alt_info"))

This deserves a comment, e.g.

For capsule support we always want to use a hard coded value.
For other cases of DFU avoid overwriting a preexisting value.

But why would you not overwrite a preexisting value if the hardware has
a fixed definition?

> +		return;
> +
> +	dfu_alt_info = update_info.dfu_string;
> +	n = strlen(dfu_alt_info);
> +	memset(buf, 0, n + 1);

This will result in a buffer overrun if update_info.dfu_string is longer
than DFU_ALT_BUF_LEN.

> +
> +	strncpy(buf, dfu_alt_info, n);

Another buffer overrun.

> +
> +	env_set("dfu_alt_info", buf);

This is all you need:

env_set("dfu_alt_info", update_info.dfu_string);

If any DFU driver has a problem with properly handling dfu_alt_info
being longer than DFU_ALT_BUF_LEN, you should fix the problem there.

Best regards

Heinrich

> +}
> +
>   /* Place holder; not supported */
>   static
>   efi_status_t EFIAPI efi_firmware_get_image_unsupported(