| Message ID | 20250213195351.3518305-3-j-humphreys@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilias Apalodimas |
| Headers | show |
| Series | EFI Capsule update explicitly sets dfu_alt_info | expand |
Hi Jon, Thank you for the patch. On jeu., févr. 13, 2025 at 13:53, Jonathan Humphreys <j-humphreys@ti.com> wrote: > The current implementation of EFI capsule update uses set_dfu_alt_info() to > set the dfu_alt_info environment variable with the settings it requires. > However, set_dfu_alt_info() is doing this for all DFU operations, even > those unrelated to capsule update. > > Thus other uses of DFU, such as DFU boot which sets its own value for the > dfu_alt_info environment variable, will have that setting overwritten with > the capsule update setting. Similarly, any user defined value for the > dfu_alt_info environment variable would get overwritten when any DFU > operation was performed, including simply performing a "dfu 0 list" > command. > > The solution is stop using the set_dfu_alt_info() mechanism to set the > dfu_alt_info environment variable and instead explicitly set it to the > capsule update's setting just before performing the capsule update's DFU > operation, and then restore the environment variable back to its original > value. > > This patch implements the explicit setting and restoring of the > dfu_alt_info environment variable as part of the EFI capsule update > operation. > > The fix is fully implemented in a subsequent patch that removes the capsule > update dfu_alt_info support in set_dfu_alt_info(). > > Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > ---
Hi Jonathan, The patch looks correct but there's a few more things to address. On Thu, Feb 13, 2025 at 01:53:50PM -0600, Jonathan Humphreys wrote: > The current implementation of EFI capsule update uses set_dfu_alt_info() to > set the dfu_alt_info environment variable with the settings it requires. > However, set_dfu_alt_info() is doing this for all DFU operations, even > those unrelated to capsule update. > > Thus other uses of DFU, such as DFU boot which sets its own value for the > dfu_alt_info environment variable, will have that setting overwritten with > the capsule update setting. Similarly, any user defined value for the > dfu_alt_info environment variable would get overwritten when any DFU > operation was performed, including simply performing a "dfu 0 list" > command. > > The solution is stop using the set_dfu_alt_info() mechanism to set the > dfu_alt_info environment variable and instead explicitly set it to the > capsule update's setting just before performing the capsule update's DFU > operation, and then restore the environment variable back to its original > value. > > This patch implements the explicit setting and restoring of the > dfu_alt_info environment variable as part of the EFI capsule update > operation. > > The fix is fully implemented in a subsequent patch that removes the capsule > update dfu_alt_info support in set_dfu_alt_info(). > > Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com> > --- > lib/efi_loader/efi_firmware.c | 39 ++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > index 5a754c9cd03..1a1cf3b55e1 100644 > --- a/lib/efi_loader/efi_firmware.c > +++ b/lib/efi_loader/efi_firmware.c > @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( > efi_status_t (*progress)(efi_uintn_t completion), > u16 **abort_reason) > { > + int ret; > efi_status_t status; > struct fmp_state state = { 0 }; > + char *orig_dfu_env; > > EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, > image_size, vendor_code, progress, abort_reason); > @@ -663,7 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( > if (status != EFI_SUCCESS) > return EFI_EXIT(status); > > - if (fit_update(image)) > + orig_dfu_env = strdup(env_get("dfu_alt_info")); strdup might fail, we need to check that the orig_dfu_env is !NULL > + if (env_set("dfu_alt_info", update_info.dfu_string)) { > + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > + free(orig_dfu_env); > + return EFI_EXIT(EFI_DEVICE_ERROR); > + } > + > + ret = fit_update(image); > + > + if (env_set("dfu_alt_info", orig_dfu_env)) { > + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > + ret = 1; This will work but for consistency just use an EFI defined error e.g ret = EFI_DEVICE_ERROR; > + } > + free(orig_dfu_env); > + > + if (ret) I am not 100% sure this exiting here is useful. If fit_update has succeeded we have updated our firmware. So I think we should just add a warning here, that resetting the dfu to its original value failed, and any further dfu ops will fail. > return EFI_EXIT(EFI_DEVICE_ERROR); > > efi_firmware_set_fmp_state_var(&state, image_index); > @@ -717,6 +734,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > u8 dfu_alt_num; > efi_status_t status; > struct fmp_state state = { 0 }; > + char *orig_dfu_env; > > EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, > image_size, vendor_code, progress, abort_reason); > @@ -747,8 +765,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > } > } > > - if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, > - NULL, NULL)) > + orig_dfu_env = strdup(env_get("dfu_alt_info")); > + if (env_set("dfu_alt_info", update_info.dfu_string)) { > + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > + free(orig_dfu_env); > + return EFI_EXIT(EFI_DEVICE_ERROR); > + } > + > + ret = dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, > + NULL, NULL); > + > + if (env_set("dfu_alt_info", orig_dfu_env)) { > + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > + ret = 1; > + } > + free(orig_dfu_env); > + > + if (ret) > return EFI_EXIT(EFI_DEVICE_ERROR); > > efi_firmware_set_fmp_state_var(&state, image_index); > -- > 2.34.1 > Thanks /Ilias
Ilias Apalodimas <ilias.apalodimas@linaro.org> writes: > Hi Jonathan, > > The patch looks correct but there's a few more things to address. > > On Thu, Feb 13, 2025 at 01:53:50PM -0600, Jonathan Humphreys wrote: >> The current implementation of EFI capsule update uses set_dfu_alt_info() to >> set the dfu_alt_info environment variable with the settings it requires. >> However, set_dfu_alt_info() is doing this for all DFU operations, even >> those unrelated to capsule update. >> >> Thus other uses of DFU, such as DFU boot which sets its own value for the >> dfu_alt_info environment variable, will have that setting overwritten with >> the capsule update setting. Similarly, any user defined value for the >> dfu_alt_info environment variable would get overwritten when any DFU >> operation was performed, including simply performing a "dfu 0 list" >> command. >> >> The solution is stop using the set_dfu_alt_info() mechanism to set the >> dfu_alt_info environment variable and instead explicitly set it to the >> capsule update's setting just before performing the capsule update's DFU >> operation, and then restore the environment variable back to its original >> value. >> >> This patch implements the explicit setting and restoring of the >> dfu_alt_info environment variable as part of the EFI capsule update >> operation. >> >> The fix is fully implemented in a subsequent patch that removes the capsule >> update dfu_alt_info support in set_dfu_alt_info(). >> >> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com> >> --- >> lib/efi_loader/efi_firmware.c | 39 ++++++++++++++++++++++++++++++++--- >> 1 file changed, 36 insertions(+), 3 deletions(-) >> >> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c >> index 5a754c9cd03..1a1cf3b55e1 100644 >> --- a/lib/efi_loader/efi_firmware.c >> +++ b/lib/efi_loader/efi_firmware.c >> @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( >> efi_status_t (*progress)(efi_uintn_t completion), >> u16 **abort_reason) >> { >> + int ret; >> efi_status_t status; >> struct fmp_state state = { 0 }; >> + char *orig_dfu_env; >> >> EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, >> image_size, vendor_code, progress, abort_reason); >> @@ -663,7 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( >> if (status != EFI_SUCCESS) >> return EFI_EXIT(status); >> >> - if (fit_update(image)) >> + orig_dfu_env = strdup(env_get("dfu_alt_info")); > > strdup might fail, we need to check that the orig_dfu_env is !NULL > >> + if (env_set("dfu_alt_info", update_info.dfu_string)) { >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); >> + free(orig_dfu_env); >> + return EFI_EXIT(EFI_DEVICE_ERROR); >> + } >> + >> + ret = fit_update(image); > >> + >> + if (env_set("dfu_alt_info", orig_dfu_env)) { >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); >> + ret = 1; > > This will work but for consistency just use an EFI defined error e.g > ret = EFI_DEVICE_ERROR; > >> + } >> + free(orig_dfu_env); >> + >> + if (ret) > > I am not 100% sure this exiting here is useful. If fit_update has succeeded > we have updated our firmware. So I think we should just add a warning here, > that resetting the dfu to its original value failed, and any further dfu > ops will fail. > Hi Ilias, thanks for the review. The exit here is for either fit_update() failing OR we cannot restore the original value for the dfu_alt_info variable, so it is needed, at least in the fit_update() failure case. I was trying to keep the control flow simpler by handling the 2 failure events together. Are you wanting to separate the 2 failure conditions, and return EFI_DEVICE_ERROR if fit_update() fails, and emit a warning and return EFI_SUCCESS if restoring the env variable fails? Jon >> return EFI_EXIT(EFI_DEVICE_ERROR); >> >> efi_firmware_set_fmp_state_var(&state, image_index); >> @@ -717,6 +734,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( >> u8 dfu_alt_num; >> efi_status_t status; >> struct fmp_state state = { 0 }; >> + char *orig_dfu_env; >> >> EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, >> image_size, vendor_code, progress, abort_reason); >> @@ -747,8 +765,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( >> } >> } >> >> - if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, >> - NULL, NULL)) >> + orig_dfu_env = strdup(env_get("dfu_alt_info")); >> + if (env_set("dfu_alt_info", update_info.dfu_string)) { >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); >> + free(orig_dfu_env); >> + return EFI_EXIT(EFI_DEVICE_ERROR); >> + } >> + >> + ret = dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, >> + NULL, NULL); >> + >> + if (env_set("dfu_alt_info", orig_dfu_env)) { >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); >> + ret = 1; >> + } >> + free(orig_dfu_env); >> + >> + if (ret) >> return EFI_EXIT(EFI_DEVICE_ERROR); >> >> efi_firmware_set_fmp_state_var(&state, image_index); >> -- >> 2.34.1 >> > > Thanks > /Ilias
Hi Jonathan, Apologies for the late reply On Mon, 24 Feb 2025 at 04:38, Jon Humphreys <j-humphreys@ti.com> wrote: [...] > >> + > >> + ret = fit_update(image); > > > >> + > >> + if (env_set("dfu_alt_info", orig_dfu_env)) { > >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > >> + ret = 1; > > > > This will work but for consistency just use an EFI defined error e.g > > ret = EFI_DEVICE_ERROR; > > > >> + } > >> + free(orig_dfu_env); > >> + > >> + if (ret) > > > > I am not 100% sure this exiting here is useful. If fit_update has succeeded > > we have updated our firmware. So I think we should just add a warning here, > > that resetting the dfu to its original value failed, and any further dfu > > ops will fail. > > > > Hi Ilias, thanks for the review. > > The exit here is for either fit_update() failing OR we cannot restore the > original value for the dfu_alt_info variable, so it is needed, at least in > the fit_update() failure case. > > I was trying to keep the control flow simpler by handling the 2 failure > events together. Are you wanting to separate the 2 failure conditions, and > return EFI_DEVICE_ERROR if fit_update() fails, and emit a warning and > return EFI_SUCCESS if restoring the env variable fails? Yes, I think it's better Thanks /Ilias > > Jon > > >> return EFI_EXIT(EFI_DEVICE_ERROR); > >> > >> efi_firmware_set_fmp_state_var(&state, image_index); > >> @@ -717,6 +734,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > >> u8 dfu_alt_num; > >> efi_status_t status; > >> struct fmp_state state = { 0 }; > >> + char *orig_dfu_env; > >> > >> EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, > >> image_size, vendor_code, progress, abort_reason); > >> @@ -747,8 +765,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > >> } > >> } > >> > >> - if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, > >> - NULL, NULL)) > >> + orig_dfu_env = strdup(env_get("dfu_alt_info")); > >> + if (env_set("dfu_alt_info", update_info.dfu_string)) { > >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > >> + free(orig_dfu_env); > >> + return EFI_EXIT(EFI_DEVICE_ERROR); > >> + } > >> + > >> + ret = dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, > >> + NULL, NULL); > >> + > >> + if (env_set("dfu_alt_info", orig_dfu_env)) { > >> + log_err("unable to set env variable \"dfu_alt_info\"!\n"); > >> + ret = 1; > >> + } > >> + free(orig_dfu_env); > >> + > >> + if (ret) > >> return EFI_EXIT(EFI_DEVICE_ERROR); > >> > >> efi_firmware_set_fmp_state_var(&state, image_index); > >> -- > >> 2.34.1 > >> > > > > Thanks > > /Ilias
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 5a754c9cd03..1a1cf3b55e1 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( efi_status_t (*progress)(efi_uintn_t completion), u16 **abort_reason) { + int ret; efi_status_t status; struct fmp_state state = { 0 }; + char *orig_dfu_env; EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, image_size, vendor_code, progress, abort_reason); @@ -663,7 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( if (status != EFI_SUCCESS) return EFI_EXIT(status); - if (fit_update(image)) + orig_dfu_env = strdup(env_get("dfu_alt_info")); + if (env_set("dfu_alt_info", update_info.dfu_string)) { + log_err("unable to set env variable \"dfu_alt_info\"!\n"); + free(orig_dfu_env); + return EFI_EXIT(EFI_DEVICE_ERROR); + } + + ret = fit_update(image); + + if (env_set("dfu_alt_info", orig_dfu_env)) { + log_err("unable to set env variable \"dfu_alt_info\"!\n"); + ret = 1; + } + free(orig_dfu_env); + + if (ret) return EFI_EXIT(EFI_DEVICE_ERROR); efi_firmware_set_fmp_state_var(&state, image_index); @@ -717,6 +734,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( u8 dfu_alt_num; efi_status_t status; struct fmp_state state = { 0 }; + char *orig_dfu_env; EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, image_size, vendor_code, progress, abort_reason); @@ -747,8 +765,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( } } - if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, - NULL, NULL)) + orig_dfu_env = strdup(env_get("dfu_alt_info")); + if (env_set("dfu_alt_info", update_info.dfu_string)) { + log_err("unable to set env variable \"dfu_alt_info\"!\n"); + free(orig_dfu_env); + return EFI_EXIT(EFI_DEVICE_ERROR); + } + + ret = dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, + NULL, NULL); + + if (env_set("dfu_alt_info", orig_dfu_env)) { + log_err("unable to set env variable \"dfu_alt_info\"!\n"); + ret = 1; + } + free(orig_dfu_env); + + if (ret) return EFI_EXIT(EFI_DEVICE_ERROR); efi_firmware_set_fmp_state_var(&state, image_index);
The current implementation of EFI capsule update uses set_dfu_alt_info() to set the dfu_alt_info environment variable with the settings it requires. However, set_dfu_alt_info() is doing this for all DFU operations, even those unrelated to capsule update. Thus other uses of DFU, such as DFU boot which sets its own value for the dfu_alt_info environment variable, will have that setting overwritten with the capsule update setting. Similarly, any user defined value for the dfu_alt_info environment variable would get overwritten when any DFU operation was performed, including simply performing a "dfu 0 list" command. The solution is stop using the set_dfu_alt_info() mechanism to set the dfu_alt_info environment variable and instead explicitly set it to the capsule update's setting just before performing the capsule update's DFU operation, and then restore the environment variable back to its original value. This patch implements the explicit setting and restoring of the dfu_alt_info environment variable as part of the EFI capsule update operation. The fix is fully implemented in a subsequent patch that removes the capsule update dfu_alt_info support in set_dfu_alt_info(). Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com> --- lib/efi_loader/efi_firmware.c | 39 ++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)