* [PATCH v2 1/4] efi: stmm: Fix incorrect buffer allocation method
2025-08-23 17:22 [PATCH v2 0/4] efi: stmm: Fix for incorrect buffer allocation and cleanups Jan Kiszka
@ 2025-08-23 17:22 ` Jan Kiszka
2025-08-25 8:12 ` Ilias Apalodimas
2025-08-23 17:22 ` [PATCH v2 2/4] efi: stmm: Do not return EFI_OUT_OF_RESOURCES on internal errors Jan Kiszka
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2025-08-23 17:22 UTC (permalink / raw)
To: Ard Biesheuvel, Masahisa Kojima, Ilias Apalodimas
Cc: linux-efi, linux-kernel, Sumit Garg, Jens Wiklander, Hua Qian Li
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 | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
index f741ca279052..e15d11ed165e 100644
--- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -143,6 +143,10 @@ static efi_status_t mm_communicate(u8 *comm_buf, size_t payload_size)
return var_hdr->ret_status;
}
+#define COMM_BUF_SIZE(__payload_size) (MM_COMMUNICATE_HEADER_SIZE + \
+ MM_VARIABLE_COMMUNICATE_SIZE + \
+ (__payload_size))
+
/**
* setup_mm_hdr() - Allocate a buffer for StandAloneMM and initialize the
* header data.
@@ -173,9 +177,8 @@ 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);
+ comm_buf = alloc_pages_exact(COMM_BUF_SIZE(payload_size),
+ GFP_KERNEL | __GFP_ZERO);
if (!comm_buf) {
*ret = EFI_OUT_OF_RESOURCES;
return NULL;
@@ -239,7 +242,7 @@ static efi_status_t get_max_payload(size_t *size)
*/
*size -= 2;
out:
- kfree(comm_buf);
+ free_pages_exact(comm_buf, COMM_BUF_SIZE(payload_size));
return ret;
}
@@ -282,7 +285,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, COMM_BUF_SIZE(payload_size));
return ret;
}
@@ -347,7 +350,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, COMM_BUF_SIZE(payload_size));
return ret;
}
@@ -404,7 +407,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, COMM_BUF_SIZE(payload_size));
return ret;
}
@@ -467,7 +470,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, COMM_BUF_SIZE(payload_size));
return ret;
}
@@ -507,7 +510,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, COMM_BUF_SIZE(payload_size));
return ret;
}
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/4] efi: stmm: Fix incorrect buffer allocation method
2025-08-23 17:22 ` [PATCH v2 1/4] efi: stmm: Fix incorrect buffer allocation method Jan Kiszka
@ 2025-08-25 8:12 ` Ilias Apalodimas
0 siblings, 0 replies; 8+ messages in thread
From: Ilias Apalodimas @ 2025-08-25 8:12 UTC (permalink / raw)
To: Jan Kiszka
Cc: Ard Biesheuvel, Masahisa Kojima, linux-efi, linux-kernel,
Sumit Garg, Jens Wiklander, Hua Qian Li
Thanks Jan
On Sat, 23 Aug 2025 at 20:22, 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>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> drivers/firmware/efi/stmm/tee_stmm_efi.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> index f741ca279052..e15d11ed165e 100644
> --- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -143,6 +143,10 @@ static efi_status_t mm_communicate(u8 *comm_buf, size_t payload_size)
> return var_hdr->ret_status;
> }
>
> +#define COMM_BUF_SIZE(__payload_size) (MM_COMMUNICATE_HEADER_SIZE + \
> + MM_VARIABLE_COMMUNICATE_SIZE + \
> + (__payload_size))
> +
> /**
> * setup_mm_hdr() - Allocate a buffer for StandAloneMM and initialize the
> * header data.
> @@ -173,9 +177,8 @@ 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);
> + comm_buf = alloc_pages_exact(COMM_BUF_SIZE(payload_size),
> + GFP_KERNEL | __GFP_ZERO);
> if (!comm_buf) {
> *ret = EFI_OUT_OF_RESOURCES;
> return NULL;
> @@ -239,7 +242,7 @@ static efi_status_t get_max_payload(size_t *size)
> */
> *size -= 2;
> out:
> - kfree(comm_buf);
> + free_pages_exact(comm_buf, COMM_BUF_SIZE(payload_size));
> return ret;
> }
>
> @@ -282,7 +285,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, COMM_BUF_SIZE(payload_size));
> return ret;
> }
>
> @@ -347,7 +350,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, COMM_BUF_SIZE(payload_size));
> return ret;
> }
>
> @@ -404,7 +407,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, COMM_BUF_SIZE(payload_size));
> return ret;
> }
>
> @@ -467,7 +470,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, COMM_BUF_SIZE(payload_size));
> return ret;
> }
>
> @@ -507,7 +510,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, COMM_BUF_SIZE(payload_size));
> return ret;
> }
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/4] efi: stmm: Do not return EFI_OUT_OF_RESOURCES on internal errors
2025-08-23 17:22 [PATCH v2 0/4] efi: stmm: Fix for incorrect buffer allocation and cleanups Jan Kiszka
2025-08-23 17:22 ` [PATCH v2 1/4] efi: stmm: Fix incorrect buffer allocation method Jan Kiszka
@ 2025-08-23 17:22 ` Jan Kiszka
2025-08-25 8:14 ` Ilias Apalodimas
2025-08-23 17:22 ` [PATCH v2 3/4] efi: stmm: Drop unused EFI error from setup_mm_hdr arguments Jan Kiszka
2025-08-23 17:22 ` [PATCH v2 4/4] efi: stmm: Drop unneeded null pointer check Jan Kiszka
3 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2025-08-23 17:22 UTC (permalink / raw)
To: Ard Biesheuvel, Masahisa Kojima, Ilias Apalodimas
Cc: linux-efi, linux-kernel, Sumit Garg, Jens Wiklander, Hua Qian Li
From: Jan Kiszka <jan.kiszka@siemens.com>
When we are low on memory or when the internal API is violated, we
cannot return EFI_OUT_OF_RESOURCES. According to the UEFI standard, that
error code is either related to persistent storage used for the variable
or even not foreseen as possible error (GetVariable e.g.). Use the not
fully accurate but compliant error code EFI_DEVICE_ERROR in those cases.
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 e15d11ed165e..8501056ade8a 100644
--- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -218,7 +218,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 EFI_DEVICE_ERROR;
ret = mm_communicate(comm_buf, payload_size);
if (ret != EFI_SUCCESS)
@@ -264,7 +264,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
&comm_buf, payload_size,
SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
if (!smm_property)
- return EFI_OUT_OF_RESOURCES;
+ return EFI_DEVICE_ERROR;
memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
smm_property->name_size = name_size;
@@ -320,7 +320,7 @@ static efi_status_t tee_get_variable(u16 *name, efi_guid_t *vendor,
var_acc = setup_mm_hdr(&comm_buf, payload_size,
SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
if (!var_acc)
- return EFI_OUT_OF_RESOURCES;
+ return EFI_DEVICE_ERROR;
/* Fill in contents */
memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
@@ -386,7 +386,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 EFI_DEVICE_ERROR;
/* Fill in contents */
memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
@@ -442,7 +442,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
var_acc = setup_mm_hdr(&comm_buf, payload_size,
SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
if (!var_acc)
- return EFI_OUT_OF_RESOURCES;
+ return EFI_DEVICE_ERROR;
/*
* The API has the ability to override RO flags. If no RO check was
@@ -498,7 +498,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 EFI_DEVICE_ERROR;
mm_query_info->attr = attributes;
ret = mm_communicate(comm_buf, payload_size);
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] efi: stmm: Do not return EFI_OUT_OF_RESOURCES on internal errors
2025-08-23 17:22 ` [PATCH v2 2/4] efi: stmm: Do not return EFI_OUT_OF_RESOURCES on internal errors Jan Kiszka
@ 2025-08-25 8:14 ` Ilias Apalodimas
0 siblings, 0 replies; 8+ messages in thread
From: Ilias Apalodimas @ 2025-08-25 8:14 UTC (permalink / raw)
To: Jan Kiszka
Cc: Ard Biesheuvel, Masahisa Kojima, linux-efi, linux-kernel,
Sumit Garg, Jens Wiklander, Hua Qian Li
On Sat, 23 Aug 2025 at 20:22, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> When we are low on memory or when the internal API is violated, we
> cannot return EFI_OUT_OF_RESOURCES. According to the UEFI standard, that
> error code is either related to persistent storage used for the variable
> or even not foreseen as possible error (GetVariable e.g.). Use the not
> fully accurate but compliant error code EFI_DEVICE_ERROR in those cases.
Yea it's not ideal, but we are still limited by the EFI spec
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> 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 e15d11ed165e..8501056ade8a 100644
> --- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -218,7 +218,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 EFI_DEVICE_ERROR;
>
> ret = mm_communicate(comm_buf, payload_size);
> if (ret != EFI_SUCCESS)
> @@ -264,7 +264,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
> &comm_buf, payload_size,
> SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
> if (!smm_property)
> - return EFI_OUT_OF_RESOURCES;
> + return EFI_DEVICE_ERROR;
>
> memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
> smm_property->name_size = name_size;
> @@ -320,7 +320,7 @@ static efi_status_t tee_get_variable(u16 *name, efi_guid_t *vendor,
> var_acc = setup_mm_hdr(&comm_buf, payload_size,
> SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> if (!var_acc)
> - return EFI_OUT_OF_RESOURCES;
> + return EFI_DEVICE_ERROR;
>
> /* Fill in contents */
> memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> @@ -386,7 +386,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 EFI_DEVICE_ERROR;
>
> /* Fill in contents */
> memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
> @@ -442,7 +442,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> var_acc = setup_mm_hdr(&comm_buf, payload_size,
> SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> if (!var_acc)
> - return EFI_OUT_OF_RESOURCES;
> + return EFI_DEVICE_ERROR;
>
> /*
> * The API has the ability to override RO flags. If no RO check was
> @@ -498,7 +498,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 EFI_DEVICE_ERROR;
>
> mm_query_info->attr = attributes;
> ret = mm_communicate(comm_buf, payload_size);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/4] efi: stmm: Drop unused EFI error from setup_mm_hdr arguments
2025-08-23 17:22 [PATCH v2 0/4] efi: stmm: Fix for incorrect buffer allocation and cleanups Jan Kiszka
2025-08-23 17:22 ` [PATCH v2 1/4] efi: stmm: Fix incorrect buffer allocation method Jan Kiszka
2025-08-23 17:22 ` [PATCH v2 2/4] efi: stmm: Do not return EFI_OUT_OF_RESOURCES on internal errors Jan Kiszka
@ 2025-08-23 17:22 ` Jan Kiszka
2025-08-25 8:17 ` Ilias Apalodimas
2025-08-23 17:22 ` [PATCH v2 4/4] efi: stmm: Drop unneeded null pointer check Jan Kiszka
3 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2025-08-23 17:22 UTC (permalink / raw)
To: Ard Biesheuvel, Masahisa Kojima, Ilias Apalodimas
Cc: linux-efi, linux-kernel, Sumit Garg, Jens Wiklander, Hua Qian Li
From: Jan Kiszka <jan.kiszka@siemens.com>
No caller evaluates it.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
drivers/firmware/efi/stmm/tee_stmm_efi.c | 25 ++++++++----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
index 8501056ade8a..c2bc8467b099 100644
--- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -154,11 +154,9 @@ static efi_status_t mm_communicate(u8 *comm_buf, size_t payload_size)
* @dptr: pointer address to store allocated buffer
* @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 payload_size, size_t func)
{
const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
struct efi_mm_communicate_header *mm_hdr;
@@ -173,16 +171,13 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
if (max_buffer_size &&
max_buffer_size < (MM_COMMUNICATE_HEADER_SIZE +
MM_VARIABLE_COMMUNICATE_SIZE + payload_size)) {
- *ret = EFI_INVALID_PARAMETER;
return NULL;
}
comm_buf = alloc_pages_exact(COMM_BUF_SIZE(payload_size),
GFP_KERNEL | __GFP_ZERO);
- if (!comm_buf) {
- *ret = EFI_OUT_OF_RESOURCES;
+ if (!comm_buf)
return NULL;
- }
mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
memcpy(&mm_hdr->header_guid, &mm_var_guid, sizeof(mm_hdr->header_guid));
@@ -192,7 +187,6 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
var_hdr->function = func;
if (dptr)
*dptr = comm_buf;
- *ret = EFI_SUCCESS;
return var_hdr->data;
}
@@ -215,8 +209,7 @@ static efi_status_t get_max_payload(size_t *size)
payload_size = sizeof(*var_payload);
var_payload = setup_mm_hdr(&comm_buf, payload_size,
- SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
- &ret);
+ SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE);
if (!var_payload)
return EFI_DEVICE_ERROR;
@@ -262,7 +255,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
smm_property = setup_mm_hdr(
&comm_buf, payload_size,
- SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
+ SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET);
if (!smm_property)
return EFI_DEVICE_ERROR;
@@ -318,7 +311,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,
- SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
+ SMM_VARIABLE_FUNCTION_GET_VARIABLE);
if (!var_acc)
return EFI_DEVICE_ERROR;
@@ -383,8 +376,7 @@ static efi_status_t tee_get_next_variable(unsigned long *name_size,
payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
var_getnext = setup_mm_hdr(&comm_buf, payload_size,
- SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
- &ret);
+ SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME);
if (!var_getnext)
return EFI_DEVICE_ERROR;
@@ -440,7 +432,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
* the properties, if the allocation fails
*/
var_acc = setup_mm_hdr(&comm_buf, payload_size,
- SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
+ SMM_VARIABLE_FUNCTION_SET_VARIABLE);
if (!var_acc)
return EFI_DEVICE_ERROR;
@@ -495,8 +487,7 @@ static efi_status_t tee_query_variable_info(u32 attributes,
payload_size = sizeof(*mm_query_info);
mm_query_info = setup_mm_hdr(&comm_buf, payload_size,
- SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
- &ret);
+ SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO);
if (!mm_query_info)
return EFI_DEVICE_ERROR;
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/4] efi: stmm: Drop unused EFI error from setup_mm_hdr arguments
2025-08-23 17:22 ` [PATCH v2 3/4] efi: stmm: Drop unused EFI error from setup_mm_hdr arguments Jan Kiszka
@ 2025-08-25 8:17 ` Ilias Apalodimas
0 siblings, 0 replies; 8+ messages in thread
From: Ilias Apalodimas @ 2025-08-25 8:17 UTC (permalink / raw)
To: Jan Kiszka
Cc: Ard Biesheuvel, Masahisa Kojima, linux-efi, linux-kernel,
Sumit Garg, Jens Wiklander, Hua Qian Li
Hi Jan
On Sat, 23 Aug 2025 at 20:22, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> No caller evaluates it.
We need a better description on this one, other than that I am fine
with the patch.
Callers were never evaluating the ret, they just used the return value
in case of an error
With that fixed
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> drivers/firmware/efi/stmm/tee_stmm_efi.c | 25 ++++++++----------------
> 1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> index 8501056ade8a..c2bc8467b099 100644
> --- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -154,11 +154,9 @@ static efi_status_t mm_communicate(u8 *comm_buf, size_t payload_size)
> * @dptr: pointer address to store allocated buffer
> * @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 payload_size, size_t func)
> {
> const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
> struct efi_mm_communicate_header *mm_hdr;
> @@ -173,16 +171,13 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
> if (max_buffer_size &&
> max_buffer_size < (MM_COMMUNICATE_HEADER_SIZE +
> MM_VARIABLE_COMMUNICATE_SIZE + payload_size)) {
> - *ret = EFI_INVALID_PARAMETER;
> return NULL;
> }
>
> comm_buf = alloc_pages_exact(COMM_BUF_SIZE(payload_size),
> GFP_KERNEL | __GFP_ZERO);
> - if (!comm_buf) {
> - *ret = EFI_OUT_OF_RESOURCES;
> + if (!comm_buf)
> return NULL;
> - }
>
> mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
> memcpy(&mm_hdr->header_guid, &mm_var_guid, sizeof(mm_hdr->header_guid));
> @@ -192,7 +187,6 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
> var_hdr->function = func;
> if (dptr)
> *dptr = comm_buf;
> - *ret = EFI_SUCCESS;
>
> return var_hdr->data;
> }
> @@ -215,8 +209,7 @@ static efi_status_t get_max_payload(size_t *size)
>
> payload_size = sizeof(*var_payload);
> var_payload = setup_mm_hdr(&comm_buf, payload_size,
> - SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
> - &ret);
> + SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE);
> if (!var_payload)
> return EFI_DEVICE_ERROR;
>
> @@ -262,7 +255,7 @@ static efi_status_t get_property_int(u16 *name, size_t name_size,
>
> smm_property = setup_mm_hdr(
> &comm_buf, payload_size,
> - SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
> + SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET);
> if (!smm_property)
> return EFI_DEVICE_ERROR;
>
> @@ -318,7 +311,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,
> - SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> + SMM_VARIABLE_FUNCTION_GET_VARIABLE);
> if (!var_acc)
> return EFI_DEVICE_ERROR;
>
> @@ -383,8 +376,7 @@ static efi_status_t tee_get_next_variable(unsigned long *name_size,
>
> payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
> var_getnext = setup_mm_hdr(&comm_buf, payload_size,
> - SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> - &ret);
> + SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME);
> if (!var_getnext)
> return EFI_DEVICE_ERROR;
>
> @@ -440,7 +432,7 @@ static efi_status_t tee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> * the properties, if the allocation fails
> */
> var_acc = setup_mm_hdr(&comm_buf, payload_size,
> - SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> + SMM_VARIABLE_FUNCTION_SET_VARIABLE);
> if (!var_acc)
> return EFI_DEVICE_ERROR;
>
> @@ -495,8 +487,7 @@ static efi_status_t tee_query_variable_info(u32 attributes,
>
> payload_size = sizeof(*mm_query_info);
> mm_query_info = setup_mm_hdr(&comm_buf, payload_size,
> - SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
> - &ret);
> + SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO);
> if (!mm_query_info)
> return EFI_DEVICE_ERROR;
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 4/4] efi: stmm: Drop unneeded null pointer check
2025-08-23 17:22 [PATCH v2 0/4] efi: stmm: Fix for incorrect buffer allocation and cleanups Jan Kiszka
` (2 preceding siblings ...)
2025-08-23 17:22 ` [PATCH v2 3/4] efi: stmm: Drop unused EFI error from setup_mm_hdr arguments Jan Kiszka
@ 2025-08-23 17:22 ` Jan Kiszka
3 siblings, 0 replies; 8+ messages in thread
From: Jan Kiszka @ 2025-08-23 17:22 UTC (permalink / raw)
To: Ard Biesheuvel, Masahisa Kojima, Ilias Apalodimas
Cc: linux-efi, linux-kernel, Sumit Garg, Jens Wiklander, Hua Qian Li
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>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
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 c2bc8467b099..65c0fe1ba275 100644
--- a/drivers/firmware/efi/stmm/tee_stmm_efi.c
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -185,8 +185,7 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func)
var_hdr = (struct smm_variable_communicate_header *)mm_hdr->data;
var_hdr->function = func;
- if (dptr)
- *dptr = comm_buf;
+ *dptr = comm_buf;
return var_hdr->data;
}
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread