[PATCH 0/3] efi: stmm: Fix for incorrect buffer allocation and cleanups

[PATCH 0/3] efi: stmm: Fix for incorrect buffer allocation and cleanups

[Jan Kiszka]

One critical fix for the EFI StMM driver and two smaller cleanups.

I'm carefully optimistic that we will get better test coverage in the
future: I have a working RPMB model for QEMU, and this is where the
crash was found now and debugged in.

Jan

Jan Kiszka (3):
  efi: stmm: Fix incorrect buffer allocation method
  efi: stmm: Use EFI return code of setup_mm_hdr
  efi: stmm: Drop unneeded null pointer check

 drivers/firmware/efi/stmm/tee_stmm_efi.c | 59 ++++++++++++++----------
 1 file changed, 34 insertions(+), 25 deletions(-)

-- 
2.43.0

[PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The communication buffer allocated by setup_mm_hdr is later on passed to
tee_shm_register_kernel_buf. The latter expects those buffers to be
contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
was broken before as well.

Fix this by using alloc_pages_exact instead of kmalloc.

Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/firmware/efi/stmm/tee_stmm_efi.c | 44 +++++++++++++++---------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
index f741ca279052..706ba095a4ba 100644
--- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -148,13 +148,14 @@ static efi_status_t mm_communicate(u8 *comm_buf, size_t payload_size)
  *			header data.
  *
  * @dptr:		pointer address to store allocated buffer
+ * @nr_pages:		pointer address to store number of allocated pages
  * @payload_size:	payload size
  * @func:		standAloneMM function number
  * @ret:		EFI return code
  * Return:		pointer to corresponding StandAloneMM function buffer or NULL
  */
-static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
-			  efi_status_t *ret)
+static void *setup_mm_hdr(u8 **dptr, size_t *nr_pages, size_t payload_size,
+			  size_t func, efi_status_t *ret)
 {
 	const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
 	struct efi_mm_communicate_header *mm_hdr;
@@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
 		return NULL;
 	}
 
