linux-kernel - Re: [PATCH 2/3] efi: stmm: Use EFI return code of setup_mm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAC_iWjLMFGp3wg=59PruJQb7heds6CUcy8FMZ_cdT0b2vC5a3g@mail.gmail.com>
Date: Wed, 20 Aug 2025 10:10:21 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Jan Kiszka <jan.kiszka@...mens.com>
Cc: Ard Biesheuvel <ardb@...nel.org>, Masahisa Kojima <masahisa.kojima@...aro.org>, 
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Sumit Garg <sumit.garg@...aro.org>, Jens Wiklander <jens.wiklander@...aro.org>
Subject: Re: [PATCH 2/3] efi: stmm: Use EFI return code of setup_mm_hdr

Hi Jan

On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@...mens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@...mens.com>
>
> If a too large payload_size is passed to setup_mm_hdr, callers will
> returned EFI_OUT_OF_RESOURCES rather than EFI_INVALID_PARAMETER that is
> passed down via ret. No need to fold errors here.

Apart from not folding the error here, the current code kind of
violates the EFI spec.
If you look at GetVariable, GetNextVariable, SetVariable, and
QueryVariableInfo only SetVariable is supposed to return
EFI_OUT_OF_RESOURCES, if there's no storage space left.

Should we also change setup_mm_hdr() and return EFI_INVALID_PARAMETER
always? It's still not ideal, but much closer to the spec.

Cheers
/Ilias
>
> Signed-off-by: Jan Kiszka <jan.kiszka@...mens.com>
> ---
>  drivers/firmware/efi/stmm/tee_stmm_efi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> index 706ba095a4ba..bf992b42be70 100644
> --- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -220,7 +220,7 @@ static efi_status_t get_max_payload(size_t *size)
>                                    SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
>                                    &ret);
>         if (!var_payload)
> -               return EFI_OUT_OF_RESOURCES;
> +               return ret;
>
>         ret = mm_communicate(comm_buf, payload_size);
>         if (ret != EFI_SUCCESS)
> @@ -267,7 +267,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
>                 &comm_buf, &nr_pages, payload_size,
>                 SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
>         if (!smm_property)
> -               return EFI_OUT_OF_RESOURCES;
> +               return ret;
>
>         memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
>         smm_property->name_size = name_size;
> @@ -324,7 +324,7 @@ static efi_status_t tee_get_variable(u16 *name, efi_guid_t *vendor,
>         var_acc = setup_mm_hdr(&comm_buf, &nr_pages, payload_size,
>                                SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
>         if (!var_acc)
> -               return EFI_OUT_OF_RESOURCES;
> +               return ret;
>
>         /* Fill in contents */
>         memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> @@ -391,7 +391,7 @@ static efi_status_t tee_get_next_variable(unsigned long *name_size,
>                                    SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
>                                    &ret);
>         if (!var_getnext)
> -               return EFI_OUT_OF_RESOURCES;
> +               return ret;
>
>         /* Fill in contents */
>         memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
> @@ -448,7 +448,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
>         var_acc = setup_mm_hdr(&comm_buf, &nr_pages, payload_size,
>                                SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
>         if (!var_acc)
> -               return EFI_OUT_OF_RESOURCES;
> +               return ret;
>
>         /*
>          * The API has the ability to override RO flags. If no RO check was
> @@ -505,7 +505,7 @@ static efi_status_t tee_query_variable_info(u32 attributes,
>                                 SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
>                                 &ret);
>         if (!mm_query_info)
> -               return EFI_OUT_OF_RESOURCES;
> +               return ret;
>
>         mm_query_info->attr = attributes;
>         ret = mm_communicate(comm_buf, payload_size);
> --
> 2.43.0
>