[v2] efi: stmm: Fix for incorrect buffer allocation and cleanups

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

Posted by Jan Kiszka 1 month, 1 week ago

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

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

Posted by Ilias Apalodimas 1 month, 1 week ago

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
>

[PATCH v2 1/4] efi: stmm: Fix incorrect buffer allocation method
[PATCH v2 2/4] efi: stmm: Do not return EFI_OUT_OF_RESOURCES on internal errors
[PATCH v2 3/4] efi: stmm: Drop unused EFI error from setup_mm_hdr arguments
[PATCH v2 4/4] efi: stmm: Drop unneeded null pointer check