-	comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
-				   MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
-			   GFP_KERNEL);
+	*nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
+			    MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
+			    PAGE_SIZE) / PAGE_SIZE;
+
+	comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
+				     GFP_KERNEL | __GFP_ZERO);
 	if (!comm_buf) {
 		*ret = EFI_OUT_OF_RESOURCES;
 		return NULL;
@@ -205,13 +209,14 @@ static efi_status_t get_max_payload(size_t *size)
 	struct smm_variable_payload_size *var_payload = NULL;
 	size_t payload_size;
 	u8 *comm_buf = NULL;
+	size_t nr_pages;
 	efi_status_t ret;
 
 	if (!size)
 		return EFI_INVALID_PARAMETER;
 
 	payload_size = sizeof(*var_payload);
-	var_payload = setup_mm_hdr(&comm_buf, payload_size,
+	var_payload = setup_mm_hdr(&comm_buf, &nr_pages, payload_size,
 				   SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
 				   &ret);
 	if (!var_payload)
@@ -239,7 +244,7 @@ static efi_status_t get_max_payload(size_t *size)
 	 */
 	*size -= 2;
 out:
-	kfree(comm_buf);
+	free_pages_exact(comm_buf, nr_pages * PAGE_SIZE);
 	return ret;
 }
 
@@ -250,6 +255,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
 	struct smm_variable_var_check_property *smm_property;
 	size_t payload_size;
 	u8 *comm_buf = NULL;
+	size_t nr_pages;
 	efi_status_t ret;
 
 	memset(var_property, 0, sizeof(*var_property));
@@ -258,7 +264,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
 		return EFI_INVALID_PARAMETER;
 
 	smm_property = setup_mm_hdr(
-		&comm_buf, payload_size,
+		&comm_buf, &nr_pages, payload_size,
 		SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
 	if (!smm_property)
 		return EFI_OUT_OF_RESOURCES;
@@ -282,7 +288,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
 	memcpy(var_property, &smm_property->property, sizeof(*var_property));
 
 out:
-	kfree(comm_buf);
+	free_pages_exact(comm_buf, nr_pages * PAGE_SIZE);
 	return ret;
 }
 
@@ -296,6 +302,7 @@ static efi_status_t tee_get_variable(u16 *name, efi_guid_t *vendor,
 	size_t name_size;
 	size_t tmp_dsize;
 	u8 *comm_buf = NULL;
+	size_t nr_pages;
 	efi_status_t ret;
 
 	if (!name || !vendor || !data_size)
@@ -314,7 +321,7 @@ static efi_status_t tee_get_variable(u16 *name, efi_guid_t *vendor,
 	}
 
 	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
-	var_acc = setup_mm_hdr(&comm_buf, payload_size,
+	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;
@@ -347,7 +354,7 @@ static efi_status_t tee_get_variable(u16 *name, efi_guid_t *vendor,
 	memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
 	       var_acc->data_size);
 out:
-	kfree(comm_buf);
+	free_pages_exact(comm_buf, nr_pages * PAGE_SIZE);
 	return ret;
 }
 
@@ -359,6 +366,7 @@ static efi_status_t tee_get_next_variable(unsigned long *name_size,
 	size_t out_name_size;
 	size_t in_name_size;
 	u8 *comm_buf = NULL;
+	size_t nr_pages;
 	efi_status_t ret;
 
 	if (!name_size || !name || !guid)
@@ -379,7 +387,7 @@ static efi_status_t tee_get_next_variable(unsigned long *name_size,
 			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
 
 	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
-	var_getnext = setup_mm_hdr(&comm_buf, payload_size,
+	var_getnext = setup_mm_hdr(&comm_buf, &nr_pages, payload_size,
 				   SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
 				   &ret);
 	if (!var_getnext)
@@ -404,7 +412,7 @@ static efi_status_t tee_get_next_variable(unsigned long *name_size,
 	memcpy(name, var_getnext->name, var_getnext->name_size);
 
 out:
-	kfree(comm_buf);
+	free_pages_exact(comm_buf, nr_pages * PAGE_SIZE);
 	return ret;
 }
 
@@ -418,6 +426,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	size_t payload_size;
 	size_t name_size;
 	u8 *comm_buf = NULL;
+	size_t nr_pages;
 
 	if (!name || name[0] == 0 || !vendor)
 		return EFI_INVALID_PARAMETER;
@@ -436,7 +445,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	 * so we won't need to account for any failures in reading/setting
 	 * the properties, if the allocation fails
 	 */
-	var_acc = setup_mm_hdr(&comm_buf, payload_size,
+	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;
@@ -467,7 +476,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	ret = mm_communicate(comm_buf, payload_size);
 	dev_dbg(pvt_data.dev, "Set Variable %s %d %lx\n", __FILE__, __LINE__, ret);
 out:
-	kfree(comm_buf);
+	free_pages_exact(comm_buf, nr_pages * PAGE_SIZE);
 	return ret;
 }
 
@@ -489,9 +498,10 @@ static efi_status_t tee_query_variable_info(u32 attributes,
 	size_t payload_size;
 	efi_status_t ret;
 	u8 *comm_buf;
+	size_t nr_pages;
 
 	payload_size = sizeof(*mm_query_info);
-	mm_query_info = setup_mm_hdr(&comm_buf, payload_size,
+	mm_query_info = setup_mm_hdr(&comm_buf, &nr_pages, payload_size,
 				SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
 				&ret);
 	if (!mm_query_info)
@@ -507,7 +517,7 @@ static efi_status_t tee_query_variable_info(u32 attributes,
 	*max_variable_size = mm_query_info->max_variable_size;
 
 out:
-	kfree(comm_buf);
+	free_pages_exact(comm_buf, nr_pages * PAGE_SIZE);
 	return ret;
 }
 
-- 
2.43.0

[PATCH 2/3] efi: stmm: Use EFI return code of setup_mm_hdr

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.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

[PATCH 3/3] efi: stmm: Drop unneeded null pointer check

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The API documenation of setup_mm_hdr does not mention that dptr can be
NULL, this is a local function, and no caller passes NULL. So drop the
unneeded check.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/firmware/efi/stmm/tee_stmm_efi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
index bf992b42be70..ff41667b1005 100644
--- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -191,8 +191,7 @@ static void *setup_mm_hdr(u8 **dptr, size_t *nr_pages, size_t payload_size,
 
 	var_hdr = (struct smm_variable_communicate_header *)mm_hdr->data;
 	var_hdr->function = func;
-	if (dptr)
-		*dptr = comm_buf;
+	*dptr = comm_buf;
 	*ret = EFI_SUCCESS;
 
 	return var_hdr->data;
-- 
2.43.0

Re: [PATCH 3/3] efi: stmm: Drop unneeded null pointer check

[Ilias Apalodimas]

Hi Jan

On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> The API documenation of setup_mm_hdr does not mention that dptr can be
> NULL, this is a local function, and no caller passes NULL. So drop the
> unneeded check.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/firmware/efi/stmm/tee_stmm_efi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> index bf992b42be70..ff41667b1005 100644
> --- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -191,8 +191,7 @@ static void *setup_mm_hdr(u8 **dptr, size_t *nr_pages, size_t payload_size,
>
>         var_hdr = (struct smm_variable_communicate_header *)mm_hdr->data;
>         var_hdr->function = func;
> -       if (dptr)
> -               *dptr = comm_buf;
> +       *dptr = comm_buf;

I think this is ok, eventually tee_mm_communicate() will check for a
NULL ptr and return EFI_INVALID_PARAMETER;

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

>         *ret = EFI_SUCCESS;
>
>         return var_hdr->data;
> --
> 2.43.0
>

Re: [PATCH 2/3] efi: stmm: Use EFI return code of setup_mm_hdr

[Ilias Apalodimas]

Hi Jan

On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.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@siemens.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
>

Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

[Ilias Apalodimas]

(++cc Sumit and Kojima-san on their updated emails)

On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> The communication buffer allocated by setup_mm_hdr is later on passed to
> tee_shm_register_kernel_buf. The latter expects those buffers to be
> contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
> various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
> was broken before as well.
>
> Fix this by using alloc_pages_exact instead of kmalloc.
>
> Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---

[...]

>         const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
>         struct efi_mm_communicate_header *mm_hdr;
> @@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
>                 return NULL;
>         }
>
> -       comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
> -                                  MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> -                          GFP_KERNEL);
> +       *nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
> +                           MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> +                           PAGE_SIZE) / PAGE_SIZE;
> +
> +       comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
> +                                    GFP_KERNEL | __GFP_ZERO);

Rename nr_pages to something else and skip division, multiplying.
Unless there's a reason I am missing?
Also doesn't alloc_pages_exact() already rounds things up?

>         if (!comm_buf) {
>                 *ret = EFI_OUT_OF_RESOURCES;
>                 return NULL;
> @@ -205,13 +209,14 @@ static efi_status_t get_max_payload(size_t *size)
>         struct smm_variable_payload_size *var_payload = NULL;
>         size_t payload_size;
>         u8 *comm_buf = NULL;

[...]

Thanks
/Ilias

Re: [PATCH 2/3] efi: stmm: Use EFI return code of setup_mm_hdr

[Jan Kiszka]

On 20.08.25 09:10, Ilias Apalodimas wrote:
> Hi Jan
> 
> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.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.

And with storage space is likely meant the persistent part of it. ENOMEM
is something different.

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

EFI_DEVICE_ERROR? The "hardware" is has a problem by not providing us
enough RAM. Yeah, not optimal either. But invalid parameter is clearly
described, and nothing fits.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

[Jan Kiszka]

On 20.08.25 09:29, Ilias Apalodimas wrote:
> (++cc Sumit and Kojima-san on their updated emails)
> 
> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The communication buffer allocated by setup_mm_hdr is later on passed to
>> tee_shm_register_kernel_buf. The latter expects those buffers to be
>> contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
>> various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
>> was broken before as well.
>>
>> Fix this by using alloc_pages_exact instead of kmalloc.
>>
>> Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
> 
> [...]
> 
>>         const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
>>         struct efi_mm_communicate_header *mm_hdr;
>> @@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
>>                 return NULL;
>>         }
>>
>> -       comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
>> -                                  MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
>> -                          GFP_KERNEL);
>> +       *nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
>> +                           MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
>> +                           PAGE_SIZE) / PAGE_SIZE;
>> +
>> +       comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
>> +                                    GFP_KERNEL | __GFP_ZERO);
> 
> Rename nr_pages to something else and skip division, multiplying.
> Unless there's a reason I am missing?
> Also doesn't alloc_pages_exact() already rounds things up?

I was looking at tee_dyn_shm_alloc_helper and the dance it does to
calculate the pages from the size parameter. Digging into
alloc_pages_exact, though, suggests that the get_order(size) there will
already do what we need. Let me rework this.

Jan

> 
>>         if (!comm_buf) {
>>                 *ret = EFI_OUT_OF_RESOURCES;
>>                 return NULL;
>> @@ -205,13 +209,14 @@ static efi_status_t get_max_payload(size_t *size)
>>         struct smm_variable_payload_size *var_payload = NULL;
>>         size_t payload_size;
>>         u8 *comm_buf = NULL;
> 
> [...]
> 
> Thanks
> /Ilias


-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

[Sumit Garg]

Hi Jan,

On Wed, Aug 20, 2025 at 05:05:43PM +0200, Jan Kiszka wrote:
> On 20.08.25 09:29, Ilias Apalodimas wrote:
> > (++cc Sumit and Kojima-san on their updated emails)
> > 
> > On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> The communication buffer allocated by setup_mm_hdr is later on passed to
> >> tee_shm_register_kernel_buf. The latter expects those buffers to be
> >> contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
> >> various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
> >> was broken before as well.
> >>
> >> Fix this by using alloc_pages_exact instead of kmalloc.
> >>
> >> Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> > 
> > [...]
> > 
> >>         const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
> >>         struct efi_mm_communicate_header *mm_hdr;
> >> @@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
> >>                 return NULL;
> >>         }
> >>
> >> -       comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
> >> -                                  MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> >> -                          GFP_KERNEL);
> >> +       *nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
> >> +                           MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> >> +                           PAGE_SIZE) / PAGE_SIZE;
> >> +
> >> +       comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
> >> +                                    GFP_KERNEL | __GFP_ZERO);
> > 
> > Rename nr_pages to something else and skip division, multiplying.
> > Unless there's a reason I am missing?
> > Also doesn't alloc_pages_exact() already rounds things up?
> 
> I was looking at tee_dyn_shm_alloc_helper and the dance it does to
> calculate the pages from the size parameter.

