[PATCH] efi_loader: EFI TCG2 free efi memory on protocol failure

[PATCH] efi_loader: EFI TCG2 free efi memory on protocol failure

[Ilias Apalodimas]

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();
+	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;
 }
-- 
2.31.0

[PATCH] efi_loader: EFI TCG2 free efi memory on protocol failure

[Heinrich Schuchardt]

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;
>  }
>

[PATCH] efi_loader: EFI TCG2 free efi memory on protocol failure

[Ilias Apalodimas]

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;
> >  }
> >
> 

[PATCH] efi_loader: EFI TCG2 free efi memory on protocol failure

[Ilias Apalodimas]

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>
---
changes since v2:
- remove the table using efi_install_configuration_table with NULL

changes since v1:
- remove the installed config table as well
- gather all cleanups in a single function and call that instead
 lib/efi_loader/efi_tcg2.c | 40 +++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 797d6eb134f6..35ae8ed5962e 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -957,6 +957,23 @@ out:
 	return ret;
 }
 
+/**
+ * tcg2_uninit - remove the final event table and free efi memory on failures
+ */
+void tcg2_uninit(void)
+{
+	efi_status_t ret;
+
+	ret = efi_install_configuration_table(&efi_guid_final_events, NULL);
+	if (ret != EFI_SUCCESS)
+		log_err("Failed to delete final events config table\n");
+
+	efi_free_pool(event_log.buffer);
+	event_log.buffer = NULL;
+	efi_free_pool(event_log.final_buffer);
+	event_log.final_buffer = NULL;
+}
+
 /**
  * create_final_event() - Create the final event and install the config
  *			defined by the TCG EFI spec
@@ -983,10 +1000,6 @@ static efi_status_t create_final_event(void)
 	event_log.final_pos = sizeof(*final_event);
 	ret = efi_install_configuration_table(&efi_guid_final_events,
 					      final_event);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	return EFI_SUCCESS;
 out:
 	return ret;
 }
@@ -1041,8 +1054,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:
+	tcg2_uninit();
 	return ret;
 }
 
@@ -1055,23 +1072,30 @@ 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:
+	tcg2_uninit();
 	return ret;
 }
-- 
2.31.0