| Message ID | 20210324142353.1216042-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi_loader: EFI TCG2 free efi memory on protocol failure | expand |
On 24.03.21 15:23, Ilias Apalodimas wrote: > Current code doesn't free the efi allocated memory in case the protocol > failed to install > > Fixes: c8d0fd582576 ("efi_loader: Introduce eventlog support for TCG2_PROTOCOL") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/efi_loader/efi_tcg2.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index 797d6eb134f6..02de63808f9a 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -988,6 +988,7 @@ static efi_status_t create_final_event(void) > > return EFI_SUCCESS; > out: > + efi_free_pool(event_log.final_buffer); > return ret; > } > > @@ -1041,8 +1042,12 @@ static efi_status_t efi_init_event_log(void) > event_log.last_event_size = event_log.pos; > > ret = create_final_event(); Shouldn't create_final_event() should free event_log.final_buffer if efi_install_configuration_table() fails? > + if (ret != EFI_SUCCESS) > + goto out; > > + return EFI_SUCCESS; > out: > + efi_free_pool(event_log.buffer); We must set event_log.buffer to NULL to avoid double free? > return ret; > } > > @@ -1055,23 +1060,31 @@ out: > */ > efi_status_t efi_tcg2_register(void) > { > - efi_status_t ret; > + efi_status_t ret = EFI_SUCCESS; > struct udevice *dev; > > ret = platform_get_tpm2_device(&dev); > if (ret != EFI_SUCCESS) { > log_warning("Unable to find TPMv2 device\n"); > - return EFI_SUCCESS; > + ret = EFI_SUCCESS; > + goto out; > } > > ret = efi_init_event_log(); > if (ret != EFI_SUCCESS) > - return ret; > + goto fail; > > ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol, > (void *)&efi_tcg2_protocol); > - if (ret != EFI_SUCCESS) > + if (ret != EFI_SUCCESS) { > log_err("Cannot install EFI_TCG2_PROTOCOL\n"); > + goto fail; > + } > > +out: > + return ret; > +fail: In create_final_event() you have installed a configuration table EFI_TCG2_FINAL_EVENTS_TABLE_GUID. You should not free the memory of the configuration table without uninstalling the table. Should we create separate uninit function which takes care of the logic? Best regards Heinrich > + efi_free_pool(event_log.final_buffer); > + efi_free_pool(event_log.buffer); > return ret; > } >
On Thu, Mar 25, 2021 at 11:10:26AM +0100, Heinrich Schuchardt wrote: > On 24.03.21 15:23, Ilias Apalodimas wrote: > > Current code doesn't free the efi allocated memory in case the protocol > > failed to install > > > > Fixes: c8d0fd582576 ("efi_loader: Introduce eventlog support for TCG2_PROTOCOL") > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > lib/efi_loader/efi_tcg2.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index 797d6eb134f6..02de63808f9a 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -988,6 +988,7 @@ static efi_status_t create_final_event(void) > > > > return EFI_SUCCESS; > > out: > > + efi_free_pool(event_log.final_buffer); > > return ret; > > } > > > > @@ -1041,8 +1042,12 @@ static efi_status_t efi_init_event_log(void) > > event_log.last_event_size = event_log.pos; > > > > ret = create_final_event(); > > Shouldn't create_final_event() should free event_log.final_buffer if > efi_install_configuration_table() fails? > I am not sure I undetstand you here. create_final_event() frees event_log.final_buffer if installing the config table fails. > > + if (ret != EFI_SUCCESS) > > + goto out; > > > > + return EFI_SUCCESS; > > out: > > + efi_free_pool(event_log.buffer); > > We must set event_log.buffer to NULL to avoid double free? Yea sure > > > return ret; > > } > > > > @@ -1055,23 +1060,31 @@ out: > > */ > > efi_status_t efi_tcg2_register(void) > > { > > - efi_status_t ret; > > + efi_status_t ret = EFI_SUCCESS; > > struct udevice *dev; > > > > ret = platform_get_tpm2_device(&dev); > > if (ret != EFI_SUCCESS) { > > log_warning("Unable to find TPMv2 device\n"); > > - return EFI_SUCCESS; > > + ret = EFI_SUCCESS; > > + goto out; > > } > > > > ret = efi_init_event_log(); > > if (ret != EFI_SUCCESS) > > - return ret; > > + goto fail; > > > > ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol, > > (void *)&efi_tcg2_protocol); > > - if (ret != EFI_SUCCESS) > > + if (ret != EFI_SUCCESS) { > > log_err("Cannot install EFI_TCG2_PROTOCOL\n"); > > + goto fail; > > + } > > > > +out: > > + return ret; > > +fail: > > In create_final_event() you have installed a configuration table > EFI_TCG2_FINAL_EVENTS_TABLE_GUID. You should not free the memory of the > configuration table without uninstalling the table. > > Should we create separate uninit function which takes care of the logic? I guess I could gather all the frees/uninstalles in one function and call that explicitly > > Best regards > > Heinrich > > > + efi_free_pool(event_log.final_buffer); > > + efi_free_pool(event_log.buffer); > > return ret; > > } > > >
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 797d6eb134f6..02de63808f9a 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -988,6 +988,7 @@ static efi_status_t create_final_event(void) return EFI_SUCCESS; out: + efi_free_pool(event_log.final_buffer); return ret; } @@ -1041,8 +1042,12 @@ static efi_status_t efi_init_event_log(void) event_log.last_event_size = event_log.pos; ret = create_final_event(); + if (ret != EFI_SUCCESS) + goto out; + return EFI_SUCCESS; out: + efi_free_pool(event_log.buffer); return ret; } @@ -1055,23 +1060,31 @@ out: */ efi_status_t efi_tcg2_register(void) { - efi_status_t ret; + efi_status_t ret = EFI_SUCCESS; struct udevice *dev; ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) { log_warning("Unable to find TPMv2 device\n"); - return EFI_SUCCESS; + ret = EFI_SUCCESS; + goto out; } ret = efi_init_event_log(); if (ret != EFI_SUCCESS) - return ret; + goto fail; ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol, (void *)&efi_tcg2_protocol); - if (ret != EFI_SUCCESS) + if (ret != EFI_SUCCESS) { log_err("Cannot install EFI_TCG2_PROTOCOL\n"); + goto fail; + } +out: + return ret; +fail: + efi_free_pool(event_log.final_buffer); + efi_free_pool(event_log.buffer); return ret; }
Current code doesn't free the efi allocated memory in case the protocol failed to install Fixes: c8d0fd582576 ("efi_loader: Introduce eventlog support for TCG2_PROTOCOL") Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- lib/efi_loader/efi_tcg2.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) -- 2.31.0