The rework of tee_shm_register_kernel_buf() is directly accept kernel
pages instead of buffer pointers is already due. If you are willing to
fix existing TEE client drivers and the API then I will be happy to
review them.

-Sumit

Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

[Jan Kiszka]

On 21.08.25 11:35, Sumit Garg wrote:
> Hi Jan,
> 
> On Wed, Aug 20, 2025 at 05:05:43PM +0200, Jan Kiszka wrote:
>> On 20.08.25 09:29, Ilias Apalodimas wrote:
>>> (++cc Sumit and Kojima-san on their updated emails)
>>>
>>> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The communication buffer allocated by setup_mm_hdr is later on passed to
>>>> tee_shm_register_kernel_buf. The latter expects those buffers to be
>>>> contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
>>>> various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
>>>> was broken before as well.
>>>>
>>>> Fix this by using alloc_pages_exact instead of kmalloc.
>>>>
>>>> Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>
>>> [...]
>>>
>>>>         const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
>>>>         struct efi_mm_communicate_header *mm_hdr;
>>>> @@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
>>>>                 return NULL;
>>>>         }
>>>>
>>>> -       comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
>>>> -                                  MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
>>>> -                          GFP_KERNEL);
>>>> +       *nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
>>>> +                           MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
>>>> +                           PAGE_SIZE) / PAGE_SIZE;
>>>> +
>>>> +       comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
>>>> +                                    GFP_KERNEL | __GFP_ZERO);
>>>
>>> Rename nr_pages to something else and skip division, multiplying.
>>> Unless there's a reason I am missing?
>>> Also doesn't alloc_pages_exact() already rounds things up?
>>
>> I was looking at tee_dyn_shm_alloc_helper and the dance it does to
>> calculate the pages from the size parameter.
> 
> The rework of tee_shm_register_kernel_buf() is directly accept kernel
> pages instead of buffer pointers is already due. If you are willing to
> fix existing TEE client drivers and the API then I will be happy to
> review them.

