Message ID | 20240404074819.52156-1-ilias.apalodimas@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | efi_loader: move efi_var_collect to common functions | expand |
On 4/4/24 09:48, Ilias Apalodimas wrote: > From: Ilias Apalodimas <apalos@gmail.com> > > efi_var_collect() was initially placed in efi_var_file.c, since back > then we only supported efi variables stored in a file. Since then we > support variables stored in an RPMB as well and use that function to > collect variables that should be present at runtime. Thanks for cleaning this up. In the CONFIG_EFI_MM_COMM_TEE=y case we seem not to need efi_var_file.c anymore. Can we add this diff: --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -72,11 +72,11 @@ obj-y += efi_string.o obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o obj-y += efi_var_common.o obj-y += efi_var_mem.o -obj-y += efi_var_file.o ifeq ($(CONFIG_EFI_MM_COMM_TEE),y) obj-y += efi_variable_tee.o else obj-y += efi_variable.o +obj-y += efi_var_file.o obj-$(CONFIG_EFI_VARIABLES_PRESEED) += efi_var_seed.o endif obj-y += efi_watchdog.o Another comment below. > > So let's move it around in efi_var_common.c which makes more sense > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Signed-off-by: Ilias Apalodimas <apalos@gmail.com> > --- > lib/efi_loader/efi_var_common.c | 76 +++++++++++++++++++++++++++++++++ > lib/efi_loader/efi_var_file.c | 64 --------------------------- > 2 files changed, 76 insertions(+), 64 deletions(-) > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c > index 16b2c3d48828..07b9603d49f3 100644 > --- a/lib/efi_loader/efi_var_common.c > +++ b/lib/efi_loader/efi_var_common.c > @@ -9,6 +9,7 @@ > #include <efi_loader.h> > #include <efi_variable.h> > #include <stdlib.h> > +#include <u-boot/crc.h> > > enum efi_secure_mode { > EFI_MODE_SETUP, > @@ -416,3 +417,78 @@ void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) > > return buf; > } > + > +/** > + * efi_var_collect() - Copy EFI variables mstching attributes mask > + * > + * @bufp: buffer containing variable collection > + * @lenp: buffer length > + * @attr_mask: mask of matched attributes > + * > + * Return: Status code > + */ > +efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, > + u32 check_attr_mask) > +{ > + size_t len = EFI_VAR_BUF_SIZE; > + struct efi_var_file *buf; > + struct efi_var_entry *var, *old_var; > + size_t old_var_name_length = 2; > + > + *bufp = NULL; /* Avoid double free() */ > + buf = calloc(1, len); > + if (!buf) > + return EFI_OUT_OF_RESOURCES; > + var = buf->var; > + old_var = var; > + for (;;) { > + efi_uintn_t data_length, var_name_length; > + u8 *data; > + efi_status_t ret; > + > + if ((uintptr_t)buf + len <= > + (uintptr_t)var->name + old_var_name_length) > + return EFI_BUFFER_TOO_SMALL; > + > + var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name; > + memcpy(var->name, old_var->name, old_var_name_length); > + guidcpy(&var->guid, &old_var->guid); > + ret = efi_get_next_variable_name_int( > + &var_name_length, var->name, &var->guid); > + if (ret == EFI_NOT_FOUND) > + break; > + if (ret != EFI_SUCCESS) { > + free(buf); > + return ret; > + } > + old_var_name_length = var_name_length; > + old_var = var; > + > + data = (u8 *)var->name + old_var_name_length; > + data_length = (uintptr_t)buf + len - (uintptr_t)data; > + ret = efi_get_variable_int(var->name, &var->guid, > + &var->attr, &data_length, data, > + &var->time); > + if (ret != EFI_SUCCESS) { > + free(buf); > + return ret; > + } > + if ((var->attr & check_attr_mask) == check_attr_mask) { > + var->length = data_length; > + var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8); > + } > + } > + > + buf->reserved = 0; > + buf->magic = EFI_VAR_FILE_MAGIC; > + len = (uintptr_t)var - (uintptr_t)buf; > + buf->crc32 = crc32(0, (u8 *)buf->var, > + len - sizeof(struct efi_var_file)); > + buf->length = len; > + *bufp = buf; > + *lenp = len; > + > + return EFI_SUCCESS; > +} > + > + This produces a warning .git/rebase-apply/patch:95: new blank line at EOF. Could you, please, remove the blank lines at the end of the file. Best regards Heinrich > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > index 532b6b40eefe..413e1794e88c 100644 > --- a/lib/efi_loader/efi_var_file.c > +++ b/lib/efi_loader/efi_var_file.c > @@ -52,70 +52,6 @@ static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void) > return EFI_SUCCESS; > } > > -efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, > - u32 check_attr_mask) > -{ > - size_t len = EFI_VAR_BUF_SIZE; > - struct efi_var_file *buf; > - struct efi_var_entry *var, *old_var; > - size_t old_var_name_length = 2; > - > - *bufp = NULL; /* Avoid double free() */ > - buf = calloc(1, len); > - if (!buf) > - return EFI_OUT_OF_RESOURCES; > - var = buf->var; > - old_var = var; > - for (;;) { > - efi_uintn_t data_length, var_name_length; > - u8 *data; > - efi_status_t ret; > - > - if ((uintptr_t)buf + len <= > - (uintptr_t)var->name + old_var_name_length) > - return EFI_BUFFER_TOO_SMALL; > - > - var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name; > - memcpy(var->name, old_var->name, old_var_name_length); > - guidcpy(&var->guid, &old_var->guid); > - ret = efi_get_next_variable_name_int( > - &var_name_length, var->name, &var->guid); > - if (ret == EFI_NOT_FOUND) > - break; > - if (ret != EFI_SUCCESS) { > - free(buf); > - return ret; > - } > - old_var_name_length = var_name_length; > - old_var = var; > - > - data = (u8 *)var->name + old_var_name_length; > - data_length = (uintptr_t)buf + len - (uintptr_t)data; > - ret = efi_get_variable_int(var->name, &var->guid, > - &var->attr, &data_length, data, > - &var->time); > - if (ret != EFI_SUCCESS) { > - free(buf); > - return ret; > - } > - if ((var->attr & check_attr_mask) == check_attr_mask) { > - var->length = data_length; > - var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8); > - } > - } > - > - buf->reserved = 0; > - buf->magic = EFI_VAR_FILE_MAGIC; > - len = (uintptr_t)var - (uintptr_t)buf; > - buf->crc32 = crc32(0, (u8 *)buf->var, > - len - sizeof(struct efi_var_file)); > - buf->length = len; > - *bufp = buf; > - *lenp = len; > - > - return EFI_SUCCESS; > -} > - > /** > * efi_var_to_file() - save non-volatile variables as file > *
On Fri, 5 Apr 2024 at 09:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 4/4/24 09:48, Ilias Apalodimas wrote: > > From: Ilias Apalodimas <apalos@gmail.com> > > > > efi_var_collect() was initially placed in efi_var_file.c, since back > > then we only supported efi variables stored in a file. Since then we > > support variables stored in an RPMB as well and use that function to > > collect variables that should be present at runtime. > > Thanks for cleaning this up. > > In the CONFIG_EFI_MM_COMM_TEE=y case we seem not to need efi_var_file.c > anymore. Can we add this diff: > > --- a/lib/efi_loader/Makefile > +++ b/lib/efi_loader/Makefile > @@ -72,11 +72,11 @@ obj-y += efi_string.o > obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o > obj-y += efi_var_common.o > obj-y += efi_var_mem.o > -obj-y += efi_var_file.o > ifeq ($(CONFIG_EFI_MM_COMM_TEE),y) > obj-y += efi_variable_tee.o > else > obj-y += efi_variable.o > +obj-y += efi_var_file.o > obj-$(CONFIG_EFI_VARIABLES_PRESEED) += efi_var_seed.o > endif > obj-y += efi_watchdog.o Yes, I'll add it on v2 Thanks /Ilias > > Another comment below. > > > > > So let's move it around in efi_var_common.c which makes more sense > > > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Signed-off-by: Ilias Apalodimas <apalos@gmail.com> > > --- > > lib/efi_loader/efi_var_common.c | 76 +++++++++++++++++++++++++++++++++ > > lib/efi_loader/efi_var_file.c | 64 --------------------------- > > 2 files changed, 76 insertions(+), 64 deletions(-) > > > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c > > index 16b2c3d48828..07b9603d49f3 100644 > > --- a/lib/efi_loader/efi_var_common.c > > +++ b/lib/efi_loader/efi_var_common.c > > @@ -9,6 +9,7 @@ > > #include <efi_loader.h> > > #include <efi_variable.h> > > #include <stdlib.h> > > +#include <u-boot/crc.h> > > > > enum efi_secure_mode { > > EFI_MODE_SETUP, > > @@ -416,3 +417,78 @@ void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) > > > > return buf; > > } > > + > > +/** > > + * efi_var_collect() - Copy EFI variables mstching attributes mask > > + * > > + * @bufp: buffer containing variable collection > > + * @lenp: buffer length > > + * @attr_mask: mask of matched attributes > > + * > > + * Return: Status code > > + */ > > +efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, > > + u32 check_attr_mask) > > +{ > > + size_t len = EFI_VAR_BUF_SIZE; > > + struct efi_var_file *buf; > > + struct efi_var_entry *var, *old_var; > > + size_t old_var_name_length = 2; > > + > > + *bufp = NULL; /* Avoid double free() */ > > + buf = calloc(1, len); > > + if (!buf) > > + return EFI_OUT_OF_RESOURCES; > > + var = buf->var; > > + old_var = var; > > + for (;;) { > > + efi_uintn_t data_length, var_name_length; > > + u8 *data; > > + efi_status_t ret; > > + > > + if ((uintptr_t)buf + len <= > > + (uintptr_t)var->name + old_var_name_length) > > + return EFI_BUFFER_TOO_SMALL; > > + > > + var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name; > > + memcpy(var->name, old_var->name, old_var_name_length); > > + guidcpy(&var->guid, &old_var->guid); > > + ret = efi_get_next_variable_name_int( > > + &var_name_length, var->name, &var->guid); > > + if (ret == EFI_NOT_FOUND) > > + break; > > + if (ret != EFI_SUCCESS) { > > + free(buf); > > + return ret; > > + } > > + old_var_name_length = var_name_length; > > + old_var = var; > > + > > + data = (u8 *)var->name + old_var_name_length; > > + data_length = (uintptr_t)buf + len - (uintptr_t)data; > > + ret = efi_get_variable_int(var->name, &var->guid, > > + &var->attr, &data_length, data, > > + &var->time); > > + if (ret != EFI_SUCCESS) { > > + free(buf); > > + return ret; > > + } > > + if ((var->attr & check_attr_mask) == check_attr_mask) { > > + var->length = data_length; > > + var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8); > > + } > > + } > > + > > + buf->reserved = 0; > > + buf->magic = EFI_VAR_FILE_MAGIC; > > + len = (uintptr_t)var - (uintptr_t)buf; > > + buf->crc32 = crc32(0, (u8 *)buf->var, > > + len - sizeof(struct efi_var_file)); > > + buf->length = len; > > + *bufp = buf; > > + *lenp = len; > > + > > + return EFI_SUCCESS; > > +} > > + > > + > > This produces a warning > > .git/rebase-apply/patch:95: new blank line at EOF. > > Could you, please, remove the blank lines at the end of the file. > > Best regards > > Heinrich > > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > > index 532b6b40eefe..413e1794e88c 100644 > > --- a/lib/efi_loader/efi_var_file.c > > +++ b/lib/efi_loader/efi_var_file.c > > @@ -52,70 +52,6 @@ static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void) > > return EFI_SUCCESS; > > } > > > > -efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, > > - u32 check_attr_mask) > > -{ > > - size_t len = EFI_VAR_BUF_SIZE; > > - struct efi_var_file *buf; > > - struct efi_var_entry *var, *old_var; > > - size_t old_var_name_length = 2; > > - > > - *bufp = NULL; /* Avoid double free() */ > > - buf = calloc(1, len); > > - if (!buf) > > - return EFI_OUT_OF_RESOURCES; > > - var = buf->var; > > - old_var = var; > > - for (;;) { > > - efi_uintn_t data_length, var_name_length; > > - u8 *data; > > - efi_status_t ret; > > - > > - if ((uintptr_t)buf + len <= > > - (uintptr_t)var->name + old_var_name_length) > > - return EFI_BUFFER_TOO_SMALL; > > - > > - var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name; > > - memcpy(var->name, old_var->name, old_var_name_length); > > - guidcpy(&var->guid, &old_var->guid); > > - ret = efi_get_next_variable_name_int( > > - &var_name_length, var->name, &var->guid); > > - if (ret == EFI_NOT_FOUND) > > - break; > > - if (ret != EFI_SUCCESS) { > > - free(buf); > > - return ret; > > - } > > - old_var_name_length = var_name_length; > > - old_var = var; > > - > > - data = (u8 *)var->name + old_var_name_length; > > - data_length = (uintptr_t)buf + len - (uintptr_t)data; > > - ret = efi_get_variable_int(var->name, &var->guid, > > - &var->attr, &data_length, data, > > - &var->time); > > - if (ret != EFI_SUCCESS) { > > - free(buf); > > - return ret; > > - } > > - if ((var->attr & check_attr_mask) == check_attr_mask) { > > - var->length = data_length; > > - var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8); > > - } > > - } > > - > > - buf->reserved = 0; > > - buf->magic = EFI_VAR_FILE_MAGIC; > > - len = (uintptr_t)var - (uintptr_t)buf; > > - buf->crc32 = crc32(0, (u8 *)buf->var, > > - len - sizeof(struct efi_var_file)); > > - buf->length = len; > > - *bufp = buf; > > - *lenp = len; > > - > > - return EFI_SUCCESS; > > -} > > - > > /** > > * efi_var_to_file() - save non-volatile variables as file > > * >
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 16b2c3d48828..07b9603d49f3 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -9,6 +9,7 @@ #include <efi_loader.h> #include <efi_variable.h> #include <stdlib.h> +#include <u-boot/crc.h> enum efi_secure_mode { EFI_MODE_SETUP, @@ -416,3 +417,78 @@ void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) return buf; } + +/** + * efi_var_collect() - Copy EFI variables mstching attributes mask + * + * @bufp: buffer containing variable collection + * @lenp: buffer length + * @attr_mask: mask of matched attributes + * + * Return: Status code + */ +efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, + u32 check_attr_mask) +{ + size_t len = EFI_VAR_BUF_SIZE; + struct efi_var_file *buf; + struct efi_var_entry *var, *old_var; + size_t old_var_name_length = 2; + + *bufp = NULL; /* Avoid double free() */ + buf = calloc(1, len); + if (!buf) + return EFI_OUT_OF_RESOURCES; + var = buf->var; + old_var = var; + for (;;) { + efi_uintn_t data_length, var_name_length; + u8 *data; + efi_status_t ret; + + if ((uintptr_t)buf + len <= + (uintptr_t)var->name + old_var_name_length) + return EFI_BUFFER_TOO_SMALL; + + var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name; + memcpy(var->name, old_var->name, old_var_name_length); + guidcpy(&var->guid, &old_var->guid); + ret = efi_get_next_variable_name_int( + &var_name_length, var->name, &var->guid); + if (ret == EFI_NOT_FOUND) + break; + if (ret != EFI_SUCCESS) { + free(buf); + return ret; + } + old_var_name_length = var_name_length; + old_var = var; + + data = (u8 *)var->name + old_var_name_length; + data_length = (uintptr_t)buf + len - (uintptr_t)data; + ret = efi_get_variable_int(var->name, &var->guid, + &var->attr, &data_length, data, + &var->time); + if (ret != EFI_SUCCESS) { + free(buf); + return ret; + } + if ((var->attr & check_attr_mask) == check_attr_mask) { + var->length = data_length; + var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8); + } + } + + buf->reserved = 0; + buf->magic = EFI_VAR_FILE_MAGIC; + len = (uintptr_t)var - (uintptr_t)buf; + buf->crc32 = crc32(0, (u8 *)buf->var, + len - sizeof(struct efi_var_file)); + buf->length = len; + *bufp = buf; + *lenp = len; + + return EFI_SUCCESS; +} + + diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index 532b6b40eefe..413e1794e88c 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -52,70 +52,6 @@ static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void) return EFI_SUCCESS; } -efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, - u32 check_attr_mask) -{ - size_t len = EFI_VAR_BUF_SIZE; - struct efi_var_file *buf; - struct efi_var_entry *var, *old_var; - size_t old_var_name_length = 2; - - *bufp = NULL; /* Avoid double free() */ - buf = calloc(1, len); - if (!buf) - return EFI_OUT_OF_RESOURCES; - var = buf->var; - old_var = var; - for (;;) { - efi_uintn_t data_length, var_name_length; - u8 *data; - efi_status_t ret; - - if ((uintptr_t)buf + len <= - (uintptr_t)var->name + old_var_name_length) - return EFI_BUFFER_TOO_SMALL; - - var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name; - memcpy(var->name, old_var->name, old_var_name_length); - guidcpy(&var->guid, &old_var->guid); - ret = efi_get_next_variable_name_int( - &var_name_length, var->name, &var->guid); - if (ret == EFI_NOT_FOUND) - break; - if (ret != EFI_SUCCESS) { - free(buf); - return ret; - } - old_var_name_length = var_name_length; - old_var = var; - - data = (u8 *)var->name + old_var_name_length; - data_length = (uintptr_t)buf + len - (uintptr_t)data; - ret = efi_get_variable_int(var->name, &var->guid, - &var->attr, &data_length, data, - &var->time); - if (ret != EFI_SUCCESS) { - free(buf); - return ret; - } - if ((var->attr & check_attr_mask) == check_attr_mask) { - var->length = data_length; - var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8); - } - } - - buf->reserved = 0; - buf->magic = EFI_VAR_FILE_MAGIC; - len = (uintptr_t)var - (uintptr_t)buf; - buf->crc32 = crc32(0, (u8 *)buf->var, - len - sizeof(struct efi_var_file)); - buf->length = len; - *bufp = buf; - *lenp = len; - - return EFI_SUCCESS; -} - /** * efi_var_to_file() - save non-volatile variables as file *