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.0Hi 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 >
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
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
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
© 2016 - 2025 Red Hat, Inc.