I'm currently testing the stmm quite a bit but I have no setup/use case
for the trusted_tee so far. Testing is eating most of the time,
specifically with these seriously complex firmware security stacks.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH 2/3] efi: stmm: Use EFI return code of setup_mm_hdr

[Jan Kiszka]

On 20.08.25 16:59, Jan Kiszka wrote:
> On 20.08.25 09:10, Ilias Apalodimas wrote:
>> Hi Jan
>>
>> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>> From: Jan Kiszka <jan.kiszka@siemens.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.
> 
> And with storage space is likely meant the persistent part of it. ENOMEM
> is something different.
> 
>>
>> Should we also change setup_mm_hdr() and return EFI_INVALID_PARAMETER
>> always? It's still not ideal, but much closer to the spec.
> 
> EFI_DEVICE_ERROR? The "hardware" is has a problem by not providing us
> enough RAM. Yeah, not optimal either. But invalid parameter is clearly
> described, and nothing fits.
> 

If there are no concerns, I will switch to EFI_DEVICE_ERROR and even
drop the error "ret" argument in v2.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH 2/3] efi: stmm: Use EFI return code of setup_mm_hdr

[Ilias Apalodimas]

On Thu, 21 Aug 2025 at 16:00, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 20.08.25 16:59, Jan Kiszka wrote:
> > On 20.08.25 09:10, Ilias Apalodimas wrote:
> >> Hi Jan
> >>
> >> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>
> >>> From: Jan Kiszka <jan.kiszka@siemens.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.
> >
> > And with storage space is likely meant the persistent part of it. ENOMEM
> > is something different.
> >
> >>
> >> Should we also change setup_mm_hdr() and return EFI_INVALID_PARAMETER
> >> always? It's still not ideal, but much closer to the spec.
> >
> > EFI_DEVICE_ERROR? The "hardware" is has a problem by not providing us
> > enough RAM. Yeah, not optimal either. But invalid parameter is clearly
> > described, and nothing fits.
> >
>
> If there are no concerns, I will switch to EFI_DEVICE_ERROR and even
> drop the error "ret" argument in v2.

