| Message ID | 20220401191759.1670812-5-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi: capsule: Capsule Update fixes and enhancements | expand |
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@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(
On Sat, 2 Apr 2022 at 15:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > 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@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. Okay. WIll check and remove the aligned allocation. This is how it has been defined for all the set_dfu_alt_info functions. If not needed, I will remove it. > > > + > > + 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? This can be done away with in this particular definition of the function, since this will be used only for the capsule update use case, where we do need to get the variable as defined by the board through the dfu_string. Will remove this check. > > > + 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. Will clean this up. Thanks. -sughosh > > Best regards > > Heinrich > > > +} > > + > > /* Place holder; not supported */ > > static > > efi_status_t EFIAPI efi_firmware_get_image_unsupported( >
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); + + if (!CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT) && + env_get("dfu_alt_info")) + return; + + dfu_alt_info = update_info.dfu_string; + n = strlen(dfu_alt_info); + memset(buf, 0, n + 1); + + strncpy(buf, dfu_alt_info, n); + + env_set("dfu_alt_info", buf); +} + /* Place holder; not supported */ static efi_status_t EFIAPI efi_firmware_get_image_unsupported(
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@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(+)