* [PATCH 0/4] efi_loader: centralize known vendor GUIDs
@ 2021-09-11 7:28 Heinrich Schuchardt
2021-09-11 7:28 ` [PATCH 1/4] efi_loader: treat UEFI variable name as const Heinrich Schuchardt
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2021-09-11 7:28 UTC (permalink / raw)
To: u-boot
Cc: Ilias Apalodimas, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima,
Alexander Graf, Heinrich Schuchardt
The UEFI specification defines which vendor GUIDs should be used for
predefined variables like 'PK'. Currently we have multiple places
where this relationship is stored.
With this patch series a function for retrieving the GUID is provided
and existing code is adjusted to used it.
Heinrich Schuchardt (4):
efi_loader: treat UEFI variable name as const
efi_loader: function to get GUID for variable name
efi_loader: simplify efi_sigstore_parse_sigdb()
efi_loader: simplify tcg2_measure_secure_boot_variable()
include/efi_loader.h | 2 +-
include/efi_variable.h | 24 +++++++++++++++------
lib/efi_loader/efi_signature.c | 35 ++++++-------------------------
lib/efi_loader/efi_tcg2.c | 31 +++++++++++++--------------
lib/efi_loader/efi_var_common.c | 14 +++++++++++--
lib/efi_loader/efi_var_mem.c | 7 ++++---
lib/efi_loader/efi_variable.c | 9 ++++----
lib/efi_loader/efi_variable_tee.c | 16 ++++++++------
8 files changed, 70 insertions(+), 68 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/4] efi_loader: treat UEFI variable name as const 2021-09-11 7:28 [PATCH 0/4] efi_loader: centralize known vendor GUIDs Heinrich Schuchardt @ 2021-09-11 7:28 ` Heinrich Schuchardt 2021-09-11 14:10 ` Ilias Apalodimas 2021-09-11 7:28 ` [PATCH 2/4] efi_loader: function to get GUID for variable name Heinrich Schuchardt ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Heinrich Schuchardt @ 2021-09-11 7:28 UTC (permalink / raw) To: u-boot Cc: Ilias Apalodimas, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf, Heinrich Schuchardt Adjust several internal functions to treat UEFI variable names as const. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- include/efi_loader.h | 2 +- include/efi_variable.h | 16 ++++++++++------ lib/efi_loader/efi_tcg2.c | 2 +- lib/efi_loader/efi_var_common.c | 5 +++-- lib/efi_loader/efi_var_mem.c | 7 ++++--- lib/efi_loader/efi_variable.c | 9 +++++---- lib/efi_loader/efi_variable_tee.c | 16 ++++++++++------ 7 files changed, 34 insertions(+), 23 deletions(-) diff --git a/include/efi_loader.h b/include/efi_loader.h index c440962fe5..125052d002 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -816,7 +816,7 @@ efi_status_t EFIAPI efi_query_variable_info( u64 *remaining_variable_storage_size, u64 *maximum_variable_size); -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); /* * See section 3.1.3 in the v2.7 UEFI spec for more details on diff --git a/include/efi_variable.h b/include/efi_variable.h index 0440d356bc..8f666b2309 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -32,7 +32,8 @@ enum efi_auth_var_type { * @timep: authentication time (seconds since start of epoch) * Return: status code */ -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, +efi_status_t efi_get_variable_int(const u16 *variable_name, + const efi_guid_t *vendor, u32 *attributes, efi_uintn_t *data_size, void *data, u64 *timep); @@ -47,7 +48,8 @@ efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, * @ro_check: check the read only read only bit in attributes * Return: status code */ -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, +efi_status_t efi_set_variable_int(const u16 *variable_name, + const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data, bool ro_check); @@ -224,7 +226,7 @@ void efi_var_mem_del(struct efi_var_entry *var); * @time: time of authentication (as seconds since start of epoch) * Result: status code */ -efi_status_t efi_var_mem_ins(u16 *variable_name, +efi_status_t efi_var_mem_ins(const u16 *variable_name, const efi_guid_t *vendor, u32 attributes, const efi_uintn_t size1, const void *data1, const efi_uintn_t size2, const void *data2, @@ -251,7 +253,8 @@ efi_status_t efi_init_secure_state(void); * @guid: guid of UEFI variable * Return: identifier for authentication related variables */ -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid); +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, + const efi_guid_t *guid); /** * efi_get_next_variable_name_mem() - Runtime common code across efi variable @@ -280,8 +283,9 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na * Return: status code */ efi_status_t __efi_runtime -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, - efi_uintn_t *data_size, void *data, u64 *timep); +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor, + u32 *attributes, efi_uintn_t *data_size, void *data, + u64 *timep); /** * efi_get_variable_runtime() - runtime implementation of GetVariable() diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 401acf3d4f..beb224f66a 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -1359,7 +1359,7 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev) * Return: status code */ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index, - u32 event_type, u16 *var_name, + u32 event_type, const u16 *var_name, const efi_guid_t *guid, efi_uintn_t data_size, u8 *data) { diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index a00bbf1620..e179932124 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -374,7 +374,8 @@ bool efi_secure_boot_enabled(void) return efi_secure_boot; } -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid) +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, + const efi_guid_t *guid) { for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) { if (!u16_strcmp(name, name_type[i].name) && @@ -393,7 +394,7 @@ enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid) * * Return: buffer with variable data or NULL */ -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) { efi_status_t ret; void *buf = NULL; diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c index 3d335a8274..13909b1d26 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -134,7 +134,7 @@ void __efi_runtime efi_var_mem_del(struct efi_var_entry *var) } efi_status_t __efi_runtime efi_var_mem_ins( - u16 *variable_name, + const u16 *variable_name, const efi_guid_t *vendor, u32 attributes, const efi_uintn_t size1, const void *data1, const efi_uintn_t size2, const void *data2, @@ -274,8 +274,9 @@ efi_status_t efi_var_mem_init(void) } efi_status_t __efi_runtime -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, - efi_uintn_t *data_size, void *data, u64 *timep) +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor, + u32 *attributes, efi_uintn_t *data_size, void *data, + u64 *timep) { efi_uintn_t old_size; struct efi_var_entry *var; diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index fa2b6bc7a8..5adc7f821a 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -45,7 +45,7 @@ * * Return: status code */ -static efi_status_t efi_variable_authenticate(u16 *variable, +static efi_status_t efi_variable_authenticate(const u16 *variable, const efi_guid_t *vendor, efi_uintn_t *data_size, const void **data, u32 given_attr, @@ -194,7 +194,7 @@ err: return ret; } #else -static efi_status_t efi_variable_authenticate(u16 *variable, +static efi_status_t efi_variable_authenticate(const u16 *variable, const efi_guid_t *vendor, efi_uintn_t *data_size, const void **data, u32 given_attr, @@ -205,7 +205,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable, #endif /* CONFIG_EFI_SECURE_BOOT */ efi_status_t __efi_runtime -efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, +efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, efi_uintn_t *data_size, void *data, u64 *timep) { @@ -219,7 +219,8 @@ efi_get_next_variable_name_int(efi_uintn_t *variable_name_size, return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor); } -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, +efi_status_t efi_set_variable_int(const u16 *variable_name, + const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data, bool ro_check) { diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c index 51920bcb51..281f886124 100644 --- a/lib/efi_loader/efi_variable_tee.c +++ b/lib/efi_loader/efi_variable_tee.c @@ -284,7 +284,8 @@ out: * StMM can store internal attributes and properties for variables, i.e enabling * R/O variables */ -static efi_status_t set_property_int(u16 *variable_name, efi_uintn_t name_size, +static efi_status_t set_property_int(const u16 *variable_name, + efi_uintn_t name_size, const efi_guid_t *vendor, struct var_check_property *var_property) { @@ -317,7 +318,8 @@ out: return ret; } -static efi_status_t get_property_int(u16 *variable_name, efi_uintn_t name_size, +static efi_status_t get_property_int(const u16 *variable_name, + efi_uintn_t name_size, const efi_guid_t *vendor, struct var_check_property *var_property) { @@ -361,7 +363,8 @@ out: return ret; } -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, +efi_status_t efi_get_variable_int(const u16 *variable_name, + const efi_guid_t *vendor, u32 *attributes, efi_uintn_t *data_size, void *data, u64 *timep) { @@ -502,9 +505,10 @@ out: return ret; } -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, - u32 attributes, efi_uintn_t data_size, - const void *data, bool ro_check) +efi_status_t efi_set_variable_int(const u16 *variable_name, + const efi_guid_t *vendor, u32 attributes, + efi_uintn_t data_size, const void *data, + bool ro_check) { efi_status_t ret, alt_ret = EFI_SUCCESS; struct var_check_property var_property; -- 2.30.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] efi_loader: treat UEFI variable name as const 2021-09-11 7:28 ` [PATCH 1/4] efi_loader: treat UEFI variable name as const Heinrich Schuchardt @ 2021-09-11 14:10 ` Ilias Apalodimas 2021-09-12 19:19 ` Heinrich Schuchardt 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-09-11 14:10 UTC (permalink / raw) To: Heinrich Schuchardt Cc: u-boot, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf Hi Heinrich, On Sat, Sep 11, 2021 at 09:28:29AM +0200, Heinrich Schuchardt wrote: > Adjust several internal functions to treat UEFI variable names as const. It's obvious what the patch does, but is there a reason ? I think that's a better fit for the commit log. Cheers /Ilias > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > include/efi_loader.h | 2 +- > include/efi_variable.h | 16 ++++++++++------ > lib/efi_loader/efi_tcg2.c | 2 +- > lib/efi_loader/efi_var_common.c | 5 +++-- > lib/efi_loader/efi_var_mem.c | 7 ++++--- > lib/efi_loader/efi_variable.c | 9 +++++---- > lib/efi_loader/efi_variable_tee.c | 16 ++++++++++------ > 7 files changed, 34 insertions(+), 23 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index c440962fe5..125052d002 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -816,7 +816,7 @@ efi_status_t EFIAPI efi_query_variable_info( > u64 *remaining_variable_storage_size, > u64 *maximum_variable_size); > > -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); > +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); > > /* > * See section 3.1.3 in the v2.7 UEFI spec for more details on > diff --git a/include/efi_variable.h b/include/efi_variable.h > index 0440d356bc..8f666b2309 100644 > --- a/include/efi_variable.h > +++ b/include/efi_variable.h > @@ -32,7 +32,8 @@ enum efi_auth_var_type { > * @timep: authentication time (seconds since start of epoch) > * Return: status code > */ > -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, > +efi_status_t efi_get_variable_int(const u16 *variable_name, > + const efi_guid_t *vendor, > u32 *attributes, efi_uintn_t *data_size, > void *data, u64 *timep); > > @@ -47,7 +48,8 @@ efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, > * @ro_check: check the read only read only bit in attributes > * Return: status code > */ > -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, > +efi_status_t efi_set_variable_int(const u16 *variable_name, > + const efi_guid_t *vendor, > u32 attributes, efi_uintn_t data_size, > const void *data, bool ro_check); > > @@ -224,7 +226,7 @@ void efi_var_mem_del(struct efi_var_entry *var); > * @time: time of authentication (as seconds since start of epoch) > * Result: status code > */ > -efi_status_t efi_var_mem_ins(u16 *variable_name, > +efi_status_t efi_var_mem_ins(const u16 *variable_name, > const efi_guid_t *vendor, u32 attributes, > const efi_uintn_t size1, const void *data1, > const efi_uintn_t size2, const void *data2, > @@ -251,7 +253,8 @@ efi_status_t efi_init_secure_state(void); > * @guid: guid of UEFI variable > * Return: identifier for authentication related variables > */ > -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid); > +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, > + const efi_guid_t *guid); > > /** > * efi_get_next_variable_name_mem() - Runtime common code across efi variable > @@ -280,8 +283,9 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na > * Return: status code > */ > efi_status_t __efi_runtime > -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, > - efi_uintn_t *data_size, void *data, u64 *timep); > +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor, > + u32 *attributes, efi_uintn_t *data_size, void *data, > + u64 *timep); > > /** > * efi_get_variable_runtime() - runtime implementation of GetVariable() > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index 401acf3d4f..beb224f66a 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -1359,7 +1359,7 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev) > * Return: status code > */ > static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index, > - u32 event_type, u16 *var_name, > + u32 event_type, const u16 *var_name, > const efi_guid_t *guid, > efi_uintn_t data_size, u8 *data) > { > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c > index a00bbf1620..e179932124 100644 > --- a/lib/efi_loader/efi_var_common.c > +++ b/lib/efi_loader/efi_var_common.c > @@ -374,7 +374,8 @@ bool efi_secure_boot_enabled(void) > return efi_secure_boot; > } > > -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid) > +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, > + const efi_guid_t *guid) > { > for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) { > if (!u16_strcmp(name, name_type[i].name) && > @@ -393,7 +394,7 @@ enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid) > * > * Return: buffer with variable data or NULL > */ > -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) > +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) > { > efi_status_t ret; > void *buf = NULL; > diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c > index 3d335a8274..13909b1d26 100644 > --- a/lib/efi_loader/efi_var_mem.c > +++ b/lib/efi_loader/efi_var_mem.c > @@ -134,7 +134,7 @@ void __efi_runtime efi_var_mem_del(struct efi_var_entry *var) > } > > efi_status_t __efi_runtime efi_var_mem_ins( > - u16 *variable_name, > + const u16 *variable_name, > const efi_guid_t *vendor, u32 attributes, > const efi_uintn_t size1, const void *data1, > const efi_uintn_t size2, const void *data2, > @@ -274,8 +274,9 @@ efi_status_t efi_var_mem_init(void) > } > > efi_status_t __efi_runtime > -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, > - efi_uintn_t *data_size, void *data, u64 *timep) > +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor, > + u32 *attributes, efi_uintn_t *data_size, void *data, > + u64 *timep) > { > efi_uintn_t old_size; > struct efi_var_entry *var; > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index fa2b6bc7a8..5adc7f821a 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -45,7 +45,7 @@ > * > * Return: status code > */ > -static efi_status_t efi_variable_authenticate(u16 *variable, > +static efi_status_t efi_variable_authenticate(const u16 *variable, > const efi_guid_t *vendor, > efi_uintn_t *data_size, > const void **data, u32 given_attr, > @@ -194,7 +194,7 @@ err: > return ret; > } > #else > -static efi_status_t efi_variable_authenticate(u16 *variable, > +static efi_status_t efi_variable_authenticate(const u16 *variable, > const efi_guid_t *vendor, > efi_uintn_t *data_size, > const void **data, u32 given_attr, > @@ -205,7 +205,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable, > #endif /* CONFIG_EFI_SECURE_BOOT */ > > efi_status_t __efi_runtime > -efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, > +efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor, > u32 *attributes, efi_uintn_t *data_size, void *data, > u64 *timep) > { > @@ -219,7 +219,8 @@ efi_get_next_variable_name_int(efi_uintn_t *variable_name_size, > return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor); > } > > -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, > +efi_status_t efi_set_variable_int(const u16 *variable_name, > + const efi_guid_t *vendor, > u32 attributes, efi_uintn_t data_size, > const void *data, bool ro_check) > { > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c > index 51920bcb51..281f886124 100644 > --- a/lib/efi_loader/efi_variable_tee.c > +++ b/lib/efi_loader/efi_variable_tee.c > @@ -284,7 +284,8 @@ out: > * StMM can store internal attributes and properties for variables, i.e enabling > * R/O variables > */ > -static efi_status_t set_property_int(u16 *variable_name, efi_uintn_t name_size, > +static efi_status_t set_property_int(const u16 *variable_name, > + efi_uintn_t name_size, > const efi_guid_t *vendor, > struct var_check_property *var_property) > { > @@ -317,7 +318,8 @@ out: > return ret; > } > > -static efi_status_t get_property_int(u16 *variable_name, efi_uintn_t name_size, > +static efi_status_t get_property_int(const u16 *variable_name, > + efi_uintn_t name_size, > const efi_guid_t *vendor, > struct var_check_property *var_property) > { > @@ -361,7 +363,8 @@ out: > return ret; > } > > -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, > +efi_status_t efi_get_variable_int(const u16 *variable_name, > + const efi_guid_t *vendor, > u32 *attributes, efi_uintn_t *data_size, > void *data, u64 *timep) > { > @@ -502,9 +505,10 @@ out: > return ret; > } > > -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, > - u32 attributes, efi_uintn_t data_size, > - const void *data, bool ro_check) > +efi_status_t efi_set_variable_int(const u16 *variable_name, > + const efi_guid_t *vendor, u32 attributes, > + efi_uintn_t data_size, const void *data, > + bool ro_check) > { > efi_status_t ret, alt_ret = EFI_SUCCESS; > struct var_check_property var_property; > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] efi_loader: treat UEFI variable name as const 2021-09-11 14:10 ` Ilias Apalodimas @ 2021-09-12 19:19 ` Heinrich Schuchardt 0 siblings, 0 replies; 14+ messages in thread From: Heinrich Schuchardt @ 2021-09-12 19:19 UTC (permalink / raw) To: Ilias Apalodimas Cc: u-boot, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf On 9/11/21 4:10 PM, Ilias Apalodimas wrote: > Hi Heinrich, > > On Sat, Sep 11, 2021 at 09:28:29AM +0200, Heinrich Schuchardt wrote: >> Adjust several internal functions to treat UEFI variable names as const. > > It's obvious what the patch does, but is there a reason ? I think that's a > better fit for the commit log. Thanks for reviewing. We all this functions with const strings like "PE". It does not make sense to convert to from (u16 *) to (const u16 *) somewhere in the code. Instead the changed functions should offer an easily usable interface. I will rework the commit message. Best regards Heinrich > > Cheers > /Ilias >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> include/efi_loader.h | 2 +- >> include/efi_variable.h | 16 ++++++++++------ >> lib/efi_loader/efi_tcg2.c | 2 +- >> lib/efi_loader/efi_var_common.c | 5 +++-- >> lib/efi_loader/efi_var_mem.c | 7 ++++--- >> lib/efi_loader/efi_variable.c | 9 +++++---- >> lib/efi_loader/efi_variable_tee.c | 16 ++++++++++------ >> 7 files changed, 34 insertions(+), 23 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index c440962fe5..125052d002 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -816,7 +816,7 @@ efi_status_t EFIAPI efi_query_variable_info( >> u64 *remaining_variable_storage_size, >> u64 *maximum_variable_size); >> >> -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); >> +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); >> >> /* >> * See section 3.1.3 in the v2.7 UEFI spec for more details on >> diff --git a/include/efi_variable.h b/include/efi_variable.h >> index 0440d356bc..8f666b2309 100644 >> --- a/include/efi_variable.h >> +++ b/include/efi_variable.h >> @@ -32,7 +32,8 @@ enum efi_auth_var_type { >> * @timep: authentication time (seconds since start of epoch) >> * Return: status code >> */ >> -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> +efi_status_t efi_get_variable_int(const u16 *variable_name, >> + const efi_guid_t *vendor, >> u32 *attributes, efi_uintn_t *data_size, >> void *data, u64 *timep); >> >> @@ -47,7 +48,8 @@ efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> * @ro_check: check the read only read only bit in attributes >> * Return: status code >> */ >> -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> +efi_status_t efi_set_variable_int(const u16 *variable_name, >> + const efi_guid_t *vendor, >> u32 attributes, efi_uintn_t data_size, >> const void *data, bool ro_check); >> >> @@ -224,7 +226,7 @@ void efi_var_mem_del(struct efi_var_entry *var); >> * @time: time of authentication (as seconds since start of epoch) >> * Result: status code >> */ >> -efi_status_t efi_var_mem_ins(u16 *variable_name, >> +efi_status_t efi_var_mem_ins(const u16 *variable_name, >> const efi_guid_t *vendor, u32 attributes, >> const efi_uintn_t size1, const void *data1, >> const efi_uintn_t size2, const void *data2, >> @@ -251,7 +253,8 @@ efi_status_t efi_init_secure_state(void); >> * @guid: guid of UEFI variable >> * Return: identifier for authentication related variables >> */ >> -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid); >> +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, >> + const efi_guid_t *guid); >> >> /** >> * efi_get_next_variable_name_mem() - Runtime common code across efi variable >> @@ -280,8 +283,9 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na >> * Return: status code >> */ >> efi_status_t __efi_runtime >> -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, >> - efi_uintn_t *data_size, void *data, u64 *timep); >> +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor, >> + u32 *attributes, efi_uintn_t *data_size, void *data, >> + u64 *timep); >> >> /** >> * efi_get_variable_runtime() - runtime implementation of GetVariable() >> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c >> index 401acf3d4f..beb224f66a 100644 >> --- a/lib/efi_loader/efi_tcg2.c >> +++ b/lib/efi_loader/efi_tcg2.c >> @@ -1359,7 +1359,7 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev) >> * Return: status code >> */ >> static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index, >> - u32 event_type, u16 *var_name, >> + u32 event_type, const u16 *var_name, >> const efi_guid_t *guid, >> efi_uintn_t data_size, u8 *data) >> { >> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c >> index a00bbf1620..e179932124 100644 >> --- a/lib/efi_loader/efi_var_common.c >> +++ b/lib/efi_loader/efi_var_common.c >> @@ -374,7 +374,8 @@ bool efi_secure_boot_enabled(void) >> return efi_secure_boot; >> } >> >> -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid) >> +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, >> + const efi_guid_t *guid) >> { >> for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) { >> if (!u16_strcmp(name, name_type[i].name) && >> @@ -393,7 +394,7 @@ enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid) >> * >> * Return: buffer with variable data or NULL >> */ >> -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) >> +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) >> { >> efi_status_t ret; >> void *buf = NULL; >> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c >> index 3d335a8274..13909b1d26 100644 >> --- a/lib/efi_loader/efi_var_mem.c >> +++ b/lib/efi_loader/efi_var_mem.c >> @@ -134,7 +134,7 @@ void __efi_runtime efi_var_mem_del(struct efi_var_entry *var) >> } >> >> efi_status_t __efi_runtime efi_var_mem_ins( >> - u16 *variable_name, >> + const u16 *variable_name, >> const efi_guid_t *vendor, u32 attributes, >> const efi_uintn_t size1, const void *data1, >> const efi_uintn_t size2, const void *data2, >> @@ -274,8 +274,9 @@ efi_status_t efi_var_mem_init(void) >> } >> >> efi_status_t __efi_runtime >> -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes, >> - efi_uintn_t *data_size, void *data, u64 *timep) >> +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor, >> + u32 *attributes, efi_uintn_t *data_size, void *data, >> + u64 *timep) >> { >> efi_uintn_t old_size; >> struct efi_var_entry *var; >> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c >> index fa2b6bc7a8..5adc7f821a 100644 >> --- a/lib/efi_loader/efi_variable.c >> +++ b/lib/efi_loader/efi_variable.c >> @@ -45,7 +45,7 @@ >> * >> * Return: status code >> */ >> -static efi_status_t efi_variable_authenticate(u16 *variable, >> +static efi_status_t efi_variable_authenticate(const u16 *variable, >> const efi_guid_t *vendor, >> efi_uintn_t *data_size, >> const void **data, u32 given_attr, >> @@ -194,7 +194,7 @@ err: >> return ret; >> } >> #else >> -static efi_status_t efi_variable_authenticate(u16 *variable, >> +static efi_status_t efi_variable_authenticate(const u16 *variable, >> const efi_guid_t *vendor, >> efi_uintn_t *data_size, >> const void **data, u32 given_attr, >> @@ -205,7 +205,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable, >> #endif /* CONFIG_EFI_SECURE_BOOT */ >> >> efi_status_t __efi_runtime >> -efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> +efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor, >> u32 *attributes, efi_uintn_t *data_size, void *data, >> u64 *timep) >> { >> @@ -219,7 +219,8 @@ efi_get_next_variable_name_int(efi_uintn_t *variable_name_size, >> return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor); >> } >> >> -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> +efi_status_t efi_set_variable_int(const u16 *variable_name, >> + const efi_guid_t *vendor, >> u32 attributes, efi_uintn_t data_size, >> const void *data, bool ro_check) >> { >> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c >> index 51920bcb51..281f886124 100644 >> --- a/lib/efi_loader/efi_variable_tee.c >> +++ b/lib/efi_loader/efi_variable_tee.c >> @@ -284,7 +284,8 @@ out: >> * StMM can store internal attributes and properties for variables, i.e enabling >> * R/O variables >> */ >> -static efi_status_t set_property_int(u16 *variable_name, efi_uintn_t name_size, >> +static efi_status_t set_property_int(const u16 *variable_name, >> + efi_uintn_t name_size, >> const efi_guid_t *vendor, >> struct var_check_property *var_property) >> { >> @@ -317,7 +318,8 @@ out: >> return ret; >> } >> >> -static efi_status_t get_property_int(u16 *variable_name, efi_uintn_t name_size, >> +static efi_status_t get_property_int(const u16 *variable_name, >> + efi_uintn_t name_size, >> const efi_guid_t *vendor, >> struct var_check_property *var_property) >> { >> @@ -361,7 +363,8 @@ out: >> return ret; >> } >> >> -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> +efi_status_t efi_get_variable_int(const u16 *variable_name, >> + const efi_guid_t *vendor, >> u32 *attributes, efi_uintn_t *data_size, >> void *data, u64 *timep) >> { >> @@ -502,9 +505,10 @@ out: >> return ret; >> } >> >> -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, >> - u32 attributes, efi_uintn_t data_size, >> - const void *data, bool ro_check) >> +efi_status_t efi_set_variable_int(const u16 *variable_name, >> + const efi_guid_t *vendor, u32 attributes, >> + efi_uintn_t data_size, const void *data, >> + bool ro_check) >> { >> efi_status_t ret, alt_ret = EFI_SUCCESS; >> struct var_check_property var_property; >> -- >> 2.30.2 >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] efi_loader: function to get GUID for variable name 2021-09-11 7:28 [PATCH 0/4] efi_loader: centralize known vendor GUIDs Heinrich Schuchardt 2021-09-11 7:28 ` [PATCH 1/4] efi_loader: treat UEFI variable name as const Heinrich Schuchardt @ 2021-09-11 7:28 ` Heinrich Schuchardt 2021-09-11 14:13 ` Ilias Apalodimas 2021-09-11 7:28 ` [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() Heinrich Schuchardt 2021-09-11 7:28 ` [PATCH 4/4] efi_loader: simplify tcg2_measure_secure_boot_variable() Heinrich Schuchardt 3 siblings, 1 reply; 14+ messages in thread From: Heinrich Schuchardt @ 2021-09-11 7:28 UTC (permalink / raw) To: u-boot Cc: Ilias Apalodimas, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf, Heinrich Schuchardt In multiple places we need the default GUID used for variables like 'PK', 'KEK', 'db'. Provide a function for it. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- include/efi_variable.h | 8 ++++++++ lib/efi_loader/efi_var_common.c | 9 +++++++++ 2 files changed, 17 insertions(+) diff --git a/include/efi_variable.h b/include/efi_variable.h index 8f666b2309..03a3ecb235 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -256,6 +256,14 @@ efi_status_t efi_init_secure_state(void); enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, const efi_guid_t *guid); +/** + * efi_auth_var_get_guid() - get the predefined GUID for a variable name + * + * @name: name of UEFI variable + * Return: guid of UEFI variable + */ +const efi_guid_t *efi_auth_var_get_guid(const u16 *name); + /** * efi_get_next_variable_name_mem() - Runtime common code across efi variable * implementations for GetNextVariable() diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index e179932124..3cbb7c96c2 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -385,6 +385,15 @@ enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, return EFI_AUTH_VAR_NONE; } +const efi_guid_t *efi_auth_var_get_guid(const u16 *name) +{ + for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) { + if (!u16_strcmp(name, name_type[i].name)) + return name_type[i].guid; + } + return &efi_global_variable_guid; +} + /** * efi_get_var() - read value of an EFI variable * -- 2.30.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] efi_loader: function to get GUID for variable name 2021-09-11 7:28 ` [PATCH 2/4] efi_loader: function to get GUID for variable name Heinrich Schuchardt @ 2021-09-11 14:13 ` Ilias Apalodimas 2021-09-11 14:21 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-09-11 14:13 UTC (permalink / raw) To: Heinrich Schuchardt Cc: u-boot, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf On Sat, Sep 11, 2021 at 09:28:30AM +0200, Heinrich Schuchardt wrote: > In multiple places we need the default GUID used for variables like > 'PK', 'KEK', 'db'. Provide a function for it. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > include/efi_variable.h | 8 ++++++++ > lib/efi_loader/efi_var_common.c | 9 +++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/include/efi_variable.h b/include/efi_variable.h > index 8f666b2309..03a3ecb235 100644 > --- a/include/efi_variable.h > +++ b/include/efi_variable.h > @@ -256,6 +256,14 @@ efi_status_t efi_init_secure_state(void); > enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, > const efi_guid_t *guid); > > +/** > + * efi_auth_var_get_guid() - get the predefined GUID for a variable name > + * > + * @name: name of UEFI variable > + * Return: guid of UEFI variable > + */ > +const efi_guid_t *efi_auth_var_get_guid(const u16 *name); > + > /** > * efi_get_next_variable_name_mem() - Runtime common code across efi variable > * implementations for GetNextVariable() > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c > index e179932124..3cbb7c96c2 100644 > --- a/lib/efi_loader/efi_var_common.c > +++ b/lib/efi_loader/efi_var_common.c > @@ -385,6 +385,15 @@ enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, > return EFI_AUTH_VAR_NONE; > } > > +const efi_guid_t *efi_auth_var_get_guid(const u16 *name) > +{ > + for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) { > + if (!u16_strcmp(name, name_type[i].name)) > + return name_type[i].guid; > + } > + return &efi_global_variable_guid; > +} > + > /** > * efi_get_var() - read value of an EFI variable > * > -- > 2.30.2 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] efi_loader: function to get GUID for variable name 2021-09-11 14:13 ` Ilias Apalodimas @ 2021-09-11 14:21 ` Ilias Apalodimas 0 siblings, 0 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-09-11 14:21 UTC (permalink / raw) To: Heinrich Schuchardt Cc: U-Boot Mailing List, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf On Sat, 11 Sept 2021 at 17:13, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Sat, Sep 11, 2021 at 09:28:30AM +0200, Heinrich Schuchardt wrote: > > In multiple places we need the default GUID used for variables like > > 'PK', 'KEK', 'db'. Provide a function for it. > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > --- > > include/efi_variable.h | 8 ++++++++ > > lib/efi_loader/efi_var_common.c | 9 +++++++++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/include/efi_variable.h b/include/efi_variable.h > > index 8f666b2309..03a3ecb235 100644 > > --- a/include/efi_variable.h > > +++ b/include/efi_variable.h > > @@ -256,6 +256,14 @@ efi_status_t efi_init_secure_state(void); > > enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, > > const efi_guid_t *guid); > > > > +/** > > + * efi_auth_var_get_guid() - get the predefined GUID for a variable name > > + * > > + * @name: name of UEFI variable > > + * Return: guid of UEFI variable > > + */ > > +const efi_guid_t *efi_auth_var_get_guid(const u16 *name); > > + > > /** > > * efi_get_next_variable_name_mem() - Runtime common code across efi variable > > * implementations for GetNextVariable() > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c > > index e179932124..3cbb7c96c2 100644 > > --- a/lib/efi_loader/efi_var_common.c > > +++ b/lib/efi_loader/efi_var_common.c > > @@ -385,6 +385,15 @@ enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, > > return EFI_AUTH_VAR_NONE; > > } > > > > +const efi_guid_t *efi_auth_var_get_guid(const u16 *name) > > +{ > > + for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) { > > + if (!u16_strcmp(name, name_type[i].name)) > > + return name_type[i].guid; > > + } > > + return &efi_global_variable_guid; Actually looking at the following patch, shouldn't this be NULL? > > +} > > + > > /** > > * efi_get_var() - read value of an EFI variable > > * > > -- > > 2.30.2 > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() 2021-09-11 7:28 [PATCH 0/4] efi_loader: centralize known vendor GUIDs Heinrich Schuchardt 2021-09-11 7:28 ` [PATCH 1/4] efi_loader: treat UEFI variable name as const Heinrich Schuchardt 2021-09-11 7:28 ` [PATCH 2/4] efi_loader: function to get GUID for variable name Heinrich Schuchardt @ 2021-09-11 7:28 ` Heinrich Schuchardt 2021-09-11 14:25 ` Ilias Apalodimas 2021-09-11 7:28 ` [PATCH 4/4] efi_loader: simplify tcg2_measure_secure_boot_variable() Heinrich Schuchardt 3 siblings, 1 reply; 14+ messages in thread From: Heinrich Schuchardt @ 2021-09-11 7:28 UTC (permalink / raw) To: u-boot Cc: Ilias Apalodimas, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf, Heinrich Schuchardt Simplify efi_sigstore_parse_sigdb() by using existing functions. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- lib/efi_loader/efi_signature.c | 35 ++++++---------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c index bdd09881fc..b741905a99 100644 --- a/lib/efi_loader/efi_signature.c +++ b/lib/efi_loader/efi_signature.c @@ -7,6 +7,7 @@ #include <common.h> #include <charset.h> #include <efi_loader.h> +#include <efi_variable.h> #include <image.h> #include <hexdump.h> #include <malloc.h> @@ -740,44 +741,20 @@ err: */ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name) { - struct efi_signature_store *sigstore = NULL; const efi_guid_t *vendor; void *db; efi_uintn_t db_size; - efi_status_t ret; - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { - vendor = &efi_global_variable_guid; - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { - vendor = &efi_guid_image_security_database; - } else { + vendor = efi_auth_var_get_guid(name); + if (!vendor) { EFI_PRINT("unknown signature database, %ls\n", name); return NULL; } - /* retrieve variable data */ - db_size = 0; - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); - if (ret == EFI_NOT_FOUND) { - EFI_PRINT("variable, %ls, not found\n", name); - sigstore = calloc(sizeof(*sigstore), 1); - return sigstore; - } else if (ret != EFI_BUFFER_TOO_SMALL) { - EFI_PRINT("Getting variable, %ls, failed\n", name); - return NULL; - } - - db = malloc(db_size); + db = efi_get_var(name, vendor, &db_size); if (!db) { - EFI_PRINT("Out of memory\n"); - return NULL; - } - - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); - if (ret != EFI_SUCCESS) { - EFI_PRINT("Getting variable, %ls, failed\n", name); - free(db); - return NULL; + EFI_PRINT("variable, %ls, not found\n", name); + return calloc(sizeof(struct efi_signature_store), 1); } return efi_build_signature_store(db, db_size); -- 2.30.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() 2021-09-11 7:28 ` [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() Heinrich Schuchardt @ 2021-09-11 14:25 ` Ilias Apalodimas 2021-09-12 19:16 ` Heinrich Schuchardt 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-09-11 14:25 UTC (permalink / raw) To: Heinrich Schuchardt Cc: u-boot, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf On Sat, Sep 11, 2021 at 09:28:31AM +0200, Heinrich Schuchardt wrote: > Simplify efi_sigstore_parse_sigdb() by using existing functions. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > lib/efi_loader/efi_signature.c | 35 ++++++---------------------------- > 1 file changed, 6 insertions(+), 29 deletions(-) > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > index bdd09881fc..b741905a99 100644 > --- a/lib/efi_loader/efi_signature.c > +++ b/lib/efi_loader/efi_signature.c > @@ -7,6 +7,7 @@ > #include <common.h> > #include <charset.h> > #include <efi_loader.h> > +#include <efi_variable.h> > #include <image.h> > #include <hexdump.h> > #include <malloc.h> > @@ -740,44 +741,20 @@ err: > */ > struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name) > { > - struct efi_signature_store *sigstore = NULL; > const efi_guid_t *vendor; > void *db; > efi_uintn_t db_size; > - efi_status_t ret; > > - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { > - vendor = &efi_global_variable_guid; > - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { > - vendor = &efi_guid_image_security_database; > - } else { > + vendor = efi_auth_var_get_guid(name); > + if (!vendor) { > EFI_PRINT("unknown signature database, %ls\n", name); > return NULL; > } efi_auth_var_get_guid() will return &efi_global_variable_guid if the GUID for the variable name isn't found. > > - /* retrieve variable data */ > - db_size = 0; > - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); > - if (ret == EFI_NOT_FOUND) { > - EFI_PRINT("variable, %ls, not found\n", name); > - sigstore = calloc(sizeof(*sigstore), 1); > - return sigstore; > - } else if (ret != EFI_BUFFER_TOO_SMALL) { > - EFI_PRINT("Getting variable, %ls, failed\n", name); > - return NULL; > - } > - > - db = malloc(db_size); > + db = efi_get_var(name, vendor, &db_size); > if (!db) { > - EFI_PRINT("Out of memory\n"); > - return NULL; > - } > - > - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); > - if (ret != EFI_SUCCESS) { > - EFI_PRINT("Getting variable, %ls, failed\n", name); > - free(db); > - return NULL; > + EFI_PRINT("variable, %ls, not found\n", name); > + return calloc(sizeof(struct efi_signature_store), 1); > } > > return efi_build_signature_store(db, db_size); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() 2021-09-11 14:25 ` Ilias Apalodimas @ 2021-09-12 19:16 ` Heinrich Schuchardt 2021-09-12 19:23 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Heinrich Schuchardt @ 2021-09-12 19:16 UTC (permalink / raw) To: Ilias Apalodimas Cc: u-boot, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf On 9/11/21 4:25 PM, Ilias Apalodimas wrote: > On Sat, Sep 11, 2021 at 09:28:31AM +0200, Heinrich Schuchardt wrote: >> Simplify efi_sigstore_parse_sigdb() by using existing functions. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> lib/efi_loader/efi_signature.c | 35 ++++++---------------------------- >> 1 file changed, 6 insertions(+), 29 deletions(-) >> >> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c >> index bdd09881fc..b741905a99 100644 >> --- a/lib/efi_loader/efi_signature.c >> +++ b/lib/efi_loader/efi_signature.c >> @@ -7,6 +7,7 @@ >> #include <common.h> >> #include <charset.h> >> #include <efi_loader.h> >> +#include <efi_variable.h> >> #include <image.h> >> #include <hexdump.h> >> #include <malloc.h> >> @@ -740,44 +741,20 @@ err: >> */ >> struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name) >> { >> - struct efi_signature_store *sigstore = NULL; >> const efi_guid_t *vendor; >> void *db; >> efi_uintn_t db_size; >> - efi_status_t ret; >> >> - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { >> - vendor = &efi_global_variable_guid; >> - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { >> - vendor = &efi_guid_image_security_database; >> - } else { >> + vendor = efi_auth_var_get_guid(name); >> + if (!vendor) { >> EFI_PRINT("unknown signature database, %ls\n", name); >> return NULL; >> } > > efi_auth_var_get_guid() will return &efi_global_variable_guid if the > GUID for the variable name isn't found. Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID. I want to reuse the same function there in future. Best regards Heinrich > >> >> - /* retrieve variable data */ >> - db_size = 0; >> - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); >> - if (ret == EFI_NOT_FOUND) { >> - EFI_PRINT("variable, %ls, not found\n", name); >> - sigstore = calloc(sizeof(*sigstore), 1); >> - return sigstore; >> - } else if (ret != EFI_BUFFER_TOO_SMALL) { >> - EFI_PRINT("Getting variable, %ls, failed\n", name); >> - return NULL; >> - } >> - >> - db = malloc(db_size); >> + db = efi_get_var(name, vendor, &db_size); >> if (!db) { >> - EFI_PRINT("Out of memory\n"); >> - return NULL; >> - } >> - >> - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); >> - if (ret != EFI_SUCCESS) { >> - EFI_PRINT("Getting variable, %ls, failed\n", name); >> - free(db); >> - return NULL; >> + EFI_PRINT("variable, %ls, not found\n", name); >> + return calloc(sizeof(struct efi_signature_store), 1); >> } >> >> return efi_build_signature_store(db, db_size); >> -- >> 2.30.2 >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() 2021-09-12 19:16 ` Heinrich Schuchardt @ 2021-09-12 19:23 ` Ilias Apalodimas 2021-10-01 16:42 ` Heinrich Schuchardt 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-09-12 19:23 UTC (permalink / raw) To: Heinrich Schuchardt Cc: U-Boot Mailing List, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf Hi Heinrich [...] > >> - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { > >> - vendor = &efi_global_variable_guid; > >> - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { > >> - vendor = &efi_guid_image_security_database; > >> - } else { > >> + vendor = efi_auth_var_get_guid(name); > >> + if (!vendor) { > >> EFI_PRINT("unknown signature database, %ls\n", name); > >> return NULL; > >> } > > > > efi_auth_var_get_guid() will return &efi_global_variable_guid if the > > GUID for the variable name isn't found. > > Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID. > I want to reuse the same function there in future. > > Best regards Then I guess the check can go away ? > > Heinrich > > > > >> > >> - /* retrieve variable data */ > >> - db_size = 0; > >> - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); > >> - if (ret == EFI_NOT_FOUND) { > >> - EFI_PRINT("variable, %ls, not found\n", name); > >> - sigstore = calloc(sizeof(*sigstore), 1); > >> - return sigstore; > >> - } else if (ret != EFI_BUFFER_TOO_SMALL) { > >> - EFI_PRINT("Getting variable, %ls, failed\n", name); > >> - return NULL; > >> - } > >> - > >> - db = malloc(db_size); > >> + db = efi_get_var(name, vendor, &db_size); > >> if (!db) { > >> - EFI_PRINT("Out of memory\n"); > >> - return NULL; > >> - } > >> - > >> - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); > >> - if (ret != EFI_SUCCESS) { > >> - EFI_PRINT("Getting variable, %ls, failed\n", name); > >> - free(db); > >> - return NULL; > >> + EFI_PRINT("variable, %ls, not found\n", name); > >> + return calloc(sizeof(struct efi_signature_store), 1); Why? From the patch alone it's not clear why you want to allocate memory here instead of returning NULL. > >> } > >> > >> return efi_build_signature_store(db, db_size); > >> -- > >> 2.30.2 > >> Cheers /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() 2021-09-12 19:23 ` Ilias Apalodimas @ 2021-10-01 16:42 ` Heinrich Schuchardt 2021-10-01 19:08 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Heinrich Schuchardt @ 2021-10-01 16:42 UTC (permalink / raw) To: Ilias Apalodimas Cc: U-Boot Mailing List, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf On 9/12/21 21:23, Ilias Apalodimas wrote: > Hi Heinrich > > [...] >>>> - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { >>>> - vendor = &efi_global_variable_guid; >>>> - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { >>>> - vendor = &efi_guid_image_security_database; >>>> - } else { >>>> + vendor = efi_auth_var_get_guid(name); >>>> + if (!vendor) { >>>> EFI_PRINT("unknown signature database, %ls\n", name); >>>> return NULL; >>>> } >>> >>> efi_auth_var_get_guid() will return &efi_global_variable_guid if the >>> GUID for the variable name isn't found. >> >> Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID. >> I want to reuse the same function there in future. >> >> Best regards > > Then I guess the check can go away ? Yes > >> >> Heinrich >> >>> >>>> >>>> - /* retrieve variable data */ >>>> - db_size = 0; >>>> - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); >>>> - if (ret == EFI_NOT_FOUND) { >>>> - EFI_PRINT("variable, %ls, not found\n", name); >>>> - sigstore = calloc(sizeof(*sigstore), 1); >>>> - return sigstore; >>>> - } else if (ret != EFI_BUFFER_TOO_SMALL) { >>>> - EFI_PRINT("Getting variable, %ls, failed\n", name); >>>> - return NULL; >>>> - } >>>> - >>>> - db = malloc(db_size); >>>> + db = efi_get_var(name, vendor, &db_size); >>>> if (!db) { >>>> - EFI_PRINT("Out of memory\n"); >>>> - return NULL; >>>> - } >>>> - >>>> - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); >>>> - if (ret != EFI_SUCCESS) { >>>> - EFI_PRINT("Getting variable, %ls, failed\n", name); >>>> - free(db); >>>> - return NULL; >>>> + EFI_PRINT("variable, %ls, not found\n", name); >>>> + return calloc(sizeof(struct efi_signature_store), 1); > > Why? From the patch alone it's not clear why you want to allocate > memory here instead of returning NULL. This is existing code. See the same lines deleted above. Best regards Heinrich > >>>> } >>>> >>>> return efi_build_signature_store(db, db_size); >>>> -- >>>> 2.30.2 >>>> > > Cheers > /Ilias > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() 2021-10-01 16:42 ` Heinrich Schuchardt @ 2021-10-01 19:08 ` Ilias Apalodimas 0 siblings, 0 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-10-01 19:08 UTC (permalink / raw) To: Heinrich Schuchardt Cc: U-Boot Mailing List, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf Hi Heinrich, On Fri, Oct 01, 2021 at 06:42:14PM +0200, Heinrich Schuchardt wrote: > > > On 9/12/21 21:23, Ilias Apalodimas wrote: > > Hi Heinrich > > > > [...] > > > > > - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { > > > > > - vendor = &efi_global_variable_guid; > > > > > - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { > > > > > - vendor = &efi_guid_image_security_database; > > > > > - } else { > > > > > + vendor = efi_auth_var_get_guid(name); > > > > > + if (!vendor) { > > > > > EFI_PRINT("unknown signature database, %ls\n", name); > > > > > return NULL; > > > > > } > > > > > > > > efi_auth_var_get_guid() will return &efi_global_variable_guid if the > > > > GUID for the variable name isn't found. > > > > > > Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID. > > > I want to reuse the same function there in future. > > > > > > Best regards > > > > Then I guess the check can go away ? > > Yes > > > > > > > > > Heinrich > > > > > > > > > > > > > > > > > - /* retrieve variable data */ > > > > > - db_size = 0; > > > > > - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); > > > > > - if (ret == EFI_NOT_FOUND) { > > > > > - EFI_PRINT("variable, %ls, not found\n", name); > > > > > - sigstore = calloc(sizeof(*sigstore), 1); > > > > > - return sigstore; > > > > > - } else if (ret != EFI_BUFFER_TOO_SMALL) { > > > > > - EFI_PRINT("Getting variable, %ls, failed\n", name); > > > > > - return NULL; > > > > > - } > > > > > - > > > > > - db = malloc(db_size); > > > > > + db = efi_get_var(name, vendor, &db_size); > > > > > if (!db) { > > > > > - EFI_PRINT("Out of memory\n"); > > > > > - return NULL; > > > > > - } > > > > > - > > > > > - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); > > > > > - if (ret != EFI_SUCCESS) { > > > > > - EFI_PRINT("Getting variable, %ls, failed\n", name); > > > > > - free(db); > > > > > - return NULL; > > > > > + EFI_PRINT("variable, %ls, not found\n", name); > > > > > + return calloc(sizeof(struct efi_signature_store), 1); > > > > Why? From the patch alone it's not clear why you want to allocate > > memory here instead of returning NULL. > > This is existing code. See the same lines deleted above. If I read the code correctly, we are trying to be smart about the buffer outcome. Check for example efi_image_unsigned_authenticate(). By returning an empty buffer the 'dbx' check will succeed but the 'db' check a few lines after will fail. But this is pointless imho... Why don't we just have efi_status_t efi_signature_store efi_sigstore_parse_sigdb(u16 *name, struct efi_signature_store *store) We can the control the EFI return value in efi_sigstore_parse_sigdb() and any callers would just have to look at the result, instead of getting a memory that contains empty data and try to reason about it. IOW you can check for EFI_NOT_FOUND in both cases on the caller function. If you are working with 'dbx' then that's fine and you can continue. If you are working with 'db' you need to fail the authentication. This imho is much more readable. Regards /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] efi_loader: simplify tcg2_measure_secure_boot_variable() 2021-09-11 7:28 [PATCH 0/4] efi_loader: centralize known vendor GUIDs Heinrich Schuchardt ` (2 preceding siblings ...) 2021-09-11 7:28 ` [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() Heinrich Schuchardt @ 2021-09-11 7:28 ` Heinrich Schuchardt 3 siblings, 0 replies; 14+ messages in thread From: Heinrich Schuchardt @ 2021-09-11 7:28 UTC (permalink / raw) To: u-boot Cc: Ilias Apalodimas, AKASHI Takahiro, Sughosh Ganu, Masahisa Kojima, Alexander Graf, Heinrich Schuchardt Don't duplicate GUIDs. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- lib/efi_loader/efi_tcg2.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index beb224f66a..eb2c0a413c 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -11,6 +11,7 @@ #include <common.h> #include <dm.h> #include <efi_loader.h> +#include <efi_variable.h> #include <efi_tcg2.h> #include <log.h> #include <malloc.h> @@ -79,17 +80,12 @@ static const struct digest_info hash_algo_list[] = { }, }; -struct variable_info { - u16 *name; - const efi_guid_t *guid; -}; - -static struct variable_info secure_variables[] = { - {L"SecureBoot", &efi_global_variable_guid}, - {L"PK", &efi_global_variable_guid}, - {L"KEK", &efi_global_variable_guid}, - {L"db", &efi_guid_image_security_database}, - {L"dbx", &efi_guid_image_security_database}, +static const u16 *secure_variables[] = { + u"SecureBoot", + u"PK", + u"KEK", + u"db", + u"dbx", }; #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list) @@ -1587,19 +1583,20 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev) count = ARRAY_SIZE(secure_variables); for (i = 0; i < count; i++) { + const efi_guid_t *guid; + + guid = efi_auth_var_get_guid(secure_variables[i]); + /* * According to the TCG2 PC Client PFP spec, "SecureBoot", * "PK", "KEK", "db" and "dbx" variables must be measured * even if they are empty. */ - data = efi_get_var(secure_variables[i].name, - secure_variables[i].guid, - &data_size); + data = efi_get_var(secure_variables[i], guid, &data_size); ret = tcg2_measure_variable(dev, 7, EV_EFI_VARIABLE_DRIVER_CONFIG, - secure_variables[i].name, - secure_variables[i].guid, + secure_variables[i], guid, data_size, data); free(data); if (ret != EFI_SUCCESS) -- 2.30.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-10-01 19:08 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-11 7:28 [PATCH 0/4] efi_loader: centralize known vendor GUIDs Heinrich Schuchardt 2021-09-11 7:28 ` [PATCH 1/4] efi_loader: treat UEFI variable name as const Heinrich Schuchardt 2021-09-11 14:10 ` Ilias Apalodimas 2021-09-12 19:19 ` Heinrich Schuchardt 2021-09-11 7:28 ` [PATCH 2/4] efi_loader: function to get GUID for variable name Heinrich Schuchardt 2021-09-11 14:13 ` Ilias Apalodimas 2021-09-11 14:21 ` Ilias Apalodimas 2021-09-11 7:28 ` [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() Heinrich Schuchardt 2021-09-11 14:25 ` Ilias Apalodimas 2021-09-12 19:16 ` Heinrich Schuchardt 2021-09-12 19:23 ` Ilias Apalodimas 2021-10-01 16:42 ` Heinrich Schuchardt 2021-10-01 19:08 ` Ilias Apalodimas 2021-09-11 7:28 ` [PATCH 4/4] efi_loader: simplify tcg2_measure_secure_boot_variable() Heinrich Schuchardt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).