Re: [PATCH v2 1/6] efi_loader: stop recursion in efi_init_secure_state

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Thu, Aug 26, 2021 at 03:48:00PM +0200, Heinrich Schuchardt wrote:
> efi_init_secure_state() calls efi_transfer_secure_state() which may delete
> variable "PK" which will result in calling efi_init_secure_state() again.

I don't think it is a right thing to do. So I would say nak to this version.
When I first implemented those functions, I intended to call
efi_init_secure_state() only at the system initialization.
Later on, all the transitions should be managed by efi_transfer_secure_state()
as well as its callers.

Calling efi_init_secure_state() in efi_set_variable_int() is a bad idea.
(then you see 'recursion'.)
I will explain more in your patch#5.

-Takahiro Akashi


> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	no change
> ---
>  lib/efi_loader/efi_var_common.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index 3d92afe2eb..654ce81f9d 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -314,11 +314,15 @@ err:
>  
>  efi_status_t efi_init_secure_state(void)
>  {
> +	static bool lock;
>  	enum efi_secure_mode mode = EFI_MODE_SETUP;
>  	u8 efi_vendor_keys = 0;
>  	efi_uintn_t size = 0;
>  	efi_status_t ret;
>  
> +	if (lock)
> +		return EFI_SUCCESS;
> +
>  	ret = efi_get_variable_int(L"PK", &efi_global_variable_guid,
>  				   NULL, &size, NULL, NULL);
>  	if (ret == EFI_BUFFER_TOO_SMALL) {
> @@ -326,7 +330,9 @@ efi_status_t efi_init_secure_state(void)
>  			mode = EFI_MODE_USER;
>  	}
>  
> +	lock = true;
>  	ret = efi_transfer_secure_state(mode);
> +	lock = false;
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> -- 
> 2.30.2
>