* [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE
@ 2019-09-03 5:40 AKASHI Takahiro
2019-09-03 20:31 ` Heinrich Schuchardt
0 siblings, 1 reply; 3+ messages in thread
From: AKASHI Takahiro @ 2019-09-03 5:40 UTC (permalink / raw)
To: u-boot
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;
+ 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, &attr);
+ oval = env_get(native_name);
+ if (oval) {
+ oval = parse_attr(oval, &attr);
- /* 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE
2019-09-03 5:40 [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE AKASHI Takahiro
@ 2019-09-03 20:31 ` Heinrich Schuchardt
2019-09-05 9:33 ` AKASHI Takahiro
0 siblings, 1 reply; 3+ messages in thread
From: Heinrich Schuchardt @ 2019-09-03 20:31 UTC (permalink / raw)
To: u-boot
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))) {
> 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, &attr);
> + oval = env_get(native_name);
> + if (oval) {
> + oval = parse_attr(oval, &attr);
>
> - /* 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, data, data_size);
> *s = '\0';
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE
2019-09-03 20:31 ` Heinrich Schuchardt
@ 2019-09-05 9:33 ` AKASHI Takahiro
0 siblings, 0 replies; 3+ messages in thread
From: AKASHI Takahiro @ 2019-09-05 9:33 UTC (permalink / raw)
To: u-boot
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 <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.
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, &attr);
> >+ oval = env_get(native_name);
> >+ if (oval) {
> >+ oval = parse_attr(oval, &attr);
> >
> >- /* 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);
> >@@ -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';
> >
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-05 9:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-03 5:40 [U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE AKASHI Takahiro
2019-09-03 20:31 ` Heinrich Schuchardt
2019-09-05 9:33 ` AKASHI Takahiro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox