Re: [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE
On Tue, Sep 03, 2019 at 10:31:25PM +0200, Heinrich Schuchardt wrote: > 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 > >--- > > 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. Hmm, okay. > >+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))) { > > ret = EFI_INVALID_PARAMETER; > > goto out; > > } > > Append with data_size = 0 should not delete the variable. See UEFI 2.8 spec. Good point. Will add a check against it. > 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. Okay. > >@@ -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, ); > >+oval = env_get(native_name); > >+if (oval) { > >+oval = parse_attr(oval, ); > > > >-/* We should not free val */ > >-val = NULL; > > if (attr & READ_ONLY) { > > > 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. Agree here. > 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. Okay. I know that my patch passes the test itself. -Takahiro Akashi > 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); > > if (!val) { > > ret = EFI_OUT_OF_RESOURCES; > > goto out; > >@@ -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); >
Re: [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE
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 --- 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))) { ret = EFI_INVALID_PARAMETER; goto out; } 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, ); + oval = env_get(native_name); + if (oval) { + oval = parse_attr(oval, ); - /* We should not free val */ - val = NULL; if (attr & READ_ONLY) { 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); if (!val) { ret = EFI_OUT_OF_RESOURCES; goto out; @@ -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,
[U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE
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 --- 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; + size_t oval_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))) { ret = EFI_INVALID_PARAMETER; goto out; } @@ -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, ); + oval = env_get(native_name); + if (oval) { + oval = parse_attr(oval, ); - /* We should not free val */ - val = NULL; if (attr & READ_ONLY) { 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? */ + 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); if (!val) { ret = EFI_OUT_OF_RESOURCES; goto out; @@ -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'; -- 2.21.0 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot