On 9/3/19 7:40 AM, AKASHI Takahiro wrote:
If EFI_VARIABLE_APPEND_WRITE is specified in attributes at efi_set_variable(), specified data will be appended to the variable's original value. Attributes other than APPEND_WRITE should not be modified.
With this patch, APPEND_WRITE test in 'variables' selftest will pass.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_variable.c | 50 +++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 6687b69a400d..5d1ee50a606e 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -423,18 +423,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data) {
- char *native_name = NULL, *val = NULL, *s;
- char *native_name = NULL, *oval, *val = NULL, *s;
Please, use old_val install of oval.
- size_t oval_size;
Please, use old_val_size.
efi_status_t ret = EFI_SUCCESS; u32 attr;
EFI_ENTRY(""%ls" %pUl %x %zu %p", variable_name, vendor, attributes, data_size, data);
- /* TODO: implement APPEND_WRITE */ if (!variable_name || !*variable_name || !vendor || ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
!(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||(attributes & EFI_VARIABLE_APPEND_WRITE)) {
!(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
Append with data_size = 0 should not delete the variable. See UEFI 2.8 spec.
If a variable does not exist, trying to delete it should return EFI_NOT_FOUND.
If a variable is READ_ONLY we have to return EFI_WRITE_PROTECTED and should not delete it.
I know these deficiencies are not caused by your patch. But as you are touching the code now, could you, please, consider them.
@@ -452,28 +451,37 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, goto out; }
- val = env_get(native_name);
- if (val) {
parse_attr(val, &attr);
- oval = env_get(native_name);
- if (oval) {
oval = parse_attr(oval, &attr);
/* We should not free val */val = NULL;
ret = EFI_WRITE_PROTECTED; goto out; }
/** attributes won't be changed* TODO: take care of APPEND_WRITE once supported*/if (attr != attributes) {
/* attributes won't be changed */if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) { ret = EFI_INVALID_PARAMETER; goto out;if (attributes & EFI_VARIABLE_APPEND_WRITE) {if (!prefix(oval, "(blob)")) {/* TODO: should support utf8? */
If the variable is read_only we should not append to it but return EFI_WRITE_PROTECTED.
We never stored any data as utf8. We can eliminate all references to the (utf8) prefix in a future patch.
In lib/efi_selftest/efi_selftest_variables.c we already have a test for APPEND. We should change the efi_st_todo() into efi_st_error() in a follow-up patch.
Thanks for taking care of this gap.
Best regards
Heinrich
return EFI_DEVICE_ERROR;goto out;}oval_size = strlen(oval);} else {oval_size = 0;}- } else {
oval_size = 0;
- val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
- val = malloc(oval_size + 2 * data_size
+ strlen("{ro,run,boot,nv}(blob)") + 1);@@ -481,10 +489,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
s = val;
- /*
* store attributes* TODO: several attributes are not supported*/
- /* store attributes */ attributes &= (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);
@@ -505,8 +510,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, } s += sprintf(s, "}");
- if (oval_size)
/* APPEND_WRITE */s += sprintf(s, oval);- else
s += sprintf(s, "(blob)");- /* store payload: */
- s += sprintf(s, "(blob)"); s = bin2hex(s, data, data_size); *s = '\0';