Yea, I don't think there's an ideal scenario and the EFI spec doesn't
cover the case where some allocation failed, but please add this info
on the commit message.

Thanks
/Ilias
>
> Jan
>
> --
> Siemens AG, Foundational Technologies
> Linux Expert Center

Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

[Sumit Garg]

On Thu, Aug 21, 2025 at 02:56:59PM +0200, Jan Kiszka wrote:
> On 21.08.25 11:35, Sumit Garg wrote:
> > Hi Jan,
> > 
> > On Wed, Aug 20, 2025 at 05:05:43PM +0200, Jan Kiszka wrote:
> >> On 20.08.25 09:29, Ilias Apalodimas wrote:
> >>> (++cc Sumit and Kojima-san on their updated emails)
> >>>
> >>> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> The communication buffer allocated by setup_mm_hdr is later on passed to
> >>>> tee_shm_register_kernel_buf. The latter expects those buffers to be
> >>>> contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
> >>>> various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
> >>>> was broken before as well.
> >>>>
> >>>> Fix this by using alloc_pages_exact instead of kmalloc.
> >>>>
> >>>> Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> ---
> >>>
> >>> [...]
> >>>
> >>>>         const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
> >>>>         struct efi_mm_communicate_header *mm_hdr;
> >>>> @@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
> >>>>                 return NULL;
> >>>>         }
> >>>>
> >>>> -       comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
> >>>> -                                  MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> >>>> -                          GFP_KERNEL);
> >>>> +       *nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
> >>>> +                           MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> >>>> +                           PAGE_SIZE) / PAGE_SIZE;
> >>>> +
> >>>> +       comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
> >>>> +                                    GFP_KERNEL | __GFP_ZERO);
> >>>
> >>> Rename nr_pages to something else and skip division, multiplying.
> >>> Unless there's a reason I am missing?
> >>> Also doesn't alloc_pages_exact() already rounds things up?
> >>
> >> I was looking at tee_dyn_shm_alloc_helper and the dance it does to
> >> calculate the pages from the size parameter.
> > 
> > The rework of tee_shm_register_kernel_buf() to directly accept kernel
> > pages instead of buffer pointers is already due. If you are willing to
> > fix existing TEE client drivers and the API then I will be happy to
> > review them.
> 
> I'm currently testing the stmm quite a bit but I have no setup/use case
> for the trusted_tee so far. Testing is eating most of the time,
> specifically with these seriously complex firmware security stacks.

For TEE based trusted keys, it is rather a bit straigtforward to run
tests using OP-TEE Qemu build setup where you would only need to patch
the kernel.

Test command:

$ make -j$(nproc) CHECK_TESTS="trusted-keys" check

-Sumit