efi_loader: EFI TCG2 free efi memory on protocol failure

Message ID 20210324142353.1216042-1-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series
  • efi_loader: EFI TCG2 free efi memory on protocol failure
Related show

Commit Message

Ilias Apalodimas March 24, 2021, 2:23 p.m.
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

Comments

Heinrich Schuchardt March 25, 2021, 10:10 a.m. | #1
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;

>  }

>
Ilias Apalodimas March 25, 2021, 10:31 a.m. | #2
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

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